GitFlow and code reviews

Presentation1Many companies that uses GIT as its code repository use git-flow as the branching model for its projects. The question, that one can come up with is where and when the code review should be taken? Should it be before:

git flow feature finish MYFEATURE

If yes, then the reviewer looks through some version of code, but still, the person responsible for closing the feature creates a a new merge commit after all which can change a lot. On the other hand, if the reviewer creates the merge commit, he/she may not know all the aspects needed for a successful merge. There are a few pages trying to answer this, but still I haven’t found anything satisfying. Please read further for my proposal of clean and proper code review process with git-flow.

The problem with git-flow is the fact that finishing a feature is an atomic action. In one action the author does plenty of things

  1. Author: pushes the commit to the remote
  2. Reviewer: reviews the code
  3. A: creates a merge commit (new commit created!)
  4. A: moves the develop cursor to the new commit
  5. A: deletes the feature branch
  6. A: pushes to the remote

As always, splitting one action into multiple and then proper grouping might help. Consider the following flow including the author and the reviewer.

  1. Author: creates a merge commit
  2. A: moves the feature/a to the merge commit
  3. A: pushes the merge commit to the remote
  4. Reviewer: reviews the merge commit
  5. R: if the review is successful, rebase the develop with fast-forward to the merge commit (some automation can be introduced).

According to this flow, the reviewer always reviews the commit which will land in the develop. Additionally, this makes the author of the feature branch aware of merge problems before he/she pushes out his/her work to the review. Effectively, a feature can be completed and merged and simply wait for acceptance which then, is a simple “go/no go” without considering merge conflicts later on.

This isn’t a git-flow anymore, but still, it is a solid flow to lower the context switching of the author. Ones he/she finishes, it’s a real end, not an entry for incoming merge.

AzureDirectory – code review

The project AzureDirectory provides an Azure implementation of the Lucene.NET abstraction – Directory. It targets in providing ability to store Lucene index in the Azure storage services. The code can be found in here AzureDirectory . The packages can be found on nuget here. There aren’t marked as prereleases.

The solutions consists of two projects:

  1. AzureDirectory
  2. TestApp

The first provides implementation of the Lucene abstractions. The are a few classes, only needed for the feature implementation (implementations of Lucene abstractions). Additionally some utils class are introduced.
The code is structured with regions, which I personally dislike. Names of regions like: CTORS, internal methods, DIRECTORY METHODS shows the way the code is molded, with no classes holding common wrapped in region functionality. The lengthy methods and ctors are another disadvantage of this code base.
The spacing, using directives, fields that may be readonly are messy. Something which may be cleared with a ReSharper Clean Code is left for a reader to deal with.
You can find in there usages of Lucene obsolete API (like IndexInput.Close in disposal), as well as informative comments like:

// sometimes we get access denied on the 2nd stream…but not always. I haven’t tracked it down yet
// but this covers our tail until I do

It’s good and informative for author but leaving the project in an immature state.

The second project is not a test project but a sample app using the lib. No tests at all.

Summing up, after consideration, I wouldn’t use this implementation for my production Azure app. The code is badly composed, with no tests and left with comments pointing at situations where authors are aware of the unsolved-yet problem.

Code reviews

Recently, I’ve been involved in a discussion about code reviews. It made me remember a code review board that I introduced in one of my previous projects. Beside that I had to clarify positive aspects of code reviews. Verified by experience the most important point of doing code reviews with a tool is total asynchrony of the process. The reviewer can easily go through the diff, marking lines, changes in a given time selected by himself (or scheduled via team rules). The second point would be the artifact left after this process. It’s not a discussion over a coffee or an email in a private inbox. Once it’s published in a tool, it’s visible part of the project, a manifestation of the change in the code. Isn’t it great?

The list of possible tools to use:

  • with it’s possibility to comment over pull requests and commits.
  • I used this tool, it’s simple and easy to go with. It’s free!
  • Crucible an Atlasian tool. I haven’t used it so far, but hope to do it in a while

Getting asynchronous, public, strongly connected with code discussion over the project. This will make your code thrive and the knowledge spread across the team. Trust me.

Reviewing code

Recently I’ve been introducing a code reviewing tool, called Review Board. The whole ‘review’ thing was inspired by a free copy of Best Kept Secrets of Peer Code Review . The aim was to empower my team with a tool, which will allow propagating best practices easily. The other, obvious target, was ensuring quality of a software being developed. In a few next posts, I’ll describe the whole process, as well, as a team accommodation.

The book
The paper version of the book was sent for free. I was astonished that one company can afford such a gift. I do understand that Smartbear released their revision tool called Code Collaborator, but… come on. A book for free? It’s nice of them. Speaking about this position, it’s a must read for anyone who wants to introduce reviewing in his/her team. The book covers some research done in a combat conditions which can be valuable for your manager, if you have problems with incorporating a new thing into your tech toolbox. The numbers show, that reviewing is much more valuable than writing tests. I know, it can harm some zealous TDDers, but you should take into consideration by every developer thinking seriously about his/her craft.

The tool
As I scanned the Code Collaborator pricing I knew, that my company won’t cover costs for the whole team. I searched for some replacement and found the Review Board. The application is written entirely in Python and the server part is destined to be installed on a Linux environment. We’ve managed to make it run in less than one day. Then, the time came to integrate it with VisualSVN hosted on a Windows 2003 server. The VisualSVN has a nice support for post-commit hooks. It passes two parameters to the hook body. Although they can be easily passed to your batch, exe, whatever with a simple property window, it took me a while to get all the needed data to match the post-review, as it needs for instance a revision range for which the diff will be generated. Finally, I’ve got it running.

The team
As always I tried to be the change. The reviewing after a small number of delays became a habit. Receiving emails helps with this, as each review request is notified with a short email with a request summary. The whole process made the whole team to write better commit comments as no-one wants to be asked (at least, I wouldn’t like) “what did you just commit?”. It seems, that beside measured factors, the code review empowers a team as a whole. The journey goes on!