Good afternoon pop-pickers! It’s Christmas and that means we’re run out of ideas for interesting TV programs so we’re producing another Top List of Things Voted By You!
Todays program is all about the code reviews and the 10 most common comments. You can bear these in mind when writing your next chunk of code.
10. Embedded Literal Values
Straight in at number 10 we have literal values embedded in the code.
Embedding literal values in code is bad, but why is it bad? I can think of two main reasons – but I’m sure there are more.
- Potential to introduce multiple bugs
If you’re writing some kind of data access class, chances are you’ll refer to database column names. Embedding those names in your code means that if they change (or you make typo) you have to change them in multiple places.
If you have to fix some problem with those names you have to fix them in multple places leading to another potential bug if you forget to update one of them – or make more typos etc. - Values are meaningless
Consider the code sample;
int days = 5*52;
Now this is a noddy example and doesn’t hold up to close scrutiny, but nevermind. Firstly, “days” isn’t the most descriptive name. And what does 5 and 52 mean? Changing the snippet to;
int days = 5*numWeeksInYear;is better, but I now think there’s a bug in the system, ’cause it looks like it’s working out the number of days in a year, so that should be seven, not five, right?
First sub-point, I don’t know if it should be 5 or 7 because the name “days” doesn’t give me a clue, but ignoring that…
Second sub-point, using “workDaysInWeek” instead of the literal value ‘5′ tells me exactly what this line of code is trying to do. Something I didn’t know before.
In short, use named values not literals.
9. null Checking
Down 5 at number 9 we have lack of null checking
This one has dropped several points, it has been replaced by 5. Checked Exceptions, for reasons that I shall explain.
To practise defensive programming you should be checking that the values of things are what you expect them to be. That means that making sure a thing is not null before calling methods on it. This is going to at least reduce the number of potential exceptions that are going to get thrown and possibly translated into HTTP Error 500s.
The reason for this dropping a few places is that NullPointerException is at least a runtime exception which means we can use the framework to deal with it.
8. multiple return points
Up 1; we have methods with multiple return statements.
A point perhaps open to argument, but having multiple return statements in a single method make that method more difficult to read and understand. It a form of spaghetti code – and we got rid of goto for the same crime.
It can also make the code harder to debug. In an stack trace you won’t see which return statement was executed when a method ended which means that you have less of an idea what that method did and how it executed. And remember that a method might call other objects and do ’stuff’ to other ‘things’ you don’t know how much stuff happened because you don’t know what return was used.
It’s also dead-easy to write methods that only have one return statement and it doesn’t cost you anything in terms of performance. The following quote is applicable to most items in this list, it’s good here though.
Any fool can write code that a computer can understand. Good programmers write code that humans can understand.
Fowler, “Refactoring: Improving the Design of Existing Code”
7. Code headers
Holding stready at 7 we have missing, wrong or un-updated code headers.
Code headers are still the easiest, quickest and most readable way to identify how and why a change was made to the source. Particularly if you’re viewing the source in something other than Eclipse.
6. Readability
Another non-mover. Code that’s difficult to read.
This is a catch-all for some of the other items in this list. We’re all writing code that needs to be maintained and supported by all kinds of people. Which, by definition means; not just the original author.
So what does making code readable mean? It means checking each line, construct or method and seeing if it is simple. Is it obvious what it’s trying to do? Does it do it in the most simple way? Could someone else, who doesn’t have the same context and history with it as [the author], read it and immediately understand it without having to spend large swathes of time digesting it?
So to get around this make sure that your code is laid out nicely, that things line up and are in logical chunks. Teniary operators do execute faster than if-than-else statements but at the cost of readability. So if you’re going to use them make sure that they are simple to read and aren’t overloaded with lots of different conditions.
Increasingly, people seem to misinterpret complexity as sophistication, which is baffling – the incomprehensible should cause suspicion rather than admiration. Possibly this trend results from a mistaken belief that using a somewhat mysterious device confers an aura of power on the user.
Niklaus Wirth
Classes over 400 lines (roughly) are too long. Overly long classes are trying to do to much. Similarly, methods that are longer than the length of your screen are too long and trying to do to much. Lines that are longer than 80-100 characters are again too long. We’re all only human, after all, so split things up and make them easier to read and understand.
These simple rules make the code easier to read which translates to;
- Easier to understand
- Easier to review
- Easier to support
- Easier to debug
- Easier to enhance
5. (Checked) Exceptions
Climbing 3 places we have the use of checked exceptions.
If multiple return statement make the code a bit spaghetti like, then exceptions make the code look really spaghetti like. We don’t use goto because it makes the thread of execution jump around all over the place… just like exceptions do. How is it a good idea to have a method than can exit on any arbitary line?
Exceptions also require a lot of boilerplate code to use properly. throws makes the method signatures bloated, while try/catch statements artificially inflate the methods and interrupt the flow of solving the original business problem. Instead of reading and manipulating a DOM document, I have to write a bunch of code to handle SAX exceptions, premature ends of files, file not founds and all kinds of other stuff that just gets in the way of solving the problem. At least with runtime exceptions I can rely on the framework to handle them for me…
Exceptions are open to some serious abuse. They are not logic flow controllers. Exceptions are supposed to describe, er, exceptional things. And bad things at that. Yes your code will behave differently when an exception is thrown, but it should not rely on an exception being thrown before it will complete some business task.
Exceptions of any type are evil, but by contrast; runtime exceptions kick the puppies whereas checked exceptions tie them in sacks and drown them.
So please think of the puppies next time you want to use an exception. And to reply to the next person who tells me “Sun/Spring/Hibernate/Whizzy-Framework uses exceptions”…
Just because the standard provides a cliff in front of you, you are not necessarily required to jump off it.
Norman Diamond
4. Comments
Up 1, we have the old favourite; inapproriate code comments.
Good code is its own best documentation. As you’re about to add a comment, ask yourself, ‘How can I improve the code so that this comment isn’t needed?’ Improve the code and then document it to make it even clearer.
Steve McConnell – Code Complete
Why do we even need to talk about code comments? Developing (not Programming) software at it’s most basic is the practise of writing good comments along with your code. Again, it might make sense to you now, but it may not make sense to the next person. In fact it probably won’t even make sense to you in a few weeks time when you’ve moved onto the next problem.
Several times I have done a code review for someone and asked “why” they have done something. There are not enough numbers in the world to count the times the author(s) have responded with “Erm… Ahh… I think it was because… I can’t remember. Oh, yes I can! It was so that… no that’s not right.”
Again, it’s back to the unwritten non-functional requirement that your code must be maintainable and supportable by all the team. Comments help reach that goal.
3. Copy and Paste vs. Inheritence
Holding steady in third place is code that uses copy/paste code.
Every line of code any developer writes is potentially introducing a bug. So if we want to statistically reduce the number of potential bugs in the code base we should write less code.
A simple way to do this is to use inheritence. I shouldn’t have to explain what that means. But I shall explain it’s essence, it’s basically getting functionality for free from some other class. So if I have one class that is the parent of 10 other classes and shares it’s functionality with them, I only have to write that code once. Rather than write it once and then copy/paste it into the others.
The obvious thing here is that if we use cut/paste and there is a bug in the code, we have to first of all remember all the places we pasted the buggy code into and we have to remember to change them all. Of course, if our fix introduces a second and different bug we’ve got 10 additional changes to make.
That’s the alternative to just writing the code once, and then making the fix once. It also means that you only need one suite of unit tests for that functionality. Whereas, if you copy the code into more than one place you will fail your code review if you don’t have a unit test for every place that code has been copied to.
Inheritence makes using getters and setters within an object much more important. So using;
getMyAttribute();
rather than;
this.myAttribute;
Unfortunately that particular issue was at number 11 and therefore doesn’t feature in this list.
2. OO Design vs. Procedural Code
Down 1 we have the practise of writing procedural code in an Object Orientated environment.
Object Orientation is not the be-all and end-all of the programming world. It (probably) isn’t the greatest thing in the whole wide world. However, if you’ve chosen to be a Java and therefore OO shop and so you should be using OO principles in all our design and implementation.
That means modelling things as real objects with real data and real behaviours. It does not mean writing some big class that calls a bunch of procedural-based methods in a procedural-based way.
What else this means is that you should not be writing classes that consist soley of static methods. Here is a good article on why that’s bad (if you can ignore that fact it comes from the MSDN!) Inside Architecture: Are Helper Classes Evil?
Another good article about the lack of polymorphism of static-method-only-classes can be found here; Java forum thread
We can argue on the benefits and restrictions of OO verses other methods, and that’s fine. But at this point in time we have decided to be OO, so you should developing in that way.
1. Unit Tests
And our new Christmas Number 1 is unit test coverage.
For the purposes of this article I have extended the scope of “Unit Tests” to include all automatically executed tests, so that includes unit, integration and functionality tests.
How many times am I going to write about how important tests are? About how they save you time, how they make refactoring easier/quicker/safer? About how they help you understand if your code is working as intended? It seems at least once more…
Unit tests are important tests because they save you time, they make refactoring easier/quicker/safer, they help you understand that your code is working exactly as intended..
One more thing I will say about unit tests is this. If you’re going to do it then you should do it right. That means testing every possible situation, every possible permutation and every possible variable. There is to be none of this “Oh, I don’t need to test that since it’s an optional value.” It may be optional, but that implies that it’s going to have different values and you therefore need to make sure that your code behaves correctly for all possible permutations for all of those “optional values” or whatever the excuse may have been.
Justification
That’s about all we have time for this episode, so let’s wrap it up with one last word of justification.
I don’t think anyone can argue (with a straight face) that these things don’t make the code easier and cleaner to read. Yes, some of them might cost us a little bit of extra typing (e.g. using getters/setters rather than directly calling the attribute or using named values rather than literals), but that time is spent in the writing of the code something that we only do once. The saving comes from the reading, maintenance and bug fixing tasks we do – and we do plenty of those.
As Developers (not Programmers, there is a difference) one of our non-functional obligations is to make our code readable and maintainable to the next Developer, something might make complete sense to us but we all work in an environment where everyone has a different skill level so we need to make sure that our code is supportable by everyone and that means making it clear and readable.
It also means making it correct; something that the above list helps us do.
Posted by Tom 