Given that we want to reduce the number of conditional branches in our code, I wonder whether it is possible to catalogue all of the reasons they exist, so that we might then in turn list a few ways to eliminate each category.
So to that end I’ve listed out those origins of conditionals that I could think of. Is this a fool’s errand? Possibly, but let’s give it a try anyway. Please comment if you can spot any that I have missed, and let’s see if we can grow a “complete” list…
- Checking a value returned to me from code I own
This is the essence of the Null Check smell, and more generally Connascence of Meaning. For example, see this (intentionally bad) code from William C. Wake’s Refactoring Workbook:public class Report { public static void report(Writer out, List<Machine> machines, Robot robot) throws IOException { //... out.write("Robot"); if (robot.location() != null) out.write(" location=" + robot.location().name()); if (robot.bin() != null) out.write(" bin=" + robot.bin()); //... } } - Checking a value returned to me from code I do not own
For example, in Rails projects we often see code of this kind:if User.exists?(:user_id => current_user.id) # ... else # ... end
- Checking a parameter passed to me by code I own
Whenever I write a method that accepts a Boolean parameter, I open myself up to the Control Couple smell. Here’s an example from Piotr Solnica’s blog:def say(sentence, loud = false) if loud puts sentence.upcase else puts sentence end end - Checking a parameter passed to me by code I do not own
Here’s an example from the jQuery documentation:<script> var xTriggered = 0; $( "#target" ).keypress(function( event ) { if ( event.which == 13 ) { event.preventDefault(); } xTriggered++; var msg = "Handler for .keypress() called " + xTriggered + " time(s)."; $.print( msg, "html" ); $.print( event ); }); </script> - Checking my own state or attributes
Here’s an (intentionally bad) example from Martin Fowler’s Refactoring:class Employee { private int _type; static final int ENGINEER = 0; static final int SALESMAN = 1; static final int MANAGER = 2; Employee (int type) { _type = type; } int payAmount() { switch (_type) { case ENGINEER: return _monthlySalary; case SALESMAN: return _monthlySalary + _commission; case MANAGER: return _monthlySalary + _bonus; default: throw new RuntimeException("Incorrect Employee"); } } } - Checking a value I set previously in this method
For example:public String format(List<String> items) { StringBuffer s = new StringBuffer(); boolean first = true; for (String item : items) { if (first) first = false; else s.append(", "); s.append(item); } return s.toString(); } - Checking the type of an exception thrown by code I own
Here’s an example from the PHP language documentation:<?php function inverse($x) { if (!$x) { throw new Exception('Division by zero.'); } return 1/$x; } try { echo inverse(5) . "\n"; echo inverse(0) . "\n"; } catch (Exception $e) { echo 'Caught exception: ', $e->getMessage(), "\n"; } // Continue execution echo "Hello World\n"; ?> - Checking the type of an exception thrown by code I do not own
We often find code such as this in Rails controllers:def delete schedule_id = params[:scheduleId] begin Schedules.delete(schedule_id) rescue ActiveRecord::RecordNotFound render :json => "record not found" rescue ActiveRecord::ActiveRecordError # handle other ActiveRecord errors rescue # StandardError # handle most other errors rescue Exception # handle everything else end render :json => "ok" end
Is this list anything like complete? Can it ever be?
I wonder if a useful next step might be to write down some general strategies for removing each of these kinds of conditonal…?
Awesome work! Very interested in the outcome of this.
About numbers 3 and 4: I guess we can have different reasons for checking a parameter. Off the top of my head: checking for nil, checking types (as in is_a? or respond_to? in Ruby), and checking value/state for flow control. Since the strategy for getting rid of those can vary, I wonder if the list should be more specific about the reason the conditional exists.
@abernardes: My first draft had separate entries for *checking* parameters versus *validating* them. They do feel different, and I don’t know whether that means they will have different kinds of fix.
I was thinking to a strategy to remove the second kind of conditional, and it actually turned out that such conditional MAY have the reason to exist. One simple solution to prevent the if usage would be to use a method that throws an exception (User.find), so that the conditional could be removed. However we observe that:
1. This leads us to the 8th kind of “if” (e.g. we have to rescue the exception, but I agree that a framework or some other abstraction might hide this via some nice macros)
2. It may not be what we want to do.
There might be, in fact, a possibility that such “if” deserves to exist. The reason for that might be that we don’t want to deal with the user model at all, and perform two totally different tasks inside the same method:
def omg
if User.exists?(…)
User.destroy_all
else
User.create(…)
end
end
Now, I admit that the above example is pretty much meaningless, but I hope it explains the concept. We might use the solution provided by Piotr Solnica (in his control couple article) and create various classes that handle the if-branches separately even with the code above. However, for such a simple case, I wouldn’t go with that solution because, even though we may have introduced an abstraction (which MIGHT be useful), we have worsened the look of the code (and its understandability) decreasing the cohesion of the method. Moreover, new features will arrive and the abstraction could be wrong for such new features. Thus, I would wait a bit before transforming that if clauses into an abstraction.
So, to wrap up, I think that the scenarios are too many (perhaps infinite) to be written into a blog post. Or maybe I am wrong and I have just proposed other scenarios with this comment :)
What do you think?
I think the second kind is probably impossible to eliminate in general, without redesigning all the other code in the world that might call us. Our apps need to be able to ask about inputs they can’t control (“is that form field empty, or valid?” etc), so that kind is just fine. It’s the others that I think are very often redundant…
Pingback: Eliminate many conditionals with this one weird trick | silk and spinach
After reading your posts on this, I decided to check some code I had recently written. I found one main type of if statement. It looks like this:
if (_input.Length > 0)
{
result = ProcessExpression();
}
else
{
throw new MalformedExpressionException(ParserResources.EmptyInput);
}
So – if things are not going as we expect, then throw an exception.
What would be your suggestion for eliminating this kind of if statement?
@Clare — This feels like type 4, validating inputs. Assuming, of course, that an empty string is indeed invalid. In general I think it’s perfectly okay to reject invalid inputs, so we have to have conditionals around the edges of our hexagons.
So my only question would be: Is there a nice default behaviour we could apply to an empty input string? Such as having the parser simply return empty output?
PS The code in question (as you may have guessed) parses a string (it was actually a TDD exercise, so a little forced, but interesting nonetheless). The only other if statement in my code was this one:
if (IsPartOfDecimal(NextCharacter))
{
nextNumber = ExtractNextDecimal();
}
else if (IsOpeningBracket(NextCharacter))
{
nextNumber = CalculateExpressionWithinBrackets();
}
else
{
throw new MalformedExpressionException(string.Format(ParserResources.NumberOrOpeningBracketExpected, NextCharacter, _nextIndex));
}
You are in the middle of processing the input, character by character. You know the next character should either represent a number, or be the opening bracket of a sub-expression. It could be either one. Hence the if statement. Unless I’m missing something, it doesn’t fall into the list in your post. I suppose you could count it as living in the hexagonal boundary with the outside world, because we are checking an external input… but that doesn’t seem right either.
I pondered writing a char extension which contained some intelligence about what to do depending on whether the char was part of a decimal, or an opening bracket… but when I started trying to write it, I hit a lot of complexity very quickly, and was still struggling to avoid the if statement. Should I have persevered, do you think, or is there some other solution?
Having thought about it some more, I suppose the answer could be some sort of ExpressionToken class, which can be initialised with a string and a start index, and would extract the next decimal or bracket or whatever else, and would know how to act in particular circumstances.
So, you could replace this…
if (IsPartOfDecimal(NextCharacter))
{
nextNumber = ExtractNextDecimal();
}
else if (IsOpeningBracket(NextCharacter))
{
nextNumber = CalculateExpressionWithinBrackets();
}
…with something like this:
next Number = (new ExpressionToken(_input, _nextIndex)).EvaluateExpression();
@Clare — Again, you are definitely checking inputs, I think. So there has to be something somewhere that compared the next token with a list of valid next tokens, and again that’s fine.
I suppose you could have a table/hash/dictionary of possible next tokens mapping to their corresponding actions (or token objects), and throw a SyntaxError when the table lookup doesn’t find the token you have? But either way, there has to be some checking here, at the boundary, so conditionals are (usually) inevitable.
Pingback: On paperboys, newsagents and exceptions | silk and spinach
Hi Kevin.
Thanks for an interesting article!
You write “I wonder if a useful next step might be to write down some general strategies for removing each of these kinds of conditonal…?”
To answer your question: Yes, that would be cool. In the article “Eliminate many conditionals with this one weird trick | silk and spinach” you show how to eliminate “Checking a value returned to me from code I own”. It would be great if you could give examples on how to eliminate the other types of conditions.
Thanks in advance,
Anders
Hi Anders,
Yes, I must get around do doing that sometime. (Of course, anyone else reading this could do it too!)