The dreaded code review, they are a time consuming unnecessary annoyance that stop us from developing software, right !
WRONG, they are integral to a software quality process. Without a code review strategy any code that you write has the potential to destroy the project. A code review should be something that’s embraced by all developers as it allows you to get another persons perspective on the code.
Most developers work very closely with the code and start to feel very protective over it when people start to suggest changes or say that its just not right or doesn’t resolve the task / bug that it was written to resolve.
In software development you cannot afford to get protective over the code that you write because the code you write on a Monday may be code that someone else is changing on a Wednesday.
Code reviews can be performed in a number of ways, there is the totally independent review where the person who is reviewing the code simply takes the code changes that you have committed to a local source code repository (such as a local GIT repository) and comparing that against the previous check-in’s and to see if the code looks sensible and matches the bug / task brief. The feedback that they give will either be positive; i.e. “the code changes look sound, should be committed” or “sorry, this failed the review because of XYZ please correct this and resubmit for review”.
The independent reviews can be quite harsh as the person who is reviewing the code doesn’t know you, doesn’t care about any issues outside of the code. Getting responses like “what on earth were you thinking!” or “This is utter rubbish” can really get you down, but hopefully with comments like these your reviewer would leave some constructive comments detailing why they think that and maybe even giving an example of how they would have done it.
The review is a two way learning process; you may learn something during the review which will influence the code you write. On the other side you may get comments like “this is pure genius” or “cool code!” which may mean that you have managed to teach the reviewer something that they can take away and implement in their own code.
Another type of review that may occur is where a colleague sits next to you and reviews it as you explain the solution to the problem; they will want to see a suite of passing unit tests, code differences and explanations as to why you implemented it in that way. These reviews, in some respects can be more difficult to deal with as you will know the person. In this type of situation its difficult to tell a person that you know and have worked with for a while that the code is just no good, its also very difficult to hear if your the one being told.
I’ve had this happen to me, being told the code is no good is the beginning of a constructive discussion. My first response when told this was “Ok, so explain to me why and then suggest an alternative solution to the problem”
Assuming that the reviewer has a valid point and there solution is viable you can adjust your code accordingly. Remember that software development is a continuous learning process, know one knows everything and one way that you can extend your knowledge is through processes like this.
The following checklist is something that i think is a good basis for a code review.
- Does the code have comments on the classes / methods / properties and are spellings correct
- Do variables have bad names, do they use abbreviations
- Has code been duplicated, has a method been written elsewhere that does the same thing
- Are null values handled correctly
- Are input arguments being checked (for ranges, for nulls etc.. )
- Are calls to external assemblies correctly trapped with try.. catch blocks
- Does the logic make sense
- Have unit tests been created/modified to cover the code
- Do all unit tests for the solution pass
- Could the code be optimized
- Are there any bad design practices being used
- Does the code meet the coding guidelines of your organisation
- Is it possible for a method to return a null value, does the consuming code handle that
- Does the code contain any logging information, if so does it expose any potential security risk
These are a few examples of things to look for, they are pretty basic so everyone should be able to follow them quite easily.
Here are some things that i try and do when i write code to make the whole development process easier
- Use meaningful variable names, it doesn’t matter if they are long as long as they are not stupidly long and they explain what they are being used to hold
- Avoid using the term “Temp” when naming anything because it wont end up being anything temporary will it
- Avoid using the variable name retVal
- Avoid calling classes / methods “Helper” or “Manager” unless they really are dedicated to managing something
- Try not to create static variables / classes these are very difficult to unit test and are the firsts sign of a “code smell”
- Check arguments as the first step in a method, throw ArgumentException, ArgumentNullExceptions or ArgumentOutOfRange exceptions where appropriate
- Comment all your properties / methods / classes
- Try not to put comments in the body of the code, it looks messy. Your code should be self documenting
These are a few things that i do, it would take a long time to write all of my considerations down but this should be a good start for anyone looking to
In the end its up to you to make your code cleaner and understandable as it will make maintaining it at a later date easier and will make code reviews more meaningful.
I hope that this article has helped you in some way with your code reviews