Craftsmanship - Part 5
This series of posts was first published at the Fortnox developer blog during 2014. The articles where collected, refinished and put on display here after Jonas moved on from Fortnox in 2015.
The series consists of five blog posts:
Software craftsmanship - Part 1 - Covers the main points of Agile, the original version.
Software craftsmanship - Part 2 - Cover the rest of the Agile manifesto as well as talking about TDD and testing in general.
Software craftsmanship - Part 3 - Rundown of software craftsmanship.
Software craftsmanship - Part 4 - Spending some time on the SOLID principles.
Software craftsmanship - Part 5 - Refactoring, with actual code and stuff …
Intro
In this, last, instalment of my blog series about the fundamentals of building good software we will talk about how to keep your good code good, refactoring. Refactoring is to change the structure of the code without changing the computational result of the code. In practise it is a large set of transformations, both concrete and abstract, that you apply to your code regularly to keep it in shape.
But while this might sound mechanical and scientific the other part of refactoring is arty and requires a good feeling for what transformation to apply when. The transforms have conflicting constraints for when you should apply them and applying the wrong one will make your code worse instead of better.
But lets jump in and give it a go.
Refactorings
Generally there are two kinds of refactorings, local and global, for the lack of better descriptions. Basically a local refactoring is something that stays in the module and doesn’t affect the public call graph of an entity in the system. While a global refactoring does affect the call graph. So an example of a local refactoring might be renaming private variables or extracting new, private helper methods in a class. The global refactorings then is something like extract or inline class, major subsystem remodeling etc.
So while they are all refactorings there is a scale here like from that of a small hammer to a steam powered pile driver, and that thing requires some serious confidence to drive. But let’s first look at some local refactorings and why you would use them.
An example
One thing that is very true for source code is that you only write it once but read it many, many times. Sure, you might need to rewrite stuff or spend a day working on a method to get it right, but in the end you wrote that solution once and you will have to read the code from now until it gets dropped or refactored.
Since we spend a lot more time reading code than writing code the readability of code has a significant impact on productivity. Imagine that you could choose between halving the time it takes you to write code, that is typing it in, or reading it. What would impact your productivity most? I mean even while you work on your own code in a method you read that code a lot of times to keep the local context loaded in your brain.
So one way of improving the readability of the code, and approaching that halving of reading time, is to give your local expressions some names. Lets look at some code:
doStuff = function( event ){
if((event.type === 'keydown' && (event.which === 13 || event.which === 9) && !event.altKey ) && (event.type === Fortnox.clickEventType && (!$(event.target).hasClass('autocomplete-input') || event.target !== this.target) && (!$(event.target).hasClass('icon-chevron-down') || $(event.target).closest('.autocomplete-container').find('.autocomplete-input')[0] !== this.target)) && event.originalEvent && (event.type !== Fortnox.clickEventType || (event.type === Fortnox.clickEventType && $(event.target).closest('#autocomplete').length) || this.eventQueue.length)){
...
} else if( event.type === 'keydown' && event.which !== 13 || event.which !== 9 ){
return;
} else if( event.type === 'keydown' && event.which === 13 && event.altKey ){
return;
} else if( event.type === Fortnox.clickEventType ){
if( $(event.target).hasClass('autocomplete-input') && event.target === this.target ){
return;
} else if( $(event.target).hasClass('icon-chevron-down') && $(event.target).closest('.autocomplete-container').find('.autocomplete-input')[0] === this.target ){
return;
} else if( !(event.type === Fortnox.clickEventType && $(event.target).closest('#autocomplete').length) && !this.eventQueue.length ){
return;
}
} else if( !event.originalEvent ){
return;
}
};
Guard clause
This code is a right mess. There is no way that you can come in after even a week and read what this does, and yet this quality of code is common, especially when you are in the early days of your journey. The main structure of the code, the signature of the anti pattern, is that one if clause, in this case the first, checks all the requirements for the method and executes the business logic. The other parts of the if statement checks for different failure conditions and handle them, in this case they simply return, but they could do more specific handling of the different cases.
The way to refactor this is to look at the method like this: A method has a function, a purpose. That purpose should be clear when reading the method. The failure cases are necessary, but not integral to the business logic.
Given these points of consideration we can extract the failure cases into guard clauses, tests at the top of the method that handle the different failure cases:
doStuff = function( event ){
if( event.type === 'keydown' && event.which !== 13 || event.which !== 9 ){ return; }
if( event.type === 'keydown' && event.which === 13 && event.altKey ){ return; }
if( event.type === Fortnox.clickEventType && $(event.target).hasClass('autocomplete-input') && event.target === this.target ){ return; }
if( event.type === Fortnox.clickEventType && $(event.target).hasClass('icon-chevron-down') && $(event.target).closest('.autocomplete-container').find('.autocomplete-input')[0] === this.target ){ return; }
if( !event.originalEvent ){ return; }
if( event.type === Fortnox.clickEventType && !(event.type === Fortnox.clickEventType && $(event.target).closest('#autocomplete').length) && !this.eventQueue.length ){ return; }
...
};
The business logic, simply denoted with … above, has moved down to the bottom of the method and is the default action for the method. This underlines that it is that part of the code that the method is really about. It doesn’t need a lot of checks in front of it any more since the guard clauses will have exited the method before we get to the business logic if the context is a failure case. And while this code is not very pretty it at least have two clear benefits: It makes the business logic very distinct from the error handling and it removes a log of error sources by not having the checks twice, once in positive forn before executing the business logic and once in negative form for each failure case. What would happen in the first code listing if the method didn’t trigger any clause of the if-statement? How easy is it to add new constraints to a method like that and get the logic right twice?
I confess that it’s the second listing that is the actual code I copied from one of my projects, the first listing is an approximation of how this anti pattern usually looks and I’m sure I didn’t get the logic for the first clause right. Makes you wonder how often that happens in everyday work …
Introduce explaining variable
The new listing is an improvement. But I still could not go into that code and start changing it after a glance. It’s still too complex and the expressions inlined in the guard clauses give no explanation of why they are needed. So what we need is some more clarification.
We would like the code to spell out why it does something instead of just being a record of how to do something. The jQuery selections are a special pet peeve with me. They are used a lot without explaining why. It’s all good and well to see that you are selecting all the paragraphs with the whatever class inside of the main article to see if there is an even or uneven number of them, but why? What does that mean in the current context? Seeing what is done is next to no help when you come back later. The only choice is to laboriously read through the code and load it all in your head, slowly building a mental map of what it does.
So we could of cause use comments to convey context around the code we write. But first that’s just a lot of extra work, now we have to write good code and good comments. It’s bound to take up more time. And we have twice the amount of lines to maintain as well. Every time we update a line we will have to make sure that we didn’t break the comment for that line, and there are no automated tools to enforce that.
So I suggest we use a common refactoring instead, introduce explaining variable. What this refactoring does is to give expressions names by extracting them into variables. Now this all hinges on you giving the variables good names. Try to keep it simple, complete and short :)
Here is the same code as before with all non trivial expressions extracted into variables:
doStuff = function( event ){
var $target, keydown, enterOrTab, click, input, clickOnAutocomplete,
inputClosestToTarget, targetIsAutocompleteInput, targetIsCheveronDown,
keyPressedButNotEnterOrTab, altEnterPressed, machineGeneratedEvent,
clickOnCurrentlyFocusedAutocompleteCheveronDown, clickOutsideWithoutQueue,
clickOnCurrentlyFocusedAutocompleteInput, shouldExitEarly;
$target = $(event.target);
keydown = event.type === 'keydown';
enterOrTab = event.which === 13 || event.which === 9;
click = event.type === Fortnox.clickEventType;
clickOnAutocomplete = click && $target.closest('#autocomplete').length;
inputClosestToTarget = $target.closest('.autocomplete-container').find('.autocomplete-input')[0];
targetIsAutocompleteInput = $target.hasClass('autocomplete-input');
targetIsCheveronDown = $target.hasClass('icon-chevron-down')
keyPressedButNotEnterOrTab = keydown && !enterOrTab;
altEnterPressed = keydown && event.which === 13 && event.altKey;
clickOnCurrentlyFocusedAutocompleteInput = click && targetIsAutocompleteInput && event.target === this.target;
clickOnCurrentlyFocusedAutocompleteCheveronDown = click && targetIsCheveronDown && inputClosestToTarget === this.target
machineGeneratedEvent = !event.originalEvent;
clickOutsideWithoutQueue = click && !clickOnAutocomplete && !this.eventQueue.length;
shouldExitEarly = keyPressedButNotEnterOrTab || altEnterPressed ||
clickOnCurrentlyFocusedAutocompleteInput || clickOutsideWithoutQueue ||
clickOnCurrentlyFocusedAutocompleteCheveronDown || machineGeneratedEvent;
if( shouldExitEarly ){ return; }
...
};
Now, I bet that you feel a slight apprehension at this point. While each line in itself might be easier to read the whole is really not. We have exploded the number of lines and more lines means more context to load into your brain. In this case we have increased the amount of local context, by adding a lot of variable assignments, thus aggravating the problem.
But look at the line that represent what we had before: if( shouldExitEarly ){ return; } That seams simple enough right? If we are in this method to fix something with the early exit behaviour we know that it’s above this line and if not we know we need to keep reading. If only we could get rid of the visual clutter and clearly separate these two cases.
Extract method
Since this function is about something, except early exiting, we can keep that code where it is. So what we want is to get rid of the visual clutter of the early exit check…
How about his:
doStuff = function( event ){
if( shouldExitEarly( event )){ return; }
...
};
That is simple, clear and if we need to fiddle with the exit behaviour we know we need to look in the shouldExitEarly function. This refactoring pattern is, not surprisingly, called extract method. All we did was copy all the code we had before, except the final if-statement, into a new, private (if we are in an OO context) function and from it we return the result of the boolean assignment we had before:
shouldExitEarly = function( event ){
var $target, keydown, enterOrTab, click, input, clickOnAutocomplete,
inputClosestToTarget, targetIsAutocompleteInput, targetIsCheveronDown,
keyPressedButNotEnterOrTab, altEnterPressed, machineGeneratedEvent,
clickOnCurrentlyFocusedAutocompleteCheveronDown, clickOutsideWithoutQueue,
clickOnCurrentlyFocusedAutocompleteInput;
$target = $(event.target);
keydown = event.type === 'keydown';
enterOrTab = event.which === 13 || event.which === 9;
click = event.type === Fortnox.clickEventType;
clickOnAutocomplete = click && $target.closest('#autocomplete').length;
inputClosestToTarget = $target.closest('.autocomplete-container').find('.autocomplete-input')[0];
targetIsAutocompleteInput = $target.hasClass('autocomplete-input');
targetIsCheveronDown = $target.hasClass('icon-chevron-down')
keyPressedButNotEnterOrTab = keydown && !enterOrTab;
altEnterPressed = keydown && event.which === 13 && event.altKey;
clickOnCurrentlyFocusedAutocompleteInput = click && targetIsAutocompleteInput && event.target === this.target;
clickOnCurrentlyFocusedAutocompleteCheveronDown = click && targetIsCheveronDown && inputClosestToTarget === this.target
machineGeneratedEvent = !event.originalEvent;
clickOutsideWithoutQueue = click && !clickOnAutocomplete && !this.eventQueue.length;
return keyPressedButNotEnterOrTab || altEnterPressed ||
clickOnCurrentlyFocusedAutocompleteInput || clickOutsideWithoutQueue ||
clickOnCurrentlyFocusedAutocompleteCheveronDown || machineGeneratedEvent;
};
Method composition
But this is a bit messy as well. Lets extract all the parts of the main boolean expression into separate methods as well so we end up with a shouldExitEarly method like this:
...
shouldExitEarly = function( event ){
return keyPressedButNotEnterOrTab( event ) || altEnterPressed( event ) ||
clickOnCurrentlyFocusedAutocompleteInput( event ) || clickOutsideWithoutQueue( event ) ||
clickOnCurrentlyFocusedAutocompleteCheveronDown( event ) || machineGeneratedEvent( event );
};
...
This recursive application of extract method has left us with six new private methods and the shouldExitEarly has become a composed method. Method composition is another pattern that extracts small parts of logic into helper methods leaving only delegating calls in the parent method. You could do some additional setup in shouldExitEarly if you wanted. Such as the coarse splitting of the event object into the smaller parts that are shared between at least two of the child methods. This would give you isolation from the fact that you are relying on an event and make it easier to test the methods that actually contain the constraints without having to mock an entire event object.
So what we would be left with is roughly this:
keyPressedButNotEnterOrTab = function( keydown, enterOrTab ){
return keydown && !enterOrTab;
};
altEnterPressed = function( keydown, which, altKey ){
return keydown && event.which === 13 && event.altKey;
};
clickOnCurrentlyFocusedAutocompleteInput = function( click, targetIsAutocompleteInput, eventTarget, thisTarget ){
return click && targetIsAutocompleteInput && eventTarget === thisTarget;
};
clickOnCurrentlyFocusedAutocompleteCheveronDown = function( click, targetIsCheveronDown, inputClosestToTarget, thisTarget ){
return click && targetIsCheveronDown && inputClosestToTarget === thisTarget;
};
machineGeneratedEvent = function( originalEvent ){
return !originalEvent;
};
clickOutsideWithoutQueue = function( click, clickOnAutocomplete, eventQueueLength ){
return click && !clickOnAutocomplete && !eventQueueLength;
};
shouldExitEarly = function( event ){
var $target, keydown, enterOrTab, click, clickOnAutocomplete,
inputClosestToTarget, targetIsAutocompleteInput, targetIsCheveronDown;
$target = $(event.target);
keydown = event.type === 'keydown';
enterOrTab = event.which === 13 || event.which === 9;
click = event.type === Fortnox.clickEventType;
clickOnAutocomplete = click && $target.closest('#autocomplete').length;
inputClosestToTarget = $target.closest('.autocomplete-container').find('.autocomplete-input')[0];
targetIsAutocompleteInput = $target.hasClass('autocomplete-input');
targetIsCheveronDown = $target.hasClass('icon-chevron-down');
if( keyPressedButNotEnterOrTab( keydown, enterOrTab )){ return true; }
if( altEnterPressed( keydown, event.which, event.altKey )){ return true; }
if( clickOnCurrentlyFocusedAutocompleteInput( click, targetIsAutocompleteInput, event.target, this.target )){ return true; }
if( clickOnCurrentlyFocusedAutocompleteCheveronDown( click, targetIsCheveronDown, inputClosestToTarget, this.target )){ return true; }
if( machineGeneratedEvent( event.originalEvent )){ return true; }
if( clickOutsideWithoutQueue( click, clickOnAutocomplete, this.eventQueue.length )){ return true; }
return false;
};
doStuff = function( event ){
if( shouldExitEarly ){ return; }
...
};
Inline variable
We can also use the fact that expressions in the arguments to a method gets named by the argument name in the method declaration to transform context dependant meaning from one context to another. For example, using the code above, like this:
shouldExitEarly = function( event ){
var $target, keydown, click;
$target = $(event.target);
keydown = event.type === 'keydown';
click = event.type === Fortnox.clickEventType;
...
if( clickOutsideWithoutQueue(
click,
$target.hasClass('icon-chevron-down'),
$target.closest('.autocomplete-container').find('.autocomplete-input')[0],
this.target
)){ return true; }
...
};
So we have cut a lot of the local variable declarations and instead assign the results of expressions in one context to variables in another via the arguments in the methods. Taking our previously extracted variables and inlining them again is called inline variable and is the opposite of introduce explaining variable and is in essence what we are doing here.
This trick allows you to make liberal use of the introduce explaining variable pattern while keeping the number of local variables to a minimum by pairing the introduce explaining variable pattern with the extract method pattern. This also has the nice side effect of breaking up your big methods into a number of small ones, naturally grouping logic that is independent from its surroundings into methods.
We have mechanically applied a log of transformations to our example without regard for any special cases. Lets see if we can find any damage from our work so far:
keyPressedButNotEnterOrTab = function( keydown, enterOrTab ){
return keydown && !enterOrTab;
};
altEnterPressed = function( keydown, which, altKey ){
return keydown && event.which === 13 && event.altKey;
};
clickOnCurrentlyFocusedAutocompleteInput = function( click, targetIsAutocompleteInput, eventTarget, thisTarget ){
return click && targetIsAutocompleteInput && eventTarget === thisTarget;
};
clickOnCurrentlyFocusedAutocompleteCheveronDown = function( click, targetIsCheveronDown, inputClosestToTarget, thisTarget ){
return click && targetIsCheveronDown && inputClosestToTarget === thisTarget
};
machineGeneratedEvent = function( originalEvent ){
return !originalEvent;
};
clickOutsideWithoutQueue = function( click, clickOnAutocomplete, eventQueueLength ){
return click && !clickOnAutocomplete && !eventQueueLength;
};
shouldExitEarly = function( event ){
var $target, keydown, click;
$target = $(event.target);
keydown = event.type === 'keydown';
click = event.type === Fortnox.clickEventType;
if( keyPressedButNotEnterOrTab(
keydown,
event.which === 13 || event.which === 9
)){ return true; }
if( altEnterPressed(
keydown,
event.which,
event.altKey
)){ return true; }
if( clickOnCurrentlyFocusedAutocompleteInput(
click,
$target.hasClass('autocomplete-input'),
event.target,
this.target
)){ return true; }
if( clickOnCurrentlyFocusedAutocompleteCheveronDown(
click,
$target.hasClass('icon-chevron-down'),
$target.closest('.autocomplete-container').find('.autocomplete-input')[0],
this.target
)){ return true; }
if( clickOutsideWithoutQueue(
click,
click && $target.closest('#autocomplete').length,
this.eventQueue.length
)){ return true; }
if( machineGeneratedEvent(
event.originalEvent
)){ return true; }
return false;
};
Inline method
There is some things we can improve here still. The most obvious to me is that machineGeneratedEvent is needlessly a function, it should probably just be returned to a variable assignment, for readability, as it was before:
var $target, keydown, click, machineGeneratedEvent;
...
machineGeneratedEvent = !event.originalEvent;
if( machineGeneratedEvent ){ return true; }
...
That looks better and saves us an extra method declaration. This technique, the opposite of extract method, is called inline method and is often used as a later step after you have extracted methods, further refactored and modified them and them when you come back up you have a case like the one above where it’s obvious that a method call is unnecessary.
Substitute algorithm
We can also see that the signature, and indeed the implementation, of both clickOnCurrentlyFocusedAutocompleteInput and clickOnCurrentlyFocusedAutocompleteCheveronDown is the same. What we can do here is to use one of the methods and invoke it twice with different arguments. I choose to keep the clickOnCurrentlyFocusedAutocompleteInput method and call it twice.
...
if( clickOnCurrentlyFocusedAutocompleteInput(
click,
$target.hasClass('autocomplete-input'),
event.target,
this.target
)){ return true; }
if( clickOnCurrentlyFocusedAutocompleteInput(
click,
$target.hasClass('icon-chevron-down'),
$target.closest('.autocomplete-container').find('.autocomplete-input')[0],
this.target
)){ return true; }
...
Rename method
Now, while this works fine we have partially destroyed the information we are conveying by blindly reusing the same method for two related calls. To improve the information we have to adapt the names of the method and its arguments to match the new usage. This gives a better representation of what the new method does:
clickOnCurrentlyFocusedAutocompleteChild = function( click, targetIsAutocompleteChild, Target, thisTarget ){
return click && targetIsAutocompleteChild && Target === thisTarget;
};
Finishing up
First lets look at our code in its current state:
keyPressedButNotEnterOrTab = function( keydown, enterOrTab ){
return keydown && !enterOrTab;
};
altEnterPressed = function( keydown, which, altKey ){
return keydown && event.which === 13 && event.altKey;
};
clickOnCurrentlyFocusedAutocompleteChild = function( click, targetIsAutocompleteChild, Target, thisTarget ){
return click && targetIsAutocompleteChild && Target === thisTarget;
};
clickOutsideWithoutQueue = function( click, clickOnAutocomplete, eventQueueLength ){
return click && !clickOnAutocomplete && !eventQueueLength;
};
shouldExitEarly = function( event ){
var $target, keydown, click, machineGeneratedEvent;
$target = $(event.target);
keydown = event.type === 'keydown';
click = event.type === Fortnox.clickEventType;
if( keyPressedButNotEnterOrTab(
keydown,
event.which === 13 || event.which === 9
)){ return true; }
if( altEnterPressed(
keydown,
event.which,
event.altKey
)){ return true; }
if( clickOnCurrentlyFocusedAutocompleteChild(
click,
$target.hasClass('autocomplete-input'),
event.target,
this.target
)){ return true; }
if( clickOnCurrentlyFocusedAutocompleteChild(
click,
$target.hasClass('icon-chevron-down'),
$target.closest('.autocomplete-container').find('.autocomplete-input')[0],
this.target
)){ return true; }
if( clickOutsideWithoutQueue(
click,
click && $target.closest('#autocomplete').length,
this.eventQueue.length
)){ return true; }
machineGeneratedEvent = !event.originalEvent;
if( machineGeneratedEvent ){ return true; }
return false;
};
doStuff = function( event ){
if( shouldExitEarly() ){ return; }
...
};
I’m reasonably happy with what we got here. The original function is now extremely easy to grasp at a glance, at least the guard clause part. The shouldExitEarly function is also relatively easy to read.
Finding where you need to make a change is made easy by the named constraint methods, the context transfer via the method arguments makes it explicit what something in one context means in the other and adding a new constraint is as easy as adding a new constraint method.
All in all this is ok. And ok is good, often good enough.
I could go on and show the major refactorings as well, such as class manipulations and higher level architectural refactorings. But this is an introductory series and I’d like to keep it simple so I’ll stop here.
The art of refactoring
You have seen a lot of code here today. The reason I wanted to show you this is to give you some insight into how I reason around the problem of creating easy to read code. Refactoring is not an automatable task, even if you should always do it in small, deterministic steps.
If you take all the different refactorings and made them into recipes, provided that you could, the signature at the top, the part that identifies a scenario for refactoring, would be as precise as a shotgun blast at great distance. Different signatures would hit the same code, suggesting that you do several different refactorings at once. Every method would be marked for inlining and any piece of code for extraction.
The point is that there needs to be more information that just the code to be able to refactor it. Your knowledge of the domain, the rest of the system, the backlog, any future plans for the project, team size, team preferences in coding style, time constraints, language features and on and on …
The more extra information you can bring in to the process of writing code the better in general and especially when making refactoring choices. This is also a good reason to refactor your own code as soon as you have written it, you will likely never again have that level of insight into that part of the system.
It’s also a matter of experience and practice. Start by doing the smaller stuff, like introduce explaining variable or even renaming variables or methods occasionally. Stick to the obvious cases, sometimes that extract method will be punching you in the face every time you read that bit of code, and refactor when you are sure.
Soon you will get more confident and the cases when you are sure will increase. And don’t look on a failed refactoring as a failure. Trying and failing is called learning, remember that.
Wrapping up
It’s been a long series and I want to get on with some smaller, more focused, articles in the near future. I hope that you have enjoyed this series about some of the fundamentals when it comes to the thought work behind high quality, maintainable software.
There is certainly a lot more to read on this subject so if you are interested in these things keep an eye out for a blog post with some book recommendations. In the mean time you should check out SourceMaking.com. They have a collection of patterns, refactorings and code smells that describe a lot of best practises, problems and their solutions. It’s a fantastic read and free to boot. So get along reading that, watch the video above and keep an eye on the Fortnox developer channel on YouTube for more of that as well as this blog for more material from me.
Feel free to comment, tweet, email or contact me in any way if you like or dislike this series or find any errors in my code examples :) Be well and take care. Until next time, refactor mercilessly …
Image courtesy of Wendell at Flickr under a Creative Commons - Attribution, Non Commercial, No Derivatives license.