An essential part of any software development team is code reviewing and there are many elements that must be considered. Now each team/company may have different rules governing how the code reviews are actually conducted, but they all form the same basis of what you should be looking out for and doing in a code review.
Types of Code Reviews
There are 4 common ways of conducting a code review:
Over-the-shoulder: the developer of the code walks through, explains and even demo’s the code to another developer. The developer will make notes of the defects found, make these changes, have the code re-reviewed if necessary and then check it in to the source control system.
Email pass-around: the code is checked into the source control system an email is sent to the reviewer. The developer will then review the code and let the writer know of any defects via email.
Pair programming: two developers work side by side on code, and the code review is embedded within in the code itself.
Tool-assisted review: either commercial or in-house tools are used that collect metrics, file-gathering, tracking comments, automatic notifications etc.
I’ve worked using most of these ways to perform code reviews, with my least favourite being Email pass-around, as this can be a hassle and a manual process.
Code Review Tips
Let’s go through a list of things you should be looking out for when reviewing code. The way you conduct code reviews should have no effect on any of these principles:
All tests pass and the project build correctly – you shouldn’t have to spend time running any tests or building the code as your build tool should be doing that for you. I’ve used TFS Builds in the past which builds the solution and runs all tests of all types (whether it be UI, SpecFlow, MSpec, Jasmine etc. tests).
Every single piece of code has tests around it. I’m a big fan of BDD, so everything should be test-driven. Now this is an easy one to verify: if you delete a piece of new code, do all the tests still pass? If so, then the new code isn’t covered by tests. A new condition within in if statement, new method, alteration of a method, new property, you name it should all be covered by tests.
Test/break the code – the new code that’s been added, does it actually work? Can you test it within the UI? What about edge cases? You should try to break the code as end-users could.
Code formatting – your team/company should have formatting standards that you should conform to. This is very minor but goes a long way in keeping the code consistent throughout the file, projects and teams. E.g. which of the following is correct:
.my-class { color: red; } .my-class { color: red; }
Should the class name be my-class or myClass? You get the general idea.
Is the code clean? Look to see if any improvements can be made in the code, like repeated code that could be moved to a base class, public properties that should be private, methods and properties being named clearly, does all the code conform to the SOLID, DRY, KISS, WET etc. rules. Can you understand the code easily without spending a lot of time on it?
Debugging code/comments – debugging comments should never be checked in, it can make a real hassle when removing all this when you think you’re ready (whatever that means) and it’s not clean code. Commenting on the other hand will depend on whether you allow comments in your code; if you do, then you are doing it wrong in my opinion; if you don’t allow comments, then no comments should be checked in.
Don’t review too much code at a time – you should keep your code reviewing short and often; don’t sit there for hours reviewing line after line. The Cisco Code Review Study [http://support.smartbear.com/support/media/resources/cc/book/code-review-cisco-case-study.pdf] found after 10 months of monitoring you should review fewer than 200-400 lines of code. As the number of lines increase, the number of defects found gradually reduces. The following graph illustrates this:
Conclusion
Code reviewing is an absolute must within any software project, including open source projects. If done right, they will save you time and money on bugs found later down the line, keep the code clean and helps with team cohesion among many other benefits. You and your team will need to set up a good structure for the way you code review and what your priorities should be.
Comments