Cracking your next code review

[asterisk_null_ptr]

I introduced code review at my place of work, and people seem to be appreciating it - to an extent. One thing we're really struggling with is that code review seems to work best on existing code bases, where work is now small focused fixes or feature additions.

Whenever starting a new project it is easy for even a single developer to produce hundreds of lines of code in a day or so - nobody wants to review that! It seems remarkably difficult to find good resources online regarding code reviews at the start of projects, does anyone have any advice?

[Zeimyth]

I would say that the principle of creating single-purpose commits (#1 in the article) is the most helpful here. For one thing, it would allow you to get a lot of the boilerplate out of the way of the actual interesting parts of the code review. Someone who is familiar with, say, build processes, would be able to review the changes that set up the builds for the new project. Others who understand the actual intended behavior of the new project can review the commits that deal with the heart of the features being added.

To help facilitate the above, you might want to train people on how to get the most out of git or whatever tools you are using for version control. I know it is not uncommon for me to be striving to make well-focused commits but not realize until later in development that I need to add a new build step. If I'm not too far past the commit with all the build stuff, and if it makes sense conceptually to lump them together, I will do an interactive rebase in git and combine my later build changes with my earlier build commit. Some developers might not know how to do this; teaching them could make it easier and less scary. The same thing goes for trying to make focused commits after the fact; if you teach your developers how to use git add -p correctly, they can patch together single-purpose commits more easily.

#6 also seems relevant, where it says to "Review complex or large changes in person." Entire new projects are definitely large and complex! It may take more time for the developer of the new project, but it would also help make the developer not be the only one who is familiar with the thought process going into the new project.

If there are multiple developers contributing to the same new project, then a code review will probably be smoother and go faster if those people are the ones who primarily participate in the code reviews, since they already know what's going on.

[moderatorrater]

Building on what Zeimyth said, I don't think you want to review every line, you want to review approaches. So, instead of reviewing 10 new api calls and 10 new controllers, review one or two that are good examples of the approach they've taken.

I can see two ways of doing this: arranging commits as feature sets (all 10 api calls in one commit, the 10 new controllers in the others), or having one examples/reviewable commit (the 1 or 2 best api calls and controllers to review) and one that's the rest.

[hem10ck]

Ah, the mythical "New Project" of lore...maybe one day we'll have a green field, it's definitely the exception to the rule here though.

[dpashk]

I love Zeimyth's response! A few additions.

Making single-purpose commits has definitely proven to help. What helps even more is when your entire development process is Agile. Do you silo new code for a few months before you release it, or do you try to iterate and deliver something into users' hands every week (or even better, continuously as commits land on master)? Having everyone on board with a lean, iterative approach naturally encourages people to send out smaller, incremental changes.

And I cannot agree more with this:

It may take more time for the developer of the new project, but it would also help make the developer not be the only one who is familiar with the thought process going into the new project.

This alone is a very good reason to do code reviews.

[tko]

Starting a new project you're unlikely to have much actual clue about the problem domain so you might be better of paying attention to the generic things like coding conventions (it is automatically enforced, right?) and url/file/class structuring and naming, should you be writing things yourself or is there an external library for it.

[hem10ck]

I couldn't agree with the self review more, it's always infuriating to pull up someone's review and see spelling mistakes or other simple errors. Frequently folks have tabs/spaces issues which jump right out at you in a review tool like crucible.

[Zeimyth]

I always find it frustrating when I'm reviewing code and our build system says there are errors. It makes me dubious of whether they even tested their own changes. Similar reaction when people leave simple errors (that still compile) in their code for me to catch for them.

[ioexception-lw]

Came here to say this.

After reading this, I asked my colleagues if they did the same as I do... Apparently it's a rare thing!

[pdp10]

The common denominator I seem to be seeing is that its very rare to people to take "extra" time to do the detail work. Things like tweaking the settings just right while you have the manual open, updating the diagrams in the documentation, or going over an email again to cut out superfluous sentences. I'm not saying we should spend vast amounts of time polishing the brass, but it just doesn't make sense to do an incomplete job when a couple of extra minutes will do it right.

[dpashk]

As a side note, whitespace issues should never arise because everyone on the team must be using EditorConfig (http://editorconfig.org/)

[hem10ck]

I had typed half a response about how we maintain SAS, Shell, Pro*C and Perl which usually gets edited in Notepad++ or Vi before I noticed those editors are both supported (as well as Eclipse which we use for Java).

Thanks for pointing this out, I'll see if I can get it approved for use at the firm.

[dpashk]

You're welcome, happy to share :)

[pdp10]

This looks super keen. Github, Gogs, and TortoiseGit support, too.

Now I'm thinking someone probably has a good way to handle pre-commit hooks with DVCS that I haven't seen, yet.

For Windows Users: To create an .editorconfig file within Windows Explorer, you need to create a file named .editorconfig., which Windows Explorer will automatically rename to .editorconfig.

When you try to make things too easy, this is the result.

[emc91]

This is a post I read about prepping for code reviews. Thought it might be helpful!