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.

The checklist could be be given to developers as a reference, pointing out that these things will be checked. If you decide to do this, let the developers know so they can make sure they are happy with the code before the review starts.

These things aren't difficult to follow, find or explain.

Edit: Most of these things could be handled by a static code analysis extension/linter/tool. These should be installed into your IDE of choice, and warnings can be treated as errors stopping commits being allowed. This would ultimately be integrated into your CI/CD pipelines running on each build/commit/deploy too; stopping any rogue commits getting in. There have have been numerous suggestions for tools which I'll add to the Appendix below.

Unfortunately this approach is not always adhered to, and certain languages/platforms are not well supported (Salesforce, I'm looking at you). Therefore a fallback, manual human approach will always be beneficial to make sure things aren't missed. Edit End

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!

This is not to say that every code block that is duplicated needs to be refactored, if you are going to write the same code, use what's already there. If you've already finished your development work, then refactoring these methods might not make sense. Refactoring is a cyclical process, so if you go back to this feature you can always do it then if needed! #DontBecomePathological

Null Checks aka Defensive Code

Defensive code isn't particularly fun to write, 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 / Performance Tests

Simple checks, do tests pass, are there any continuous integration tests which you can check? 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.

In certain high load/availability systems, load testing and performance testing are key in making sure additional code does not slow the system down. These should be automated if possible and benchmarks and previous results should be logged.

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 <a href="in much more detail, so if you haven't then check it out here: 

Architectural/ Design Decisions

These should really be ironed out during planning, however if a project spans a number of months then there could be an opportunity to tweak things, if the landscape has changed dramatically. Use your initiative and discuss if a rewrite would benefit maintainability for the future.

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! :)

Appendix

These are a few recommendations for code analysis / lint tools.

TsLint
EsLint
Prettier
Code Climate - Ruby Tools
SonarQube
SonarLint
Team Scale
Credo - Elixir