Code read #243 ;-)

As a team leader, I really enjoy working with new programmers, to learn and to experience see their progress as they advance. One of the best and most important way is code read. To take some code they wrote, and to read it with them step by step, sujesting refactoring where apropriate. So here is a fresh example of a code read/refactor.

This is what I have seen at the beginning:

myObj.initPackages = function () {
     let tempPackages = JSON.parse(myObj.packages);
     myObj.uiPackages = [];
     tempPackages.links ? createUiLinks() : "";
     tempPackages.devtools && tempPackages.devtools.github ? createUiGit() : "";
     tempPackages.devtools && tempPackages.devtools.bitbucket ? createUiBit() : "";

     function createUiLinks() {
         let linksArr = tempPackages.links;
         for (let i = 0; i < linksArr.length; i++) {
             myObj.uiPackages.push({
                 packageType: 'Link',
                 packageLink: linksArr[i]
             })
         }
     }

     function createUiGit() {
         let gitArr = tempPackages.devtools.github;
         for (let i = 0; i < gitArr.length; i++) {
             myObj.uiPackages.push({
                 packageType: 'Github',
                 packageLink: gitArr[i]
             })
         }
     }

     function createUiBit() {
         let bitArr = tempPackages.devtools.bitbucket;
         for (let i = 0; i < bitArr.length; i++) {
             myObj.uiPackages.push({
                 packageType: 'Bitbucket',
                 packageLink: bitArr[i]
             })
         }
     }
 }S

Safety net (Unit testing)

Before we start, it’s best to have a safety net, by creating some basic primitive testing. This way we can do whatever we want with the code without worrying about changing the results.

myObj.packages = `
   {
      "links":        ["link 1",  "line 2"],
      "devtools": {
         "github":    ["git 1",   "git 2"],
         "bitbucket": ["bit 1",   "bit 2"]
      }
   }
 `;
 myObj.initPackages();

if(
    myObj.uiPackages.length != 6 ||
    myObj.uiPackages[0].packageType != 'Links' ||
    myObj.uiPackages[0].packageLink != 'link 1' ||
    ...
    ...
 ) alert('Ops')C

First thing caught my eye was this line:

tempPackages.links ? createUiLinks() : "";

There is no clue of what is it doing. But before that, the “else” is doing nothing. So we started by removing it.

if(tempPackages.links){
   createUiLinks();
}

And still, there is no clue of what is it doing. Is it changing something? Is it a static function or it depends on some data? Is it changing something? and if so, what?

Let’s look inside:

function createUiLinks() {
   let linksArr = tempPackages.links;
   for (let i = 0; i < linksArr.length; i++) {
      myObj.uiPackages.push({
         packageType: 'Link',
         packageLink: linksArr[i]
      });
   }
}

We see that this function is based on outside values. Worse than that, the function changes a value without telling us. So lets expose these two values:

if(tempPackages.links){
   createUiLinks(myObj.uiPackages, tempPackages.links);
}

function createUiLinks(uiPackages,links) {
   for (let i = 0; i < links.length; i++) {
      uiPackages.push({
         packageType: 'Link',
         packageLink: links[i]
      });
   }
}

As you can see, we have a condition outside the function upon the same value we iterate inside, so why not move it into the function and make the main lojic readable.

createUiLinks(myObj.uiPackages, tempPackages.links);

function createUiLinks(uiPackages,links) {
   if(links){
      for (let i = 0; i < links.length; i++) {
         uiPackages.push({
            packageType: 'Link',
            packageLink: links[i]
         });
      }
   }
}

Using some of the ES6 options: Default parameters and array iterators:

function createUiLinks(uiPackages,links = []) {
     links.forEach(link => {
         uiPackages.push({
            packageType: 'Link',
            packageLink: link
         });
      });
   }
}

And since the next two functions are the same, we can do one more change and remove the DRY code:

createUiLinks(myObj.uiPackages, 'Link', tempPackages.links);

if(tempPackages.devtools){
   createUiLinks(myObj.uiPackages, 'Github', tempPackages.devtools.github);
   createUiLinks(myObj.uiPackages, 'Bitbucket',  tempPackages.devtools.bitbucket);
} 

function createUiLinks(uiPackages,type,links=[]) {
   links.forEach(link => {
      uiPackages.push({
         packageType: type,
         packageLink: link
      }
   });
}

The next code smell is the three varibles. It is hard to recognize the value we change inside the function. We would want to do something like this…

myObj.uiPackages = createUiLinks('Link', tempPackages.links);

if(tempPackages.devtools){
   myObj.uiPackages = createUiLinks('Github', tempPackages.devtools.github);
   myObj.uiPackages = createUiLinks('Bitbucket', tempPackages.devtools.bitbucket);
} 

Because the function is based on the previous values we can’t without passing it in and out. So here we will use the spread method with the array push method:

myObj.uiPackages = createUiLinks('Link', tempPackages.links);

if(tempPackages.devtools){
   let gitLinks = createUiLinks('Github', tempPackages.devtools.github));
   myObj.uiPackages.push(...gitLinks);
   
   let bitLinks = createUiLinks('Bitbucket', tempPackages.devtools.bitbucket));
   myObj.uiPackages.push(...bitLinks);      
} 

And with the ES6 map method it’s even shorter

function createUiLinks(type,links = []) {
  return links.map(link => {
     return {
        packageType: type,
        packageLink: link
     }
  });
}

Lets sum it up:

myObj.initPackages = function () {
   let tempPackages = JSON.parse(myObj.packages);
   myObj.uiPackages = createUiLinks('Link', tempPackages.links);

   if(tempPackages.devtools){
      let gitLinks = createUiLinks('Github', tempPackages.devtools.github);
      myObj.uiPackages.push(...gitLinks);
   
      let bitLinks = createUiLinks('Bitbucket', tempPackages.devtools.bitbucket);
      myObj.uiPackages.push(...bitLinks); 
   } 

   function createUiLinks(type,links = []) {
      return links.map(link => {
         return {
            packageType: type,
            packageLink: link
         }
      });
   }
}

There is a thin line between to long code and unreadable shortcuts. The art of clean code is walking on that line. Making the code as readable and short as possible, so the next time someone reads it, it will be as clear as the sky outside after a long working day.