Code reviews are necessary to ensure your application is consistent. We live in an age of copy-and-paste craftsmen, so making sure that the bits that do get copied are correct is imperative for this approach to keep on working!
There are many articles out there telling you why code reviews are important, why you should use them, and how they will improve X, Y & Z. Most of us will already be familiar with the concept, practising it regularly (GitHub Pull Requests I’m looking at you!). This article is a simple checklist / cheatsheet. I may even create a nice infographic for it if it’s of use!?!
Having to complete these most days, it’s nice to have a checklist to follow, so you don’t miss anything. (You usually miss something).
— This is not a How To guide! —
I’m not going to say how you should find something positive to say, or how you help improve your team through this process, this will focus on what points to review.
Alternatively this can be given to developers as a reference, pointing out that these things will be checked, so make sure you are happy with them before we start.
These things aren’t difficult to follow, find or explain.
Making our code reviews are consistent will keep the source code consistent, thus improving maintainability.
It’s always important to have a framework to follow, the most annoying things are inconsistencies between reviewers, or focusing on irrelevant aspects. Try creating one yourself, you might even be able to base it off the list below! Let’s get stuck in.
Magic Numbers and Hardcoded Values
for (i=0; i<26;i++){
26 sure, that makes sense! How about we give that Magic Number a variable, so we understand what we’re doing? Something more akin to.
int AlphabetCharacter = 26;
for (i=0; i<AlphabetCharacter ;i++){
But even this can potentially be bad, what happens if another character gets added? We should really try and make this dynamic, and then use .size() or .length() on this object.
Code Duplication
Some people will know this as D.R.Y, but in all honesty it’s just looking for copies of code doing the same thing elsewhere. These should be refactored into their own method, easy. 2 blocks doing similar things might be allowable, but 3 or more is a definitive red cross from me!
Null Checks aka Defensive Code
Defensive code isn’t particularly fun to right, or pretty, and when things work it seems like it’s not even needed. However all to often on new implementations, when there is no data and we start running these methods things break!
if(objectYouAreCurrentlyTryingToDoSomethingWith != null){
Before accessing variables within objects and collections make sure they are there! PLEASE!
Variable Names
Which do you prefer?
int x = 12; int GregorianCalendarPeriodCount = 12; int months = 12; int MONTHS = 12;
I suppose this goes hand in hand with magic numbers, if you need to create a variable for a value, then make sure you name your variables as simply as possible. If that variable is a constant or won’t be changed then use the Const keyword in applicable languages and the CAPITALISATION convention to let users aware of your decisions about them. There are lots of articles about this variable naming, but it’s really just remembering these basics.
Functions / Methods / Services
- Names
The name of a method is more important than we give it credit for, when a method changes so should its name. The most infuriating thing is a method called Save(), when in reality it actually parses, validates, speaks to some third party, retrieves other records and adds them to global variables and maybe, hopefully finally saves the record in question. Yes it’s doing a save, but that other stuff is important too, and should probably be communicated to the developer when they are calling it.
- Return Types
Make sure you are returning the right thing, trying to make it as generic as possible. Sometimes you don’t even need a return type. If you do use the Void return type then please be aware that Void should do something, not change something!
- Access
Private vs Public, this is a big topic, and once again there are lots of good articles out there on it. It’s mainly about securing your application, making sure that certain procedures cannot be called outside of a class. Some people argue to make everything private until you need to make it public, however I find that making something private on purpose means more, and specifically tells a fellow developer that this is not to be used outside of the class. It’s not usually too costly if missed, but keeping an eye of the access level of a method can stop issues further down the line, where internal logic now cannot be changed because others have used it for other purposes.
Unit Tests
Simple checks, do tests pass? Do they assess correctly? Can you glean what the test is doing from the test name? Assuming the logic is correct, and the test has passed, it’s a case of looking for gaps in the process that are NOT being currently tested.
I like to use the convention of…
[UnitOfWork_StateUnderTest_ExpectedBehavior]
We also currently use gherkin syntax within the method to show the steps. Gherkin is a Business Readable, Domain Specific Language created especially for behavior descriptions. In a nutshell you specify the 3 main points of a test, including what you expect to happen using the following keywords GIVEN, WHEN / AND , THEN.
These can then be used as acceptance criteria and even be turned into Selenium automated tests with a little additional work.
Cyclomatic / Code Complexity
Try and look at how the code is structured, make sure methods aren’t too long, don’t have too many branches, and that for and if statements could be simplified. You may or may not have read my post on Cyclomatic complexity, I go into the topic in much more detail, so if you haven’t then check it out here: http://blog.designpuddle.com/web-development/coding-concepts-cyclomatic-complexity/
Remove Unused/Commented Code
With the uptake in Git/TFS and other source code repositories it’s unnecessary to leave commented code when working in and around areas with them. I would actually state it’s your duty to remove it. If it was removed for a reason, the last thing we want is someone trying to reinstate it again!
Hard simple rule, if you see it, delete it!
DELETE! DELETE! DELETE!
And finally after all of the above we can focus all of our time and attention on the most important issue!
Tabs / Spaces and Bracket Placement
Like seriously, this should be the least of your concerns, but if you don’t have a company-wide linting/styling rules baked into your IDE then at least make the reviewer aware of company style guides and how you expect them to be adhered to. Over time less of these issues will crop up, but seriously get some styling rules created already!
Hope the checklist has helped, as always fill me in on bits I’ve missed, I’m sure there are some gems that I’ve left out.
And finally if you’ve found this useful please share this post using the buttons below, Tweeting is encouraged! 🙂