Refactoring is the process of changing the internal structure of a program without changing its functionality.  Java programmers have come to depend on automated refactoring support in the IDE.  Other languages are not so well represented.  Untyped languages do not lend themselves to automated refactorings so easily.  Javascript is one such language.  However, Javascript’s variable scoping rules make at least one refactoring pretty easy:  extract function.

The refactoring extract function has a few simple steps.

1. Place a set of braces around the code to be extracted.
2. add a function declaration before the braces, and a call to the function after the braces
3. Run the code to test it still works.
4. Move the function outside the scope of the containing function.
5. Run the code.   It will likely now have an error.
6. expand the scope of the now undefined defined variables –OR– make them parameters passed to the newly created function.  Continue until the function runs again.

Here’s and example from the FreeIPA file entity.js. This is the starting state:

IPA.nested_tabs = function(entity_name){

    var siblings = [];
    var nested_index;
    var nested_entities;
    var label;
    if (!IPA.tab_set) {
        siblings.push(entity_name);
        return siblings;
    }

    for (var top_tab_index = 0;
         top_tab_index < IPA.tab_set.length;
         top_tab_index += 1){
        var top_tab =  IPA.tab_set[top_tab_index];
        for (var subtab_index = 0;
             subtab_index < top_tab.children.length;
             subtab_index += 1){
            if(top_tab.children[subtab_index].name){
                if (top_tab.children[subtab_index].name === entity_name){
                    siblings.push(entity_name);
                    IPA.nested_tab_labels[entity_name] =
                        top_tab.children[subtab_index].label;
                    if (top_tab.children[subtab_index].children){
                        label = top_tab.children[subtab_index].label;
                        nested_entities = top_tab.children[subtab_index].children;
                        for ( nested_index = 0;
                              nested_index < nested_entities.length;
                              nested_index += 1){
                            siblings.push (nested_entities[nested_index].name);
                            IPA.nested_tab_labels[entity_name] =
                                top_tab.children[subtab_index].label;
                        }

                    }
                }else{
                    if (top_tab.children[subtab_index].children){
                        nested_entities = top_tab.children[subtab_index].children;
                        for (nested_index = 0;
                             nested_index < nested_entities.length;
                             nested_index += 1){
                            if (nested_entities[nested_index].name === entity_name){
                                siblings.push(top_tab.children[subtab_index].name);
                                IPA.nested_tab_labels[entity_name] =
                                    top_tab.children[subtab_index].label;

                                for (var nested_index2 = 0;
                                     nested_index2 < nested_entities.lengththe;
                                     nested_index2 += 1){
                                    siblings.push(nested_entities[nested_index2].name);
                                IPA.nested_tab_labels[nested_entities[nested_index2].name] =
                                    top_tab.children[subtab_index].label;

                                }
                            }
                        }
                    }
                }
            }
        }
    }
    return siblings;
};

It is hard to read in the browser, but if you look in the downloaded file, or click on the white icon with the blue angle-brackets, it is easier to read.

I don't have a deliberate unit test for this, but fortunately, I know that this code is run for the tabs when ever I reload the page. Due to our usage of fixtures, I don't need to redeploy after each change, so I can test this fairly quickly by just reloading the browser.

Lets start by attacking a small chunk of code: the body of the for loop that starts at line 27:

for ( nested_index = 0;
        nested_index < nested_entities.length;
        nested_index += 1){
            siblings.push (nested_entities[nested_index].name);
            IPA.nested_tab_labels[entity_name] =
                                top_tab.children[subtab_index].label;
}

After we execute steps one and two of the refactoring, the code looks like this:

 for ( nested_index = 0;
    nested_index < nested_entities.length;
    nested_index += 1){
        function push_sibling(){
            siblings.push (nested_entities[nested_index].name);
            IPA.nested_tab_labels[entity_name] =
            top_tab.children[subtab_index].label;
    }
    push_sibling();
}

I called the function push_sibling. Test and run, and everything works out OK. Now, I move the function out of this position, and to the top of the function nested_tabs , which is the start of converting it from a function toward more of an object. This minor change does not affect the scope of any variables, and I can claim that this part is complete. However, the chunk of code at line 52 in the original listing looks very similar.

 siblings.push(nested_entities[nested_index2].name);
 IPA.nested_tab_labels[nested_entities[nested_index2].name] = 
    top_tab.children[subtab_index].label;

The only differences are the index variables. Lets introduce them as parameters to the function.

  function push_sibling(index,key){
        siblings.push (nested_entities[index].name);
        IPA.nested_tab_labels[key] =
            top_tab.children[subtab_index].label;
    }

And call it:

  push_sibling(nested_index,entity_name);

Now, at line 52, we extract the method the same way as before. This time, I'll name the extracted function push_sibling2 Once we are calling the extracted function, we switch to calling the parametrized version of push_sibling, and retest. The code at line 52 now looks like this:

 function push_sibling2(){
     siblings.push(nested_entities[nested_index2].name);
     IPA.nested_tab_labels[nested_entities[nested_index2].name] =
         top_tab.children[subtab_index].label;
}
//push_sibling2();
push_sibling(nested_index2,nested_entities[nested_index2].name);

Once the test works, we can remove the function definition and the commented out function call. This example removed two lines of redundant code. More applications of this refactoring will whittle it down significantly. The more important effect will be how much more readable and the code is after refactoring.

There are a couple other places where the code is similar. What is clear is that we always need a key for IPA.nested_tab_labels and a sibling. Pretty soon, I have the code looking like this.

IPA.nested_tabs = function(entity_name){

    var siblings = [];
    var i;
    var i2;
    var nested_entities;
    var sub_i;

    function push_sibling(key,sibling){
        siblings.push (sibling);
        IPA.nested_tab_labels[key] = top_tab.children[sub_i].label;
    }

    if (!IPA.tab_set) {
        siblings.push(entity_name);
        return siblings;
    }

    for (var top_i = 0; top_i < IPA.tab_set.length; top_i += 1){
        var top_tab =  IPA.tab_set[top_i];
        for (sub_i = 0; sub_i < top_tab.children.length; sub_i += 1){
            if (top_tab.children[sub_i].name === entity_name){
                push_sibling(entity_name ,entity_name);
                if (top_tab.children[sub_i].children){
                    nested_entities = top_tab.children[sub_i].children;
                    for ( i = 0;  i < nested_entities.length; i += 1){
                        push_sibling(entity_name, nested_entities[i].name);
                    }
                }
            }else{
                if (top_tab.children[sub_i].children){
                    nested_entities = top_tab.children[sub_i].children;
                    for (i = 0; i < nested_entities.length; i += 1){
                        if (nested_entities[i].name === entity_name){
                            push_sibling(entity_name,top_tab.children[sub_i].name);
                            for (i2 = 0; i2 < nested_entities.length; i2 += 1){
                                push_sibling(nested_entities[i2].name,nested_entities[i2].name);
                            }
                        }
                    }
                }
            }
        }
    }
    return siblings;
};

OK, I cheated a little, too: there is an if statement checking for a pre-condition in the original code that is really not needed, so I removed it. There is also an assignment to the variable label that was never used, and I removed that, too.

This function isn't done, yet, but by now you should see the direction a complete refactoring would head. I'll update this post with the completed refactoring once it is done.

Update: I think this is as far as I'm going to take this function. I'm not sure if I like the key variable, either.

/*Returns the entity requested, as well as:
  any nested tabs underneath it or
  its parent tab and the others nested at the same level*/

IPA.nested_tabs = function(entity_name){

    var siblings = [];
    var i;
    var i2;
    var nested_entities;
    var sub_i;
    var sub_tab;

    var key = entity_name;
    function push_sibling(sibling){
        siblings.push (sibling);
        IPA.nested_tab_labels[key] = sub_tab;
    }


    if (!IPA.tab_set) {
        siblings.push(entity_name);
        return siblings;
    }

    for (var top_i = 0; top_i < IPA.tab_set.length; top_i += 1){
        var top_tab =  IPA.tab_set[top_i];
        for (sub_i = 0; sub_i < top_tab.children.length; sub_i += 1){
            sub_tab = top_tab.children[sub_i];
            nested_entities = sub_tab.children;
            if (sub_tab.name === entity_name){
                push_sibling(entity_name);
            }
            if (sub_tab.children){
                for (i = 0; i < nested_entities.length; i += 1){
                    if (sub_tab.name === entity_name){
                        push_sibling(nested_entities[i].name);
                    }else{
                        if (nested_entities[i].name === entity_name){
                            push_sibling(sub_tab.name);
                            for (i2 = 0; i2 < nested_entities.length; i2 += 1){
                                key = nested_entities[i].name;
                                push_sibling(nested_entities[i2].name);
                            }
                        }
                    }
                }
            }
        }
    }
    return siblings;
};