Search code examples
javascriptoptimizationrefactoringcode-snippets

Nesting helper functions inside a function for readability purposes


So I am currently reading through Clean Code and I really like the idea of super small functions that each tell their own "story". I also really like the way he puts how code should be written to be read in terms of "TO paragraphs", which I've decided to kind of rename in my head to "in order to"

Anyway I have been refactoring alot of code to include more meaningful names and to make the flow in which it will be read a little better and I have stumbled into something that I am unsure on and maybe some gurus here could give me some solid advice!

I know that code-styles is a highly controversial and subjective topic, but hopefully I wont get reamed out by this post.

Thanks everyone!

PSA: I am a noob, fresh out of College creating a web app using the MEAN stack for an internal project in an internship at the moment.

Clean Code refactor

 //Modal Controller stuff above.  vm.task is an instance variable
vm.task = vm.data.task;
castTaskDataTypesForForm();

  function castTaskDataTypesForForm() {
    castPriorityToInt();
    castReminderInHoursToInt();
    castDueDateToDate();
    getAssigneObjFromAssigneeString();
  }

  function castPriorityToInt() {
    vm.task.priority = vm.task.priority === undefined ?
      0 : parseInt(vm.task.priority);
  }

  function castReminderInHoursToInt() {
    vm.task.reminderInHours = vm.task.reminderInHours === undefined ?
      0 : parseInt(vm.task.reminderInHours);
  }

  function castDueDateToDate() {
    vm.task.dueDate = new Date(vm.task.dueDate);
  }

  function getAssigneObjFromAssigneeString() {
    vm.task.assignee = getUserFromId(vm.task.assignee);
  }

Possibly better refactor? / My question ----------------------------

//Modal Controller stuff above.  vm.task is an instance variable
vm.task = vm.data.task;
castTaskDataTypesForForm();

  function castTaskDataTypesForForm() {
    castPriorityToInt();
    castReminderInHoursToInt();
    castDueDateToDate();
    getAssigneObjFromAssigneeString();

    function castPriorityToInt() {
      vm.task.priority = vm.task.priority === undefined ?
        0 : parseInt(vm.task.priority);
    }

    function castReminderInHoursToInt() {
      vm.task.reminderInHours = vm.task.reminderInHours === undefined ?
        0 : parseInt(vm.task.reminderInHours);
    }

    function castDueDateToDate() {
      vm.task.dueDate = new Date(vm.task.dueDate);
    }

    function getAssigneObjFromAssigneeString() {
      vm.task.assignee = getUserFromId(vm.task.assignee);
    }
  }

Solution

  • Posting the IIFE example here so I have more room to work with. I'm not saying this is the best option, it's the one I would use with the info the OP gave us.

    var castTaskDataTypesForForm = (function() {
        var castPriorityToInt = function castPriorityToInt() { ... },
            castReminderInHoursToInt = function castReminderInHoursToInt() { .. },
            castDueDateToDate = function castDueDateToDate() { ... },
            getAssigneObjFromAssigneeString = function getAssigneObjFromAssigneeString() { ... };
        return function castTaskDataTypesForForm() {
            castPriorityToInt();
            castReminderInHoursToInt();
            castDueDateToDate();
            getAssigneObjFromAssigneeString();
        };
    }());
    vm.task = vm.data.task;
    castTaskDataTypesForForm();
    

    This way the helper functions only get defined once and are kept private inside the closure. You can obv remove the var x = function x syntax if you prefer the function x() style.

    edit: If the function only gets called once, your own examples are probably the cleaner code. The reason you'd use the IIFE syntax would be to keep the helper functions only accesible by the main function, like in your own second example.