DEV Community

Cover image for A Clear, Concise & Comfy Code Review Checklist
Chris Bertrand
Chris Bertrand

Posted on • Updated on • Originally published at blog.designpuddle.com

A Clear, Concise & Comfy Code Review Checklist

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.

Related image

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++){
Enter fullscreen mode Exit fullscreen mode

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++){
Enter fullscreen mode Exit fullscreen mode

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){
Enter fullscreen mode Exit fullscreen mode

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;
Enter fullscreen mode Exit fullscreen mode

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!

Image result for delete gif

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

Top comments (11)

Collapse
 
chris_bertrand profile image
Chris Bertrand

A linter to check code duplication would be awesome! Do you know of any?

We all know, there are only two hard things in Computer Science: cache invalidation and naming things. That's why I check naming, even if it is entirely subjective.

Focusing on architectural/design issues at a code review is a little late for me, those should be ironed out at planning, however they should be raised at this stage if that did not occur. Thanks for the comment, I'll add in a section for it.

Collapse
 
joefuzz profile image
Joe DuVall

To add to Aleksei's list, we use Sonarqube to catch everything you mentioned for Java, Javascript, TypeScript, and C#. There are plenty of other plugins that expand Sonarqube's language support, too.

sonarqube.org/

Thread Thread
 
dan1ve profile image
Daniel Veihelmann • Edited

We use Teamscale for this purpose, as it is much faster and independent from the build.
teamscale.com

Thread Thread
 
chris_bertrand profile image
Chris Bertrand

Thanks Dan, Looks good. There are so many different tools! Picking one is always a conundrum.

Thread Thread
 
pravinuttarwar profile image
Pravin Uttarwar

We use codegrip.tech, its automated, supports more than 20+ languages and more importantly has great UI and integrations with slack. Price is very low compare to different tools present in market.

Thread Thread
 
chris_bertrand profile image
Chris Bertrand

Nice, thanks for the introduction. Will take a look.

Collapse
 
worc profile image
worc

i'd definitely double underline the caution in the Code Duplication item. i've seen dry turn pathological.

pushing or encouraging deduping too strongly leads you away from small, simple changes--hey i copied a line from one service into another--and into a cascading cluster of refactoring and renaming and rethinking--hey... so i spent the last week completely redesigning the system and now every basic equality check or destructuring statement is separated out into one of a dozen "helper" files and each concrete step of each procedure is abstracted away behind at least three levels of indirection, but hey at least we aren't repeating ourselves!

i'd rather get the functionality in first and then start a second, new task looking into that possible deduping. sometimes it makes sense, and sometimes you're just making contrived abstractions that will hurt to maintain later.

Collapse
 
chris_bertrand profile image
Chris Bertrand • Edited

Thanks for the comment, I completely agree and have amended the article. People love to refactor once a feature is complete. That basically negates the benefits, unless you know that this is going to be touched again shortly!

P.S - Underlines are not supported in Markdown, let also double underlines! But I've chucked it in a blockquote! 😁

Collapse
 
bgadrian profile image
Adrian B.G.

You can automate most of these stuff with a linter in your commit/PR to master pipeline, and treat warnings as build errors.

Collapse
 
deepanprabhu profile image
Deepan Prabhu Babu

Thank you, really nice article where you have brought together years of expertise.

 
chris_bertrand profile image
Chris Bertrand

Thanks, Pronto looks great, will give it a try.