Introduction
Looking back at code I wrote ages ago often leaves me wondering “What idiot wrote this junk?” Looking back through articles I’ve written gives me an uncomfortably similar feeling. Today I’m thinking specifically about multiple return points, which I announced to be the root of much evil in this post. What this post reminds me about most, though, is the Do What Makes Sense principle, something that I’ve often claimed to subscribe to.
How do you plead?
I have to confess my guilt before I can continue. I’ve recently started writing some methods which have more than one return point (and I’m not just talking about exceptions here). But as a kind of mitigation I only use them in very small methods and only sometimes.
Actually that’s not true either.
There is a certain kind of method where multiple return points are acceptable, like these;
public Object myMethod() {
if(comeCondition) {
return someObject;
} else {
return someOtherObject;
}
}
Or this one;
public Object myOtherMethod() {
switch (something) {
case 1:
return something1;
case 2:
return something2;
case 3:
return something3;
case 4:
return something4;
case 5:
return something5;
}
}
But in these examples the methods are doing very specific things, most plainly, they return something. No other processing is done. I would still say returns dotted around throughout a method is still a Bad Thing. But that’s not what got me thinking. Whilst trying to justify this use of MRP to myself I started running through the reasons why this code is acceptable, when previously I had said it wasn’t. The switch method above reminds me of some refactoring work I did on my company’s product a while back, and it’s the fallout from that refactoring that I ended thinking about the most.
The Situation
Here’s some background. I work for a company which produces (and uses) it’s own software product, this product allows the buying and selling of Widgets. There are many different places that we can buy and sell Widgets from using our software, there is also a piece of third party software which we must tell who has purchased what Widgets, from whom and how many. The problem comes from the fact that each the objects which represent a user’s Widget-trading position is different for each Widget Gateway. Further, the position objects which we must send to the third party software is fixed. What’s even worse, is that each Widget Gateway can be further subdivided into more specific kinds of Widget Position Objects. Again, it’s even worse than that.
The AcmeWidgetPositionSubA class and the AcmeWidgetPositionSubB class both have the same attributes, but because one is …SubA and one is …SubB, when translating this to a ThirdPartyWidgetPosition the attributes with the same name in the …SubA/B classes will be mapped to different attributes in the ThirdPartyWidgetPosition class.
For example; AcmeWidgetPositionSubA.getSpecialCode() needs to be mapped to ThirdPartyWidgetPosition.getCode(). But AcmeWidgetPositionSubB.getSpecialCode() needs to be mapped to ThirdPartyWidgetPosition.getSystemCode().
This is the scope of the problem and the existing code to do this was lots of lines of if…then…elses to get the right things into the write place before the ThirdPartyWidgetPosition is sent to the third party software for processing. Further, there each subtype of Widget Gateway had it’s own chunk of if…then…else code of varying complexity to get the job done specific to itself. To refactor this code I got rid of all the if…then…else functionality and put all the logic into the type system. By introducing some extra classes that had only overloaded constructors I could remove every occurance of if…then…else, create a AcmeWidgetPositionSubX object, cast it to a ThirdPartyWidgetPosition and have everything in the right place, as if by magic.
So far so good, lots of complex nested branches get removed, all the unit tests pass, everything works, we all go home happy. Now fast forward a couple of months. Someone else is looking at the code and they are struggling to see what goes where and how the branched code has been removed in favour of a clever type hierarchy. Now, let me first explain that it’s my opinion that having the logic inside the type system is a better, actually less complex and cleaner implementation approach than if…then…else. It just feels “more OO”; even if I can’t quantify that phrase particularly well. But to a team member who is more used to seeing and reading if…then…else structures it is not cleaner and more obvious. It’s something that they need to wrestle with and really read in order to understand it.
So if I have slowed down the work of a team member, if I have caused them extra effort to do their own tasks, is my approach still the better one? Even if the code I wrote was the “more correct” implementation; was it the right code to write because of the affect it had on someone else?
Style Over Substance
A few more of my little foibles are using Boolean.TRUE rather than the primative true, and putting the literal I am comparing as the first operand of equals – even though I know that the protection this offers me is irrelevant given I spend most of my time writing Java1. I know, because I have been told, that some people find these things more difficult to read than their more usual alternatives. Personally I don’t understand why they are more confusing to read. But given that I can read the code quite happily either way and given that the compiler will change my code to whatever it feels like; should I be tailoring my professional coding style so as the most number of team members are happiest reading it?
Drawing The Line
The answer, as everything when discussing ‘programming best practise’, is; “it depends”. In the first example with the types vs if…then…else, that change should definitely be made. I can sit down with you and discuss why it is better, what benefits it has, show you how it is easier to extend etc. And as a consequence of that discussion, the team members who will be reading that code will hopefully start to learn from that example and we’ll be seeing fewer and fewer nested-branched code. Designs will become cleaner and gradually the overall quality of the code across the project will get better.
But what of more style based differences, or indeed, anything else which you can’t prove is a tangible benefit? If they cause pain for other team members or get in the way, then I suggest always defaulting to the lowest common denominator2. That might be structuring code for the least experienced developer – as long as such structuring does not compromise quality in any way. Or at least, structuring your code so that it is consistent with the rest of the code base. It means shoe-horning your coding style into the style used in the rest of the code base to make it easier to read for the whole team – this is especially true in large legacy codebases.
Doing these kinds of silly superficial things in difference to the rest of the code base is something that I am guilty of, and on reflection, something that I should be trying much harder to put a stop to. Make my code blend more seamlessly into the rest of the code is going to make reading it easier for everyone. But beware, when there is a real change that should be made, for a real reason with benefits that can be quantified I will still argue for that change – even if it’s out of the comfort zone of those around me. And I fully expect, nay demand, that those around make those same kinds of changes to pull me out of my comfort zone – because that’s the only way I’m going to become a better programmer.
There is a lot of talk in various technical blogs about how superstar programmers should be allowed to express themselves in any way, structure their code the way they think it should be done, and basically do whatever they want because (you know) they’re superstars. Most of us are probably not superstars and most of the teams we work in are probably not 100% made up of superstars, so I really don’t find those arguments compelling. What I do find compelling is trying to increase the average code quality and the skill levels of the average (or lowest) team member, and these things are not measured by where they put their curly brackets or whether they use Boolean.FALSE or false. These things are superficial and any non-superficial programmer should be able to get over them.
And let’s not forget; everyone of us is the average-est or the lowest skilled level programmer in the team at something. And even if your team is made up of Superstars, chances are they are not all going to be the same kind of Superstar. Think of your favourite classical music composer, rock performer, rap artist, solo violinist, jazz pianist and so on. Now imagine putting them all in the same band; left to their own devices I really don’t think any music they produce is going to be bearable, let alone enjoyable – and this assumes that they’ll manage to produce anything! Not unless they all agree to do certain things in the same or similar way and all pull in the same direction3. And I guarantee that each one of them will feel restricted in some way by that. But what they produce will be well worth the Superstar fees they’ll be charging!
Conclusion
It is true that a bit of sand inside an oyster will make a pearl, but a bit of sand inside your shoe is just going to make blisters on your feet. Removing code complexity and builder and more defined cleaner design is a worthy goal and one worth fighting for. But doing what is essentially meaningless things for no tangible benefit is not. As with anything where more than one individual is concerns the rule of give and take must be applied. Forcing your team members to adopt a distorted concept of Hungarian Notation is pointless. Similarly, when such a notation has been adopted and is prevelant in a large percentage of the code base, fighting that notation is probably a hiding to nothing.
Team cohesion is important. Every team will be working better if each member is considerate to the others. By all means bring up the uncomfortable truths and fight to make the design and code better. But if the benefits of what you’re fighting for are not worth the battles to win it, then it’s likely your efforts would be better spent putting for head down and servicing user requests and bug fixes.
1 – Unless I’m comparing booleans, of course.
2 – This is not meant to be a derogatory phrase.
2 – Singing from the same song-sheet even.
Posted by Tom 