The Refactoring Tales

By Jack Franklin (@jack_franklin)

This book is freely available to read online, but you can purchase PDF, MOBI and EPUB versions for a small fee to help support The JavaScript Playground. Find out more and purchase digital download editions here.

The book is also not yet fully completed, and more content is to come very soon. Follow The JavaScript Playground on Twitter to keep up to date with changes.

Introduction

Welcome to The Refactoring Tales, a book that documents some of the refactorings and changes I’ve made in recent (and mostly real-life) projects. This book isn’t going to teach you about language constructs, conditionals, functions, or so on, but hopefully offer insight into how to take steps to make your code more readable and more importantly, maintainable.

Think of how much time you spend maintaining code, rather than being able to write code from scratch. Day to day, I’m not typically creating new projects, but I am maintaining, editing or refactoring existing projects. This book is just like that. Each chapter will start by looking at some existing code, and over the course of a few pages we will examine, dissect and then refactor the code into an improved alternative. Of course, the idea of code being “better” is largely subjective, but even if you don’t quite agree with every step I take, you should be able to see the overall benefits.

The GitHub repository for this book is here: https://github.com/jackfranklin/the-refactoring-tales, both the raw book files and the code samples are all there for you to take. If you spot any issues as you read this book, a new issue (or even better, a pull request!) is greatly appreciated.

Thanks

There are so many people who have thanked me along the way that it would be wrong to not include some form of list of names in this book. Thanks in particular go to Addy Osmani, Drew Neil, Guy Routledge, Katja Durrani, Adam Yeats, Stu Robson and Todd Motto along with many more who have shaped this book since the idea formed.

Changelog

If you’ve been reading this book since the beta, to help you know what’s changed, a list of changes and dates of update will be kept below.

21/07/14

Refactoring

Before we continue I think it’s important to define just what exactly I mean when I say “refactoring”. Refactoring has lots of definitions, but I think my favourite and the one I stick to is that of Martin Fowler, in his book on refactoring:

Refactoring is a controlled technique for improving the design of an existing code base. Its essence is applying a series of small behaviour-preserving transformations, each of which “too small to be worth doing”.

We apply small changes to the code, and do so several times, until we’re left with a section of code whose design is greatly improved. Notice how they are behaviour-preserving transformations: a refactor should never change behaviour, merely improve the code. The idea of “better” code may be subjective, but to me when I think of good qualities for code to have, I think of it being:

In his book Professional JavaScript for Web Developers, Nicholas Zakas has a brilliant definition of maintainability which cites the following characteristics: Understandable, intuitive, adaptable, extendable and debuggable. It’s these traits that I hope this book will help you adhere to and aim for.

Tests

When refactoring, you need to have confidence in the fact that you’ve not broken anything. Similarly, if you do break something, you need to know immediately. This is where the benefit of having tests plays a key role. The ability to run a set of tests and get immediate feedback is fantastic, and that’s precisely what tests give you. It’s very easy these days to get started with tests, whether you’re writing Ruby, or JavaScript on the server with NodeJS, or in the browser, there are a myriad of tools available to you. This isn’t a book on testing, and I could write in huge depth on the subject, so for each example in this book, you should assume that I have tested it as I’ve gone (in actual fact for every example there is, I did have tests every time). If you want to refactor but don’t have tests, write tests. Do not ever attempt to refactor without writing any tests. Ever.

You should also make sure you can run your tests easily. I have mine hooked up through my editor, Vim, so I can hit a 3 key shortcut and have my tests run. Configure it however you want, and make sure it’s easy to do.

Tale 1: Terrible Tabs

One of the most common things any new jQuery user will try to do is build some basic HTML tabs. It’s like the initiation into the jQuery world (not quite, that’s carousels, which are next up) and it serves as a good starting point to show you the first refactoring.

Here’s a JavaScript function called tabularize which, as you might expect, is a small function for creating a tabbed interface.

var tabularize = function() {
  var active = location.hash;
  if(active) {
    $(".tabs").children("div").hide();
    $(active).show();
    $(".active").removeClass("active");
    $(".tab-link").each(function() {
      if($(this).attr("href") === active) {
        $(this).parent().addClass("active");
      }
    });
  }
  $(".tabs").find(".tab-link").click(function() {
    $(".tabs").children("div").hide();
    $($(this).attr("href")).show();
    $(".active").removeClass("active");
    $(this).parent().addClass("active");
    return false;
  });
};

The first part of the function deals with the case where there is a fragment identifier in the URL. For example, if someone visits mysite.com/#tab2, they should have tab 2 active when they load the page. The second part deals with a user clicking on a tab, and updating the content accordingly.

To help you put this together, here is the HTML that the code is applied to.

<div class="tabs">
  <ul>
    <li class="active"><a href="#tab1" class="tab-link">Tab 1</a></li>
    <li><a href="#tab2" class="tab-link">Tab 2</a></li>
    <li><a href="#tab3" class="tab-link">Tab 3</a></li>
  </ul>
  <div id="tab1">
    <h3>Tab 1</h3>
    <p>Lorem ipsum dolor sit amet</p>
  </div>
  <div id="tab2">
    <h3>Tab 2</h3>
    <p>Lorem ipsum dolor sit amet</p>
  </div>
  <div id="tab3">
    <h3>Tab 3</h3>
    <p>Lorem ipsum dolor sit amet</p>
  </div>
</div>
<script>
  $(tabularize);
</script>

There’s a fair bit wrong with the JavaScript above, but it’s not necessarily bad code. It performs the tasks that are required of it. There are a couple of bugs, but as refactorers, we are not here to change the behaviour of the code. It passes the tests (in the introduction we discussed how every refactoring must be backed by tests) and our aim is to change the design, not the behaviour, and pass all the tests. I won’t show the tests, as they distract from the main purpose, but rest assured I did have them when making the changes I’m about to talk through and I was careful to keep them passing.

Reuse of Selectors

The key to refactoring is to make the smallest steps you possibly can. The first problem I’d like to tackle is the reuse of selectors. Let’s take a look once more at that JavaScript:

var active = location.hash;
if(active) {
  $(".tabs").children("div").hide();
  $(active).show();
  $(".active").removeClass("active");
  $(".tab-link").each(function() {
    if($(this).attr("href") === active) {
      $(this).parent().addClass("active");
    }
  });
}
$(".tabs").find(".tab-link").click(function() {
  $(".tabs").children("div").hide();
  $($(this).attr("href")).show();
  $(".active").removeClass("active");
  $(this).parent().addClass("active");
  return false;
});

Looking over the code again, you can see a few selectors that crop up again and again:

So let’s make the change. We’ll replace them, but do it one at a time, and run the tests after every change. This might seem excessive, but it really is key to this process.

Firstly, I’ll store a reference to $(".tabs"):

var tabsWrapper = $(".tabs");

And then I can replace every occurrence of $(".tabs") with tabsWrapper:

var tabsWrapper = $(".tabs");
var active = location.hash;
if(active) {
  tabsWrapper.children("div").hide();
  $(active).show();
  $(".active").removeClass("active");
  $(".tab-link").each(function() {
    if($(this).attr("href") === active) {
      $(this).parent().addClass("active");
    }
  });
}
tabsWrapper.find(".tab-link").click(function() {
  tabsWrapper.children("div").hide();
  $($(this).attr("href")).show();
  $(".active").removeClass("active");
  $(this).parent().addClass("active");
  return false;
});

Now that’s done and everything is passing, I can repeat the step with $(".tabs").children("div"):

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var active = location.hash;
if(active) {
  tabs.hide();
  $(active).show();
  $(".active").removeClass("active");
  $(".tab-link").each(function() {
    if($(this).attr("href") === active) {
      $(this).parent().addClass("active");
    }
  });
}
tabsWrapper.find(".tab-link").click(function() {
  tabs.hide();
  $($(this).attr("href")).show();
  $(".active").removeClass("active");
  $(this).parent().addClass("active");
  return false;
});

And finally, deal with $(".tab-link"):

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var active = location.hash;
if(active) {
  tabs.hide();
  $(active).show();
  $(".active").removeClass("active");
  tabLinks.each(function() {
    if($(this).attr("href") === active) {
      $(this).parent().addClass("active");
    }
  });
}
tabLinks.click(function() {
  tabs.hide();
  $($(this).attr("href")).show();
  $(".active").removeClass("active");
  $(this).parent().addClass("active");
  return false;
});

And now, even with just that small change in place, our code is improved. We have removed duplication, which in my opinion has given us two big improvements.

  1. If any of the selectors change, or our HTML changes, we only have to change those selectors in one place, not multiple.
  2. The code is clearer now. If you need to skim and quickly gain an understanding of what the code does, having well named variables in place of complex selectors helps massively.

More Duplication

There’s a bit more duplication going on though. If you look through the code, you’ll see the string "active" present far too many times. What if the class we gave the active tab had to change? Right now, we’re looking at five occurences of it. Wouldn’t it be better if that was just one?

Let’s introduce an activeClass variable to deal with this:

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var activeClass = "active";

var active = location.hash;
if(active) {
  tabs.hide();
  $(active).show();
  $("." + activeClass).removeClass(activeClass);
  tabLinks.each(function() {
    if($(this).attr("href") === active) {
      $(this).parent().addClass(activeClass);
    }
  });
}
tabLinks.click(function() {
  tabs.hide();
  $($(this).attr("href")).show();
  $("." + activeClass).removeClass(activeClass);
  $(this).parent().addClass(activeClass);
  return false;
});

That’s a good step to take, but in the midst of doing this you might have spotted some more duplication. Take a look at these two code blocks. They look pretty similar to me:

$("." + activeClass).removeClass(activeClass);
tabLinks.each(function() {
  if($(this).attr("href") === active) {
    $(this).parent().addClass(activeClass);
  }
});
$("." + activeClass).removeClass(activeClass);
$(this).parent().addClass(activeClass);

The first is slightly different, because it has to loop over the links to find the right element to work with, but both of these blocks are performing the same piece of work:

  1. Find the current element with the active class, and remove the active class.
  2. Take the new active link’s parent, and add the active class.

When we have more than one block of code doing the same thing we can abstract them out into a function. Let’s do that now:

var activateLink = function(elem) {
  $("." + activeClass).removeClass(activeClass);
  $(elem).addClass(activeClass);
};

The activateLink function takes an element and adds the active class to it once it’s first removed the active class from any other element that might have it. Now we have this function, we can use it in place of the code we looked at previously. We’ll do this change one at a time. Firstly, we can edit the code within the tabLinks.click handler:

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var activeClass = "active";
var activateLink = function(elem) {
  $("." + activeClass).removeClass(activeClass);
  $(elem).addClass(activeClass);
};

var active = location.hash;
if(active) {
  tabs.hide();
  $(active).show();
  $("." + activeClass).removeClass(activeClass);
  tabLinks.each(function() {
    if($(this).attr("href") === active) {
      $(this).parent().addClass(activeClass);
    }
  });
}
tabLinks.click(function() {
  tabs.hide();
  $($(this).attr("href")).show();
  activateLink($(this).parent());
  return false;
});

Now all we have to do is pass $(this).parent(), which is the element we want to gain the active class, into our activateLink function, and it does the rest. We can now swap our function in in place of the code in the if(active) {} block:

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var activeClass = "active";
var activateLink = function(elem) {
  $("." + activeClass).removeClass(activeClass);
  $(elem).addClass(activeClass);
};

var active = location.hash;
if(active) {
  tabs.hide();
  $(active).show();
  tabLinks.each(function() {
    if($(this).attr("href") === active) {
      activateLink($(this).parent());
    }
  });
}
tabLinks.click(function() {
  tabs.hide();
  $($(this).attr("href")).show();
  activateLink($(this).parent());
  return false;
});

Abstracting code out into functions is one of the easiest and most effective ways to make a block of code more maintainable. Functions are inherently self documenting, a well named function can tell you in one quick skim exactly what its function is, and what it does. As a new developer coming to the above block of code, I can understand the activateLink function’s effect without looking at the code within it. Being able to skim a block of code and gain an understanding without having to look at detailed implementation is a fantastic thing as a developer.

Step Back

We are far from done with these tabs, but I want you to notice how, even after just a couple of small changes, the code is now already in an improved position than when we picked it up. We have removed duplication of selectors and code, and made the code more self documenting and readable along the way. When refactoring, you should be in a position to stop the refactoring at any point, and move onto something else. If this was a real project, and suddenly I was called to an urgent bug in another project, I could commit this code now and still have improved it. You should never find yourself in such a mess that you can’t put the code down and come back later. This is vital to successful refactorings: keep them small and contained.

Higher level duplication

The code we’ve been working with has two blocks:

if(active) {
  // do tab things
};

tabLinks.click(function() {
  // do tab things
};

Although it doesn’t look like it at a glance, there’s a lot of duplication going on - both those blocks of code perform basically the same task. This can be sometimes hard to spot, as it can be hidden behind code that doesn’t immediately look the same.

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var activeClass = "active";
var activateLink = function(elem) {
  $("." + activeClass).removeClass(activeClass);
  $(elem).addClass(activeClass);
};

var active = location.hash;
if(active) {
  tabs.hide();
  $(active).show();
  tabLinks.each(function() {
    if($(this).attr("href") === active) {
      activateLink($(this).parent());
    }
  });
}
tabLinks.click(function() {
  tabs.hide();
  $($(this).attr("href")).show();
  activateLink($(this).parent());
  return false;
});

Now I’ve given you the primer, take another look over the code above. Notice how both blocks do the same thing:

The duplication is obscured somewhat because of the need for the tabLinks.each in the first block, but this doesn’t mean that we can’t abstract that duplication into a function.

Sticking with our mantra of making small steps, let’s first make a function that shows a specific tab. Sticking with the naming conventions, we’ll call it activateTab:

var activateTab = function(tabSelector) {
  tabs.hide();
  $(tabSelector).show();
};

This function takes a selector and shows it, after hiding all of the tabs first. We can now use this in both the if(active) block and in the event handler:

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var activeClass = "active";

var activateLink = function(elem) {
  $("." + activeClass).removeClass(activeClass);
  $(elem).addClass(activeClass);
};

var activateTab = function(tabSelector) {
  tabs.hide();
  $(tabSelector).show();
};

var active = location.hash;
if(active) {
  activateTab(active);
  tabLinks.each(function() {
    if($(this).attr("href") === active) {
      activateLink($(this).parent());
    }
  });
}
tabLinks.click(function() {
  activateTab($(this).attr("href"));
  activateLink($(this).parent());
  return false;
});

We’re really motoring now. Notice how the tabLinks event handler simply calls two other functions, and has very little in it. This is a good sign that we’re on the right tracks. Ben Orenstein, a developer who speaks a huge amount on refactoring, says that a function with one line inside is superior to a function with two lines, which in turn is superior to a function with three lines, and so on. Sandi Metz, a well known Ruby developer, defined a set of rules that help when building large projects, one of which is: “Methods can be no longer than five lines of code.”. Regardless of if you think five is a good or bad number for that rule, the point stands: short, small functions are nearly always preferable to large ones. Keep functions small and compose larger functions out of calling lots of little ones.

Before we continue, notice again how if we wanted to stop now, we could. It’s so important to not let yourself get down a huge rabbit hole of refactoring.

Merging the branches

Right now we have two branches in our code, the if(active) part and the event handler. I’d really like to try and get these into one, or at least make the branches as small as possible. Right now they still have duplication, they both call activateTab and activateLink. I’d really like to abstract that out into another function, but right now the obvious step isn’t that obvious. Sometimes you’ll reach a point like this when you’re coding, where you know what you need to do or want to do, but the step isn’t obvious. Often you’ll have to make another change, to make the new change easier. In their book Refactoring, Martin Fowler and Kent Beck put this nicely:

When you find you have to add a feature to a program, and the program’s code is not structured in a convenient way to add the feature, first refactor the program to make it easy to add the feature, then add the feature.

Although this quote talks about new features, what it effectively says is that if you need to make a change, but that change is proving tough to make, make other changes such that your original change is easy.

I realised after some thinking that the bit of code making this change difficult is this bit:

tabLinks.each(function() {
  if($(this).attr("href") === active) {
    activateLink($(this).parent());
  }
});

The fact that we have to loop over means we can’t just abstract out as easily. Instead of the each, we can instead use jQuery’s filter method:

var link = tabLinks.filter(function() { 
  return $(this).attr("href") === active;
}).parent();
activateLink(link);

We now filter over the tab links, looking for the one that matches the active hash, and get at the item that way instead. I can store the result of that to a variable, and then pass activateLink that element. Adding that change into our code gives us:

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var activeClass = "active";

var activateLink = function(elem) {
  $("." + activeClass).removeClass(activeClass);
  $(elem).addClass(activeClass);
};

var activateTab = function(tabSelector) {
  tabs.hide();
  $(tabSelector).show();
};

var active = location.hash;
if(active) {
  activateTab(active);
  var link = tabLinks.filter(function() { 
    return $(this).attr("href") === active;
  }).parent();
  activateLink(link);
}
tabLinks.click(function() {
  activateTab($(this).attr("href"));
  activateLink($(this).parent());
  return false;
});

Now, to see where the next change will come from, we need to examine a bit more closely the activateLink and activateTab function. Ideally, I’d like to encapsulate these into another function, but to do that we need to see what each function needs as a parameter. activateTab just takes a selector and uses that to hide and show what’s required, but activateLink actually takes in an element. However, if you look closely, you’ll note that there is a relationship between the activateTab parameter and the activateLink parameter. The activateLink is the parent of the element whose selector we pass into activateTab. So why don’t we pass the selector into activateLink, and let it find the exact element it needs?

var activateLink = function(selector) {
  $("." + activeClass).removeClass(activeClass);
  var elem = tabLinks.filter(function() { 
    return $(this).attr("href") === selector;
  }).parent();
  $(elem).addClass(activeClass);
};

With that change, suddenly we can rewrite the two branches of our code to look very similar indeed:

if(active) {
  activateTab(active);
  activateLink(active);
}
tabLinks.click(function() {
  activateTab($(this).attr("href"));
  activateLink($(this).attr("href"));
  return false;
});

Now we’ve achieved what we wanted; by performing some intermediate refactorings we now are in a position to deal with the duplication we have in the two branches.

One method to rule them all

We’re going to extract a new method, called transition, which will take the selector of the active tab in as its argument, and perform the tasks required. The transition method is very simple, it just hands off to activateTab and activateLink:

var transition = function(selector) {
  activateTab(selector);
  activateLink(selector);
};

And now we can use that method in our code. In reality I did make this change in two steps, inserting one usage at a time, but I think you get the picture, so I’ll show it here as one change. Our new code looks like so:

var tabsWrapper = $(".tabs");
var tabs = tabsWrapper.children("div");
var tabLinks = tabsWrapper.find(".tab-link");
var activeClass = "active";

var activateLink = function(elem) {
  $("." + activeClass).removeClass(activeClass);
  $(elem).addClass(activeClass);
};

var activateTab = function(tabSelector) {
  tabs.hide();
  $(tabSelector).show();
};

var transition = function(selector) {
  activateTab(selector);
  activateLink(selector);
};

var active = location.hash;
if(active) {
  transition(active);
}
tabLinks.click(function() {
  transition($(this).attr("href'));
  return false;
});

Reflection

There’s certainly more you could do with this code, and it’s also far from being the best implementation of tabs around, but I hope you agree with me that the end result is now much nicer than the one we had at the beginning, which is printed below for you to compare.

var active = location.hash;
if(active) {
  $(".tabs").children("div").hide();
  $(active).show();
  $(".active").removeClass("active");
  $(".tab-link").each(function() {
    if($(this).attr("href") === active) {
      $(this).parent().addClass("active");
    }
  });
}
$(".tabs").find(".tab-link").click(function() {
  $(".tabs").children("div").hide();
  $($(this).attr("href")).show();
  $(".active").removeClass("active");
  $(this).parent().addClass("active");
  return false;
});

By removing duplication and making things clearer, we’ve ended up with a solution that is more readable, self documenting and maintainable. Notice how at first glance the two different sections of the code looked very different, but after some initial refactorings we found them actually to be near identical. This is a common occurrence - often a refactor will open up new possibilities and ways of working, which is another reason to keep your refactorings small and your mind open - an improvement might not be immediately obvious at the beginning.

Tale 2: Cringey Carousels

In this chapter I want to talk about the value of some “quick wins” - very simple refactorings that, when applied to a codebase, will hugely improve the readability and maintainability of the project, at very little time and effort to yourself. You may find yourself too busy to fully refactor a huge method, but with these small steps you can make large gains quickly.

I’ve put together a simple enough jQuery Carousel which boasts the following feature set:

Hardily a fully featured carousel but it covers the basic functionality of most carousels I’ve seen, and those features provide enough scope for me to produce bad code that we can tidy. Take a deep breath and we’ll dive in.

First, there’s the HTML. Pretty simple and standard really:

<!DOCTYPE html>
<html>
<head>
  <title></title>
  <link rel="stylesheet" href="css/style.css">
  <script src="js/jquery.min.js"></script>
  <script src="js/app.js"></script>
</head>
<body>
  <div class="wrapper">
    <ul>
      <li><img src="http://placekitten.com/300/300" alt="Kitten" /></li>
      <li><img src="http://placekitten.com/300/300" alt="Kitten" /></li>
      <li><img src="http://placekitten.com/300/300" alt="Kitten" /></li>
      <li><img src="http://placekitten.com/300/300" alt="Kitten" /></li>
      <li><img src="http://placekitten.com/300/300" alt="Kitten" /></li>
    </ul>
    <div class="controls">
      <a href="#" class="left">Left</a>
      <a href="#" class="right">Right</a>
      <span></span>
    </div>
  </div>
</body>
</html>

There is also some CSS applied to make it look good, but we won’t be focusing on that.

Finally, the JavaScript:

$(function() {
  if(location.hash && location.hash.indexOf("image") > -1) {
    var number = parseInt(location.hash.charAt(location.hash.length -1));
    $("ul").animate({
      "margin-left": number * -300
    }, function() {
      currentImage = number;
      $(".controls span").text("Current: " + (currentImage + 1));
    });
  }
  var timeout = setTimeout(function() {
    $(".left").trigger("click");
  }, 10000);
  
  var currentImage = 0;
  $(".left").click(function() {
    clearTimeout(timeout);
    if(currentImage == $("li").length - 1) {
      $("ul").animate({
        "margin-left": 0
      }, function() {
        currentImage = 0;
        $(".controls span").text("Current: " + (currentImage + 1));
      });
    } else {
      $("ul").animate({
        "margin-left": "-=300px"
      }, function() {
        currentImage+=1;
        $(".controls span").text("Current: " + (currentImage + 1));
      });
    }
    timeout = setTimeout(function() {
      $(".left").trigger("click");
    }, 10000);
    return false;
  });
  
  $(".right").click(function() {
    clearTimeout(timeout);
    if(currentImage == 0) {
      $("ul").animate({
        "margin-left": ($("li").length - 1) * -300
      }, function() {
        currentImage = $("li").length - 1;
        $(".controls span").text("Current: " + (currentImage + 1));
      });
    } else {
      $("ul").animate({
        "margin-left": "+=300px"
      }, function() {
        currentImage-=1;
        $(".controls span").text("Current: " + (currentImage + 1));
      });
    }
    timeout = setTimeout(function() {
      $(".left").trigger("click");
    }, 10000);
    return false;
  });
});

I think JavaScript like this is JavaScript we’ve all written before. I know I have. Take a moment to study it and see if you can spot the problems with it. There’s some much larger problems that we won’t look at right now, but you should be able to spot a lot of “quick wins” that we can take care of right here and now. I highly recommend noting down on paper a list of all the problems you spot and comparing them to the one I came up with below, to see what you might miss and to see if you identify things I didn’t.

I’ve split my list of problems into two parts. Firstly, the big problems:

Remember, $(function() {}) is shorthand for $(document).ready(function() {}). Whenever you pass a function into $(), jQuery will presume you want it to run only when the document is ready.

And some things we can clean up immediately:

return false; the anti-pattern

Currently both the event handlers end with return false:

$(".left").click(function() {
  // things happen here
  return false;
});
$(".right").click(function() {
  // things happen here
  return false;
});

This topic was first talked about back in 2010 in Doug Neiner’s article “Stop (mis)using Return False and is still very much relevant today.

jQuery event handlers take one argument, the event object. This object contains information about the event that triggered the event handler to fire. This object not only contains properties, such as the co-ordinates of the mouse pointer when the event took place, but also methods, including preventDefault() and stopPropagation().

Normally when a developer writes return false, what they actually want is to pass the event object in and call event.preventDefault(), like so:

$(".right").click(function(event) {
  // things happen here
  event.preventDefault();
});

As I’m sure you’re aware, preventDefault() prevents the default action being taken. return false has the same effect, but it does something else too.

Let’s just head out on a quick tangent to fully discuss propagation. Take a look at the code sample below:

$(function() {
  $("div").on("click", function() {
    console.log("div got clicked");
  });
  
  $("div p").on("click", function(e) {
    console.log("p got clicked");
    e.stopPropagation();
  });
});

If you were to load that up in a browser and click on the p element within the div element, what would you see in the console? You would only see the second log statement, “p got clicked”. event.stopPropagation() stops the event from bubbling up the DOM tree. There are occasions when this is useful but the majority of the time, you don’t want this. What you might not realise though, is that return false; has the same effect. return false has the same effect as calling preventDefault() and stopPropagation(). This can lead to nasty side effects or bugs later which can be incredibly difficult and frustrating to deal with - trust me, I’ve been there.

So although it might take longer, it’s much better to be a bit more verbose here. If you only want the default action to be prevented, call preventDefault(). If you want propagation to be prevented, call stopPropagation(). If you want them both, don’t type return false;. Be explicit and type them both out:

event.stopPropagation();
event.preventDefault();

The question on where it makes sense to place calls to preventDefault() and stopPropagation() is largely down to you and your preference, but I like to put them at the top as the very first thing that happens in the method. That way it’s easily spotted if you or another developer is reading through the code to see what it does.

So for our first quick win, we can swap out return false with calls to event.preventDefault(). I’ve also put event.preventDefault() at the top, above the rest of the event handler code (which I’ve left out here just to save room).

$(".left").click(function(event) {
  event.preventDefault();
  // things happen here
});
$(".right").click(function(event) {
  event.preventDefault();
  // things happen here
});

on() and off()

In jQuery 1.7 there was a new API introduced for binding and unbinding event handlers in the form of on() and off() to supersede the old API which was (and still is) a myriad of methods like click(), hover(), mouseout() along with live(), bind() and so on. This point may be a bit contentious, but I think that on() and off() are absolutely vast improvements and the fact that the entire event binding API was able to be reduced to two methods is brilliant. Of course, the old methods are not going anywhere (imagine how much code would break!) but as a rule I now will never use click() or similar, and will always use on("click", function() {}). This isn’t going to bring you huge speed improvements or even gain any readability, but personally I do think it reads slightly nicer.

This one’s easy. Just swap out the calls to click with on:

$(".left").on("click", function(event) {
  event.preventDefault();
  // things happen here
});
$(".right").on("click", function(event) {
  event.preventDefault();
  // things happen here
});

Repeated Numbers

Our code has some numbers that crop up time and time again. The first is 300, which I’d refer to as a “Magic Number” and we’ll tackle separately. The second is 10000. This isn’t so much a magic number in my opinion as it’s not connected with the page as much. 300 refers to the width of an image, but it’s not immediately apparent looking at the code that that is the case. We should treat it differently, and we will. 10000 has no connections, it’s just simply the time we decided should be between each automatic progression of our carousel.

If I were in another language that has constants, I’d define this value as a constant at the top of the file. For example, if this was the language Ruby I could simply do:

CAROUSEL_TRANSITION_TIME = 10000

That constant would then be set to 10,000 and nothing could possibly change it later on.

Although JavaScript doesn’t have constants, a convention has formed that any variable in capital letters should be treated as such. So I’d actually type exactly what I typed above in the Ruby example, and place it towards the top of the JavaScript file:

$(function() {
  CAROUSEL_TRANSITION_TIME = 10000;

  if(location.hash && location.hash.indexOf("image") > -1) {
    // more code

And then I’d replace all occurrences of 10000 in the code with CAROUSEL_TRANSITION_TIME:

var timeout = setTimeout(function() {
  $(".left").trigger("click");
}, CAROUSEL_TRANSITION_TIME);

And similarly in the other two places it occurs.

By doing this we’ve definitely made this code more maintainable. Say the client turns up and wants the value changed. Now we’ve just one value to change, instead of three. Try to get into the habit of defining things as constants early. You can always remove the constant if you end up using it once, but it’s a good practice to get into if you find yourself referring to the same value over and over again.

I> Remember that constants should never be altered - in languages with actual support for constants, you are unable to edit them, you should treat your makeshift JavaScript constants equally.

Caching selectors

This is something that everybody should do but people don’t always do it (I know I’m guilty). jQuery makes it so easy to quickly query the DOM for something that it’s easy to just keep doing it and pay no attention to if you’ve done that previously or not. The common argument for this is largely performance. Whilst there is an obvious performance increase if you can avoid doing something multiple times, I would argue that today the main reason behind this should be the maintainability of your code. In the previous section we just swapped out occurrences of 10000 with a constant which took us from 3 changes down to 1 if that number should change.

Take a look at the carousel code and selectors that come up time and time again. There’s not too many unique selectors but they all occur multiple times:

The fix for this is simple, and I’m sure you already know what’s coming up. Cache those selectors!.

var ul = $("ul");
var controlText = $(".controls span");
var leftLink = $(".left");
var rightLink = $(".right");

Now go through and replace all occurrences of each selector with the relevant variable. What we’ve achieved here is a much easier job, should any of these selectors change. If you take anything away from this section, make it be this:

Anything that could change should only be referenced once in your code.

Make changes and alterations as frictionless as possible and everyone’s happy.

functions

We’re going to look more in depth at functions in a later chapter, where we’ll discuss their usage and some technical details in depth. This section can serve as a precursor to that.

A lot of people I talk to seem wary of using functions, like they come with some huge cost that people can’t afford. In other communities like the Ruby one (which I’ll reference purely because it’s the one I’m most familiar with) it’s very common to see posts heavily advocating using methods in Ruby. Ben Orenstein talks heavily about how he’s incredibly aggressive at abstracting code into new methods and having methods of extremely short length. I agree with Ben’s approach entirely we can certainly take some of what he says and apply it to our code.

As I said, we’ll go much more into this later, but for now let’s look at one quick example. This very line crops up four times:

controlText.text("Current: " + (currentImage + 1));

By abstracting this line into its own function we can gain a large amount:

  1. Maintainability. This code contains something that very much could change - the text that we put into the page to show the user what number they are on. Right now, that change would have to be made in four places.
  2. Readability. If you extract code into a function and name that function well, it’s then less code for a developer to have to read to understand the functionality. If, instead of the line above, we just had a call to updateControlText(), I can understand what that means instantly, and move on. I can understand what that means a lot quicker than I can understand what controlText.text("Current: " + (currentImage + 1)); means.
  3. Plus, it’s a first step towards properly structuring our code, something we’ll also look into in more detail later.

Let’s make the change. At the top of the code, just below all your selector variables, add the function:

var updateControlText = function() {
  controlText.text("Current: " + (currentImage + 1));
};

And then we can update the code to make use of it:

ul.animate({
  "margin-left": number * -300
}, function() {
  currentImage = number;
  updateControlText();
});

(And in the other three locations too).

Summary

With that change we’ve made five really quick and simple changes that have already improved our code hugely, with very little time spent making them. As you get used to spotting these problems and fixing them, you’ll find it becomes almost second nature to make them all the time, without thinking.

$(function() {
  var CAROUSEL_TRANSITION_TIME = 10000;
  var ul = $("ul");
  var controlText = $(".controls span");
  var leftLink = $(".left");
  var rightLink = $(".right");
  
  var updateControlText = function() {
    controlText.text("Current: " + (currentImage + 1));
  };
  
  if(location.hash && location.hash.indexOf("image") > -1) {
    var number = parseInt(location.hash.charAt(location.hash.length -1));
    ul.animate({
      "margin-left": number * -300
    }, function() {
      currentImage = number;
      updateControlText();
    });
  }
  var timeout = setTimeout(function() {
    leftLink.trigger("click");
  }, CAROUSEL_TRANSITION_TIME);
  
  var currentImage = 0;
  leftLink.on("click", function(event) {
    event.preventDefault();
    clearTimeout(timeout);
    if(currentImage == $("li").length - 1) {
      ul.animate({
        "margin-left": 0
      }, function() {
        currentImage = 0;
        updateControlText();
      });
    } else {
      ul.animate({
        "margin-left": "-=300px"
      }, function() {
        currentImage+=1;
        updateControlText();
      });
    }
    timeout = setTimeout(function() {
      leftLink.trigger("click");
    }, CAROUSEL_TRANSITION_TIME);
  });
  
  rightLink.on("click", function(event) {
    event.preventDefault();
    clearTimeout(timeout);
    if(currentImage == 0) {
      ul.animate({
        "margin-left": ($("li").length - 1) * -300
      }, function() {
        currentImage = $("li").length - 1;
        updateControlText();
      });
    } else {
      ul.animate({
        "margin-left": "+=300px"
      }, function() {
        currentImage-=1;
        updateControlText();
      });
    }
    timeout = setTimeout(function() {
      leftLink.trigger("click");
    }, CAROUSEL_TRANSITION_TIME);
  });
});

In the next chapter we will look more in depth at a smaller block of code I wrote recently that’s absolutely terrible, and see if we can’t improve it.

Tale 3: Async Abominations

The code for this example comes directly from a project I was working on recently. I was building a NodeJS powered API, and as part of that, needed a way of validating parameters that were passed as query strings in the API request. Every single request needs to be passed a token parameter, which is matched to the token of a user in the database, and some requests take a userId parameter, along with a token. In the case that both the token and userId are passed in, we validate that the token is the token for that specific user. In the case where we just take in a token, we validate that the token is valid, and exists in our database.

The API has many routes, so I wanted to abstract this into a function, which I called validateParamsExist. It is used like so:

validateParamsExist(['userId', 'token', 'foo'], req, res, function(requestIsValid) {
  if(requestIsValid) {
    // all parameters were passed
  } else {
    // something went wrong
  }
});

The function takes four arguments: - an array of parameters to ensure exist - the ExpressJS request object (don’t worry if you don’t know what it is, all you need to know is that it stores all the information about the request) - the ExpressJS response object (this is what we use to return data from the API) - a callback function, which is called once the validation is complete with a single argument, which is true if the validations passed, and false if it did not.

It’s also important to note that the ExpressJS request object (in the code, I refer to it as req), stores the parameters that we got with the URL in an object, which is stored in req.query. So if I made a request that looked like this: http://somesite.com/foo?name=jack&id=123, req.query would look like so:

req.query = {
  name: 'jack',
  id: '123'
};

The challenge here and why this refactor offers a different challenge to the others is because it’s asynchronous. It performs tasks asynchronously and hence it’s a bit more of an effort to pull pieces out. The basic idea for the implementation of this function is as follows:

  1. Loop over every parameter the user is expecting, and make sure it exists
  2. If the parameter is token, do some extra validation (as explained above). The exact extra validation to be done depends on if we also have a userId parameter or not.

The code

Rather than show you the starting code, which is badly written and tough to understand, I’m going to show you the finished code first. This is what my refactoring lead me to:

var matchTokenToUser = function(token, userId, errors, done) {
  // method for making sure a token matches a user
}

var ensureTokenExists = function(token, errors, done) {
  // method for ensuring a token exists
};

var noParamsPassed = function(req, res) {
  if(req.query) {
    return false;
  } else {
    res.json({ errors: ['no parameters supplied'] });
    return true;
  }
};

var checkTokenAndIds = function(req, errors, cb) {
  var token = req.query.token;
  var userId = req.query.userId;
  if(token) {
    if(userId) {
      matchTokenToUser(token, userId, errors, cb);
    } else {
      ensureTokenExists(token, errors, cb);
    }
  } else {
    cb();
  }
};

var validateParamsExist = function(params, req, res, cb) {
  if(noParamsPassed(req, res)) return cb(false);
  
  var errors = [];
  params.forEach(function(p) {
    if(!req.query[p]) errors.push('parameter ' + p + ' is required');
  });
  
  checkTokenAndIds(req, errors, function() {
    if(errors.length > 0) {
      res.json({ errors: errors });
      return cb(false);
    } else {
      return cb(true);
    }
  });
};

Have a read through the code, starting with the validateParamsExist method, and see if it makes sense. I have left some implementation details of functions out, because it plays no part in the refactoring (the method we’ll start with shortly also has ensureTokenExists and matchTokenToUser, too).

Stepping through the validateParamsExist method, here’s what it does:

  1. If no parameters at all were passed, call the callback and pass in false, because the validation failed.
  2. Go through each parameter we are expecting, and make sure that it actually exists. If it doesn’t, store an error in the errors array.
  3. Check to see if the token and id parameters were passed, and ensure that they are as expected.
  4. Once the checking of tokens and id are complete, check to see if the errors array has any items. If it does, use res.json to return those errors from the API, and call the callback with false, because the validation failed.
  5. Else, if we have no errors, call the callback with true, because the validation must have passed.

Back to the beginning

I showed you the finished version first because it’s readable and easy to digest what is going on, everything the first implementation isn’t. What we’ll do now is look at the pre-refactoring code and then step through the refactorings I made to get to the above code. Brace yourself, because here is the previous version:

var matchTokenToUser = function(token, userId, errors, done) {
  // implementation irrelevant
}

var ensureTokenExists = function(token, errors, done) {
  // implementation irrelevant
};

var validateParamsExist = function(params, req, res, cb) {
  if(!req.query) {
    res.json({ errors: ['no parameters supplied'] });
    return cb(false);
  } else {
    var errors = [];
    async.each(params, function(p, callback) {
      if(!req.query[p]) {
        errors.push('parameter ' + p + ' is required');
        callback();
      } else {
        if(p === 'token' && req.query.token) {
          if(params.indexOf('userId') > -1 && req.query.userId) {
            matchTokenToUser(req.query.token, req.query.userId, errors, callback);
          } else {
            ensureTokenExists(req.query.token, errors, callback);
          }
        } else {
          callback();
        }
      }
    }, function(err) {
      if(errors.length > 0) {
        res.json({ errors: errors });
        return cb(false);
      } else {
        return cb(true);
      }
    });
  };
}

Take a moment to read through that and see what’s going on. The good news is it can’t get any worse than this - things are only going to get better from here!

Abstracting functions

Before I even begin to look at the main block of code, that starts with the call to async.each, I like to immediately abstract out small blocks into functions. This is the kind of change that I might undo at a later point, but I find it really helps as a starting point to just split one large method up into a bunch of smaller functions if possible. The first bit we can do that for is the first part of our function, the if(!req.query)... part:

var noParamsPassed = function(req, res) {
  if(req.query) {
    return false;
  } else {
    res.json({ errors: ['no parameters supplied'] });
    return true;
  }
};

var validateParamsExist = function(params, req, res, cb) {
  if(noParamsPassed(req, res)) return cb(false);
  var errors = [];
  async.each(params, function(p, callback) {
    if(!req.query[p]) {
      errors.push('parameter ' + p + ' is required');
      callback();
    } else {
      if(p === 'token' && req.query.token) {
        if(params.indexOf('userId') > -1 && req.query.userId) {
          matchTokenToUser(req.query.token, req.query.userId, errors, callback);
        } else {
          ensureTokenExists(req.query.token, errors, callback);
        }
      } else {
        callback();
      }
    }
  }, function(err) {
    if(errors.length > 0) {
      res.json({ errors: errors });
      return cb(false);
    } else {
      return cb(true);
    }
  });
}

Doing this also means we can get rid of the if(!req.query) conditional that wrapped most of the body of the validateParamsExist method. I find exiting early is preferable to wrapping functions in large conditionals. These are what we call guard clauses - a conditional which checks something that is required for the function to be able to run. If it’s not there, it’s best to figure it out right away and ditch out early. Some argue that functions having multiple returns makes them unclear, but I’d much rather that then have large conditionals wrapping functions. Those are much more unclear, in my opinion.

The next abstraction is to pull out the code that checks for the existence of token and/or userId parameters.

var checkTokenAndIds = function(p, req, errors, callback) {
  if(p === 'token' && req.query.token) {
    if(params.indexOf('userId') > -1 && req.query.userId) {
      matchTokenToUser(req.query.token, req.query.userId, errors, callback);
    } else {
      ensureTokenExists(req.query.token, errors, callback);
    }
  } else {
    callback();
  }
};

// left out noParamsPassed fn so this code takes up less room

var validateParamsExist = function(params, req, res, cb) {
  if(noParamsPassed(req, res)) return cb(false);
  var errors = [];
  async.each(params, function(p, callback) {
    if(!req.query[p]) {
      errors.push('parameter ' + p + ' is required');
      callback();
    } else {
      checkTokenAndIds(p, req, errors, callback);
    }
  }, function(err) {
    if(errors.length > 0) {
      res.json({ errors: errors });
      return cb(false);
    } else {
      return cb(true);
    }
  });
}

The checkTokenAndIds is far from a perfect function, and later we will refactor it. However, once we get to the stage where we’ve pulled code out into functions, we can now digest validateParamsExist much easier.

When I started to look at this code, the first thing I spotted was how all of this code is wrapped within an async.each call. This is part of the fantastic async library. async.each offers a way to loop over an array and perform some asynchronous code for each, and then run some other code once all items have been looped over. The usage of it above though is far from sensible:

  1. The first part of the async.each, which checks for the existence of a parameter, is not asynchronous.
  2. The second part, which checks the validity of any token or userId params, only needs to run once, not every time.

It turns out that the only part of this code which does need to run in a loop is the parameter existence check, and that’s not asynchronous. We can run the code that checks for tokens and ids only once, so why is it in a loop?! (As an aside, when I wrote this code the first time I didn’t spot this. Looking back, I’m not sure what I was thinking!).

Making the first step of changes leaves us with code that looks like this:

var validateParamsExist = function(params, req, res, cb) {
  if(noParamsPassed(req, res)) return cb(false);
  var errors = [];
  params.forEach(function(p) {
    if(!req.query[p]) errors.push('parameter ' + p + ' is required');
  });

  // need to checkTokenAndIds next
  //...
}

The signature of the checkTokenAndIds function needs to change somewhat now. Previously we passed in the current parameter, but now we just need to give it all the parameters, our array of errors, and a function to call when it’s finished its checks. Here’s the final version of the checkTokenAndIds method I came up with:

var checkTokenAndIds = function(req, errors, cb) {
  var token = req.query.token;
  var userId = req.query.userId;
  if(token) {
    if(userId) {
      matchTokenToUser(token, userId, errors, cb);
    } else {
      ensureTokenExists(token, errors, cb);
    }
  } else {
    cb();
  }
};

Don’t worry about the implementation of matchTokenToUser or ensureTokenExists - they are both simple methods that just run some database queries, but they have no effect on this chapter. Notice how this method tells a story very effectively, and it’s easy to go down it line by line and see what’s happening, and follow the story. We can now go and add this method back into validateParamsExist:

var validateParamsExist = function(params, req, res, cb) {
  if(noParamsPassed(req, res)) return cb(false);
  var errors = [];
  params.forEach(function(p) {
    if(!req.query[p]) errors.push('parameter ' + p + ' is required');
  });

  checkTokenAndIds(req, errors, function() {
    if(errors.length > 0) {
      res.json({ errors: errors });
      return cb(false);
    } else {
      return cb(true);
    }
  });
}

And we’re done! This example was slightly different to previous examples - whilst the previous two chapters had code that was perfectly OK but had potential for some improvement, this code was just plain misleading, poorly constructed and badly written. Imagine if you had to make a change to the validation logic, and you had this block of code as your starting point. Do you think you could do it easily? I’m pretty sure I couldn’t.

var validateParamsExist = function(params, req, res, cb) {
  if(!req.query) {
    res.json({ errors: ['no parameters supplied'] });
    return cb(false);
  } else {
    var errors = [];
    async.each(params, function(p, callback) {
      if(!req.query[p]) {
        errors.push('parameter ' + p + ' is required');
        callback();
      } else {
        if(p === 'token' && req.query.token) {
          if(params.indexOf('userId') > -1 && req.query.userId) {
            matchTokenToUser(req.query.token, req.query.userId, errors, callback);
          } else {
            ensureTokenExists(req.query.token, errors, callback);
          }
        } else {
          callback();
        }
      }
    }, function(err) {
      if(errors.length > 0) {
        res.json({ errors: errors });
        return cb(false);
      } else {
        return cb(true);
      }
    });
  };
}

Compare that to the code we ended up with after sitting back, carefully going through the method to see exactly what it does and how it should work. The code is cleaner, its intention is obvious, there’s less nesting and indentation (a very basic, but often useful metric) and it reads nicer.

Any developer who tells you that they have never looked back over some code they’ve previously written and cringed is a liar. You’re never going to get it right first time, and the purpose of this book isn’t to try to make you to produce great code first time, but to spot where improvements can be made. Andy Appleton, a developer I’ve had the pleasure with working with, put this best when we were talking about this kind of thing one day. He said:

Defer concrete decisions as late as possible - you’ll never again know less about the problem than you do right now and the correct abstraction will become clearer over time.

As time goes on and you become more settled within the context of what you’re working on, refactorings, abstractions of classes and small tweaks should become easier to spot over time.

Tale 4: Parsing Problems

In this chapter we’re not going to look in as much detail at implementation specifics as we did in the first, but more on the overall structure of bits of code across a large system. Whilst the structure of individual blocks of code is important, arguably more so is the relationship of these components across a software system, and it’s this that I want to talk about in this chapter.

 Email Sending

As always, we need some code to work with, and this time we’re looking at some code that has to send emails. It takes in a CSV, which contains details of users, including their email address, and uses that data to get a list of emails, which it then takes and sends an email to.

var EmailSender = {
  init: function(csv) {
    this.csv = csv;
    return this;
  },
  parseEmailsFromCsv: function() {
    // implementation not important
    this.emails = [...]
  },
  sendEmail: function() {
    // sends email, implementation not important
    this.emails.forEach(...);
  }
};

On the surface of it, this object might seem pretty perfect to you. I’ve simplified it down slightly for the purposes of this book, but the details are mostly the same. It has a method to parse the emails out of the CSV, and then a method to take those emails and send an email to them.

Single Responsibility Principle

However, this code violates the Single Responsibility Principle (or SRP, as I’ll refer to it from now on). The Single Responsibility Principle states that one object should do one thing, and do it well. Our email sender right now has two responsibilities:

  1. Parse the user emails out of a CSV.
  2. Send an email to a list of email addresses

To me, that looks like two distinct responsibilities. Whilst this might not be an issue now, in the future of this large app you could imagine another part of the system wanting to send emails, or needing to parse data out of a CSV. Splitting this code up into two objects will benefit us now, but will probably benefit us more in the long run. This doesn’t mean that tomorrow at work you should aggressively split up a single object into many smaller ones, but spotting instances where an object is doing too much is a good skill to master. Often these type of refactorings are more about saving pain later down the line than immediate reward, but that doesn’t make them any less worthwhile. Long term, once you get used to the concept of SRP, you will find yourself keeping it in mind from the very moment you begin to build a new object.

An improvement

Just like any other improvement, we’re going to do it in small steps. Firstly, I can create a Parser object, that takes a CSV and can pull details out of it:

var Parser = {
  init: function(csv) {
    this.csv = csv;
    return this;
  },
  parseEmails: function() {
    ... // not important
    return emails;
  }
};

Now we have a Parser class that definitely fits the SRP - it knows how to parse data out of a CSV, and that’s the way it should be. We can now go ahead and update our EmailSender object:

var EmailSender = {
  init: function(csv) {
    this.csv = csv;
    return this;
  },
  parseEmailsFromCsv: function() {
    // implementation not important
    this.emails = Parser.init(this,csv).parseEmails()
  },
  sendEmail: function() {
    // sends email, implementation not important
    this.emails.forEach(...);
  }
};

Notice how I haven’t changed the methods available on EmailSender, but just the implementation so it uses the new Parser object.

Coupling

We are far from done here, but before continuing I want to talk about the concept of coupling. In a large system, coupling is the amount two components rely on each other to perform their functions. Wikipedia puts this nicely:

In software engineering, coupling or dependency is the degree to which each program module relies on each one of the other modules.

The best way to judge this is to take your two components and ask yourself this question: if one of these components were to change, how much would the other have to change? If the answer is “lots”, those components are tightly coupled. The overall aim is to have most of your components decoupled from each other, so that if one changes, the other doesn’t have to. Have you ever had to make a change to a system, and had to update code in multiple files at the same time? If so, that’s a good indication that all those files and components are perhaps too tightly coupled to each other.

Some coupling is necessary, because without any coupling, no component would ever be able to talk to another, but the less components know about each other, the better.

Looking at our code and where we’re up to, I’d argue that we have some coupling going on that isn’t required.

var Parser = {
  init: function(csv) {
    this.csv = csv;
    return this;
  },
  parseEmails: function() {
    ... // not important
    return emails;
  }
};

var EmailSender = {
  init: function(csv) {
    this.csv = csv;
    return this;
  },
  parseEmailsFromCsv: function() {
    // implementation not important
    this.emails = Parser.init(this,csv).parseEmails()
  },
  sendEmail: function() {
    // sends email, implementation not important
    this.emails.forEach(...);
  }
};

Here, EmailSender knows too much about how it gets emails. It knows that they come from the Parser, which uses a CSV as a data source. Why should the email sender know any of this? All we should give it is a set of emails, and it should do the rest. The next change we make is to decouple the components completely:

var Parser = {
  init: function(csv) {
    this.csv = csv;
    return this;
  },
  parseEmails: function() {
    ... // not important
    return emails;
  }
};

var EmailSender = {
  init: function(emails) {
    this.emails = emails;
    return this;
  },
  sendEmail: function() {
    // sends email, implementation not important
    this.emails.forEach(...);
  }
};

var emails = Parser.init(csv).parseEmails();
EmailSender.init(emails).sendEmail();

Notice how now the EmailSender has no knowledge of the Parser even existing. This now means if our data source were to change from being CSV to being from a database for example, we wouldn’t have to change any code. We could introduce a new object responsible for pulling our emails out of the database, and then still use EmailSender just like before.

Publish and Subscribe

Moving on to a different example, another method for keeping objects decoupled is the Publish and Subscribe Pattern, or pubsub for short.

In his (freely available online) book, “Learning JavaScript Design Patterns”, Addy Osmani summarises the goal of the pubsub pattern:

The general idea here is the promotion of loose coupling. Rather than single objects calling on the methods of other objects directly, they instead subscribe to a specific task or activity of another object and are notified when it occurs.

This approach is particularly suited to browser code, because JavaScript is largely event driven. Let’s look at an example where on the page we have a carousel and some tabs. When the user clicks on a tab to view some content, we want the carousel to stop running, so the user doesn’t miss any of the content on the carousel.

We might do it like so:

var carousel = {
  stop: function() {
    // stop animation
  }
  ...
}

var tabs = {
   tabClicked: function() {
     carousel.stop();
   }
   ...
}

This would work fine, but imagine if, along with a carousel, another element is added that needs to stop animating too? You’d have to add it to the tabClicked method:

var carousel = {
  stop: function() {
    // stop animation
  }
  ...
}

var tabs = {
   tabClicked: function() {
     carousel.stop();
     otherThing.stop();
   }
   ...
}

var otherThing = {
  stop: function() {
    // stop
  }
}

Although this is a slightly contrived example, I hope you can see that over time this is going to lead to some really messy code. It also means that we have to update our tabClicked method every time we add a new item, which is a little odd to me.

We can solve this using pubsub. In pubsub you have an object, typically called events (hold tight, I’ll link to some pubsub libraries you can use shortly). Objects can then use this pubsub object to publish events to, and subscribe to events that other objects publish. Here’s the same code in that previous code block, but using an events object.

var events = ... //pubsub object

var carousel = {
  init: function() {
    events.on('tabClicked', this.stop);
  },
  stop: function() {
    // stop animation
  }
  ...
}

var tabs = {
   tabClicked: function() {
     events.publish('tabClicked');
   }
   ...
}

var otherThing = {
  init: function() {
    events.on('tabClicked', this.stop);
  },
  stop: function() {
    // stop
  }
}

Notice now how the tabs object just published an event, and both the others can just run some code when an event is published. Crucially though, if a new item is added, the tabs code doesn’t have to change.

In terms of pubsub implementations, there are a few to choose from:

Conclusion

In this chapter we discussed the Single Responsibility Principle and the advantages it brings to the table. Getting into the habit of keeping your objects small and concerned with just one thing will make your code and overall applications much more maintainable and easier to work with as your application grows.

Tale 5: The Fat Controller

Test Driven Development

This chapter is slightly different from the rest. This isn’t a tale of some bad code turned good, but rather how I used test driven development (TTD) to create a function and my thoughts on how this effected the resulting code.

TTD often is thought of as foolish - writing a test that won’t pass because you’ve not written code seems unintuitive. The key thing that it does allow you to do is interact with the new code you’re about to write, and get a feel for what the API should look like. That’s a great benefit, and often I’ve rewritten tests many times as I figure out what the public API of the new object I’m building should look like. Another benefit though, is it lets you take small steps to successfully implementing a function. You can write your tests one at a time, and get them green one by one. Write one, make it green, and then write the next. At any point if you break a previous test, you’ll know right away. It’s that confidence that you’ve not inadvertently broken something that I enjoy most about a good test suite.

The Service

I’m working on an application where users have these objects, called “clusters”, that are groups of posts from another site. They can share these clusters with other users, who can subscribe to them. However, users can choose to keep their clusters private, and so only they can see them. I needed to build some logic for deciding if a user is allowed to subscribe to a cluster. There are various rules that govern this:

Else, if none of the four above conditions match, the user is allowed to subscribe.

Test Driven

This is the perfect example of a function that is very easy to test. Given specific input, it will either return true (the user can subscribe), or false (the user cannot subscribe). There are a few conditions we need to make sure we adhere to, so a set of unit tests is the perfect way to do this.

Let’s write the first test. Here I’m using the Jasmine testing library, but if you prefer another, feel free to use it instead. Jasmine is just my framework of choice for testing JS on the client. The actual implementation of this code was done within Angular, so in the tests there’s a fair amount of setup for that, which I’m ignoring here. The best way to test drive this is to write one spec, make it pass in the easiest way possible, and then write the next one.

Here’s my first test:

it('returns true if the user is not admin or owner and the cluster is public', function() {
  var res = UserCanSubscribeService.canSubscribe({
    id: 'abcd'
  }, {
    public: true,
    admins: [],
    subscribers: [],
    owner: 'cdef'
  });
  expect(res).toEqual(true);
});

Here I’m stating that the canSubscribe method takes two arguments: the first is the user object, which for testing purposes can just contain an ID. The second object is then the cluster that they may or may not be allowed to subscribe to. In this instance, the user is not the cluster owner, is not an admin, has not subscribed already and the cluster is public, so they are able to subscribe.

The simplest implementation to make this work?

UserCanSubscribeService = {};
UserCanSubscribeService.canSubscribe = function() {
  return true;
}

The test passes, but obviously that implementation needs a bit more work doing. However the point here, even if this might seem a little pointless, is to go one test at a time and make small steps. What this does is stops you over complicating your initial approach and potentially abstracting in the wrong place. A bad abstraction is worse than no abstraction, and by waiting until you have more code and context, you are more likely to pick the right abstraction.

Conclusion

The above code has been altered slightly for easier reading, but I did actually write this service in a real application. If you’re interested, you can find it on GitHub.