2012.02.03

Keep your modules and functions small

This post is about a very simple thing that I’ve been doing since I started to code (by coincidence) and that I feel that increases a lot the readability and organization of my code. Some people may not agree with it but for me it makes total sense and was also documented by some experienced developers like Uncle Bob on his great book Clean Code. I tend to think that a single approach may not be the best one for everybody - since every person thinks on a different way - but I’m pretty confident that this advise will be good to a lot of people and that it will increase the overall quality of the code.

The rule is simple, split larger functions/classes into smaller specialized ones, period. It will not only increase the readability but it will also make the code more reusable since it will be easier to override the default behavior if needed (especially if extending a class or reusing a 3rd party lib). I will try to explain how and give a basic example.

Why?

Basically because smaller functions are easier to understand and smaller files are easier to read. Try to read a very large paragraph and you will see that the information is way harder to grasp.

Another huge benefit in keeping your classes/modules small is that there is a big chance that it will have a very high cohesion (all functions are strongly-related), low cohesion is a very bad code smell. By grouping functions by context it will be very easy to identify which file should be edited and where. If the bug is on the “drop down menu” you should check the DropDown class, if you need to change the way files are written to the system than you should update the fileWriter, etc…

I’ve said it before and I’m going to say it again:

Comments are usually a sign of poor/lazy/confusing implementations. (source)

By keeping your functions small and by using descriptive names you will reduce the amount of necessary comments a lot, a few lines of well written code can say more to a developer than many pages of documentation (your mileage may vary). Don’t describe with comments what can be described with code!!

How?

Uncle Bob says that the optimal size of a functions is 1 line, and no, he is not talking about 1 line of mangled code with lots of chained methods, he is talking about 1 line of clean code that does one thing and does it well. It may seem drastic, but I must admit that it is a very good goal to keep in mind (even if you don’t follow it by heart).

Let’s use a simple example to describe it, first some CSS:

.awsum {
    background-color: #0ff;
    position: absolute;
    width: 300px;
    height: 300px;
}

Now some JS code (using jQuery for brevity):

//this whole example is an anti-pattern
function AwsumWidget(parent) {
  this.root = $('<div class="awsum"/>');

  this.root
    .appendTo(parent)
    .fadeOut(0)
    .fadeIn(400)
    .click(function(evt) {
    $(this).animate({
      top: Math.random() * 300,
      left: Math.random() * 300
    }, 500, 'swing');
  });
}

AwsumWidget.prototype = {

  remove: function() {
    if (!this.root) return;
    var self = this;
    this.root.fadeOut(400, function() {
      self.root.remove();
      delete self.root;
    });
  }
};

Now let’s suppose you need to create a RadWidget, it should have all the basic functionality from the AwsumWidget but it should do a different animation on click, how would you do it? Since all the logic is inside the constructor you will need to duplicate the code just to replace the click handler, not scalable/flexible at all… First step is to refactor AwsumWidget so it’s easier to extend it and overwrite just what we need, lets also abstract some hard-coded values to make it easier to update - that’s also a very good practice, don’t leave “magic” numbers in the middle of the code, specially if you use it more than once, giving a name to the value will help describe what it does:

var MAX_POS = 300;
var TRANSITION_DURATION = 400;

function AwsumWidget(parent) {
  // constructors should not cause side-effects!
  // that way it is easier to delay execution if needed and test/mock it
  this._parent = parent;
}

AwsumWidget.prototype = {

  init: function() {
    this._create();
    this._animateIn();
    this._attachEvents();
  },

  _html: '<div class="awsum"/>',

  _create: function(parent) {
    this.root = $(this._html).appendTo(this._parent);
  },

  _attachEvents: function() {
    this.root.click(this._animateOnClick);
  },

  _animateOnClick: function(evt) {
    $(this).animate({
      top: Math.random() * MAX_POS,
      left: Math.random() * MAX_POS
    }, TRANSITION_DURATION, 'swing');
  },

  _animateIn: function() {
    this.root.fadeOut(0).fadeIn(400);
  },

  remove: function() {
    if (!this.root) return;
    this._animateOut();
  },

  _animateOut: function() {
    this.root.fadeOut(TRANSITION_DURATION, $.proxy(this._destroy, this));
  },

  _destroy: function() {
    this.root.remove();
    delete this.root;
  }
};

It’s clearly more code, but by breaking the code into smaller specialized functions you can override just the functions you need and the function names already describe what the code is doing so it’s easy to follow the program logic, the RadWidget can be easily implemented as:

var MIN_SIZE = 50;
var MAX_SIZE = 300;

function RadWidget(parent) {
  AwsumWidget.call(this, parent);
}

//let's just assume we are on an environment that supports Object.create
//https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/create
RadWidget.prototype = Object.create(AwsumWidget.prototype);

RadWidget.prototype.init = function() {
  AwsumWidget.prototype.init.call(this);
  this.root.addClass('rad');
};

RadWidget.prototype._animateOnClick = function(evt) {
  var size = Math.max(Math.round(Math.random() * MAX_SIZE), MIN_SIZE);
  $(this).animate({
    width: size,
    height: size
  }, TRANSITION_DURATION, 'swing');
};

We reused all the code from the AwsumWidget and only changed the parts that we needed, that is the true power of breaking everything into separate functions, modular systems are more flexible by nature, since each part is treated as an individual piece you can replace them at will as long as you follow the proper interface.

Sum Up

Break your code into smaller pieces, it may seem more work but the benefits on the long run outweigh the initial overhead. And if you are thinking that it will slow down your code since you have more functions calls remember about the performance dogma, unless you have a very large loop doing something trivial the extra function calls shouldn’t be the bottleneck. It is better to have a clean/organized structure and tune it if needed than to have a huge pile of mess and not being able to identify where the real bottleneck might be or react to new requirements on a timely fashion.

By breaking your code into smaller blocks it will also help you follow the Single Responsibility Principle which I consider one of the best advices about program structure.

Try it for a while, the more you do it the more natural it will fell and it’s really impressive how much it improves the code readability and reusability. It’s very hard to foresee all the needs and later on the road it might be harder to change it, so my advise is to refactor often and keep it as modular as possible. Make code easier for you and future developers to understand and maintain, it will pay-off sooner than you expect.

That’s it!

Further Reading


Comments

I object

While I agree that keeping your functions short is a very good goal, I take some serious exception to your example, and I think it's a fairly fundamental disagreement. It speaks also to your subsequent post regarding private methods.

Design for inheritance or else prohibit it

This is (nearly) the title of one of the items in Josh Bloch's book Effective Java, in which he describes problems with code which allows inheritance but does not handle it appropriately. While this book is about Java, many of its lessons, including this one, have a much broader implication. I think it's true in Javascript too (even though you can't actually prohibit inheritance in JS.)

The problem is not that your methods are short. The functions in your updated example are appropriate. But they are all public, and that's a real issue. The biggest offender is _create. This function creates one of the fundamental properties of your instance. Allowing a subclass to override this violates one of the invariants of your code. From now on, your code really needs to check that this.root exists and that it's a jQuery wrapper object. Otherwise who knows what will happen when _attachEvents, _animateIn, _animateOut, or worst, _destroy is called? To say that it's the responsibility of the person who overrides your constructor sounds good, but remember that when delete this.root removes something essential, it's your code that actually performed that, not the overridden code.

Or look at _animateIn. It's called during construction. One of Bloch's maxims in this chapter is that "Constructors must not invoke overridable methods". The reason is simple. This will be called before the subclass constructor has a chance to set things up.

Let's imagine that a subclass wants to create an additional element that will emphasize the process of showing your element. It might create that element during construction and override _animateIn to also animate the new element, removing the element when the animation is done. That sounds good, but since your constructor is (usually) run before that setup, the overridden function won't have its required element in place. The subclass can't readily fix this by running the base constructor later, for it might need the results of _create to process.

One more point. If you publish that API for your awesome widget, you are mostly stuck with your implementation, unless you want to face the wrath of users whose subclasses break when you decide to use some better techniques internally. With everything public, there is essentially no distinction between API and implementation.

Add proper extension points

The main point is that you should make sure your design and your documentation support the sort of overriding you would like to support. Don't offer easy paths to other sorts of overriding, though, as that puts your code at risk.

@Scott we will have to agree that we disagree, you might be doing a different kind of work than what I'm doing and come from a totally different background as well. Code guidelines aren't set in stone, it's not because the Effective Java book says you shouldn't call an overwritable method from the constructor that you should always avoid that. If you see the logic inside the base constructor is minimal (it just calls other methods, that's why I don't have the _create logic inside the constructor) and since all the methods are public you can just cal them by yourself, another benefit of breaking things into smaller functions and making all methods public.

Constructor functions in JS works totally different than Java constructors, you don't actually need to call/execute it to inherit from another object, if you skip the AwsumWidget.call(this); inside RadWidget you can do whatever you want (you can even do more stuff before calling it). I'd rather have a flexible structure than try to be academically correct and hit walls that sometimes are "impossible" to overcome... - My projects requirements changes a lot and I usually work on a tight deadline, being flexible and clean are on the top of my priority list. - I also like to think that whoever will use my code is a "good" developer and will know what they are doing, there is a "sign" saying that the pseudo-private methods might change in the future.

Cheers.

We will probably have to agree to disagree.

I don't think that the concept is right because Bloch says it. He just says it better than anyone else I've seen. Yes, Javascript is a very different language than Java, and there are plenty of things that can and should be done differently in them.

But I don't think that basic idea of hiding your implementation and exposing a minimal API is something that should differ. I believe that what's gained in initial flexibility is outweighed by the fact that your entire implementation becomes your public API and cannot be refactored without possibly breaking all sorts of child classes.

Thanks, though. These are interesting ideas to discuss. .

The things you write about, inspired from Uncle Bob, are contradicted by your examples.

First off, you show jQuery; with long, complex, faux-overloaded functions, it's the antithesis to what you write about.

Moving the code from the constructor to properties of it's associated prototype changes the public interface, as stated in comment #1. This is not making good use of scope.

Instead, I would rather see those methods as private members by creating a closure. Likewise I would rather see "_html" as a local variable for _create and _animateOnClick be private to _attachEvents.

I covered this adequately in the thread "Where Should Variables be Declared?"

The most relevant bits are:

Adding an identifier to an object (the global object) without considering how (or if) the role of the identifier is related to that object, or if the identifier should be globally accessible is a good way to write confusing code.

Identifiers that need to be shared across a group of functions should be stored in a closure (private static). This makes the most sense from a design perspective because it simplifies the object's public interface. The added bonuses is that doing this result in better performance in all but Firefox 3.5, and will result in smaller minified file size due to munged identifiers.

Not only does it make more sense from the perspective of the object's public interface, it's actually got slightly better performance than the way you've done it (though too slight to notice).

@Garrett the reason why I'm not using private vars on the example is because privacy reduces the flexibility. If _html is private the only way to change it is to overwrite all methods that uses it (in this case just the _create() method). I could potentially write a new widget that shares all the code but have just a different markup. There is always a tradeoff on each design decision, if you choose flexibility you might lose performance and/or stability, and I tend to favor flexibility (since my projects requirements changes a lot).

I agree that jQuery is full of bad practices. I used it for the widget exactly because most of the bad examples I see are written using it (chaining makes it really easy to write bad code). It also makes it easier for most people to follow the examples since it's very popular.

Thanks for the feedback.

[...] Keep your modules and functions small [...]

Leave a Comment

Please post only comments that will add value to the content of this page. Please read the about page to understand the objective of this blog and the way I think about it. Thanks.

Comments are parsed as Markdown and you can use basic HTML tags (a, blockquote, cite, code, del, em, strong) but some code may be striped if not escaped (specially PHP and HTML tags that aren't on the list). Line and paragraph breaks automatic. Code blocks should be indented with 4 spaces.