Code Review

Git Workflow Presentation – Intel Buzz Workshop

I travelled to Stockholm this week to deliver a presentation about our Git workflow on Sonic Dash. Originally I was asked to do a talk about the process of converting Sonic Dash to intel based chip sets, but since that would take all of 3 minutes, I worked it into a talk about how we work in general.

The link is possibly tenuous, but at least they had some baring on each other!

Anyway here’s the presentation

Setting up Git on ReviewBoard

I’ve recently made the switch to Git and started using it on a couple of project at work.  One of the important things I needed was to get automatic generation of reviews working on ReviewBoard for our Git server and I was in luck because it’s pretty simply to do.

I’m posting this here both for a reminder to me should I need to do it again and in case anyone trips over on a couple of steps that are not highlighted as clearly in the documentation.

 

The Git Server

ReviewBoard works best if you have a primary Git server (we’re using Gitolite at the moment) which most people clone from and push their changes to so using this with any GitHub projects you have won’t be a problem.  It’s against the content of this server that the reviews will be built against.  I went along the path of having a local clone of the repository on the ReviewBoard server (more on that later) so for now it’s simply a case of cloning your repository onto the ReviewBoard server machine, somewhere the user used to run the ReviewBoard server can access it.

 

The ReviewBoard Repository

One you have a local clone, you can start setting up your repository in ReviewBoard.

Hosting Service: Custom

Repository Type: Git (obviously!)

Path: This needs to be the absolute path of the Git repository on the ReviewBoard server machine, not your local machine.  In my case it was simply ‘/home/administrator/git-repositories/repo_name/.git’.  Since we have a number of Git repositories on ReviewBoard they all get put in the same git-repositories folder so it’s easy to set them up.

Mirror Path: This is the URL of the Git repository you cloned from.  To find this, simply run the following git command and copy the address from the Fetch URL line.

git remote show origin

My Mirror Path (because we’re using SSH over Gitolite) is something like git@git-server:repo_name.

Save your repository and now that’s done.

 

Doing Your First Review

Now you can start on your first review to see if everything is set up correctly…  One thing to note is that a review will only be generated based on what you have committed to your local repository.  So if you have unstaged local modifications they won’t be picked up.

So, modify your code and commit.

When using Post Review (you are using Post Review, right?) creating a review is easy – simply call the following from the root of your Git repository (you can make it even easier by adding this to the right button content menu in Windows)

post-review --guess-summary --guess-description --server http://rb-server -o

If all has gone well, the review should pop up in the browser of your choice ready to be published.

 

Doing Your Next Review?

Now this will work fine until you push what you’ve been committing.  Now when you next commit and try to generate a review you’ll start to get rather cryptic errors…

The problem is the repository sitting on the ReviewBoard server is still in the same state it was when you first cloned it as the content you pushed hasn’t been pulled and the ReviewBoard server doesn’t check whether anything on the git server has changed.  So we need to make sure the RB server is keeping it’s local copies up to date.

It’s a shame this isn’t built into the Review Process to be honest, but I can understand why, so we simply need to do the work for it.

All I’ve done is create a simple Ruby script which spins through the repositories in ‘/home/administrator/git-repositories’ and polls whether anything needs to be updated.  If it does, it does a pull, if it doesn’t it moves onto the next one.

So as an example, just manually update the repository on the RB servers and try to post another review.  This time it’ll work flawlessly and you just need to set up a system to update the repositories that fits in with whatever system you’re using.

 

Creating Reviews Using Post Review

In the above examples we used the following command to create a review which pulled out all recent commits and generated a single review from that

post-review --guess-summary --guess-description --server http://rb-server -o

But there are other ways to generate reviews.

The following will create  a review containing only the last commit you made

post-review --guess-summary --guess-description --server http://rb-server -o --parent=HEAD^

This one allows you to create a review using the last [n] number of commits you made

post-review --guess-summary --guess-description --server http://rb-server -o --parent=HEAD~[n]

 

Why Code Reviews Don’t Work

I’m a strong believer in code reviews.  Having developers review and discuss code in development is one of the best ways to ensure good code quality and provides an excellent way to share domain knowledge with team members who may otherwise not get the opportunity.  It also allows experienced developers to share their knowledge and ideas with less experienced developers, an excellent way of distributing development skills across an entire team.

But review adoption doesn’t always stick.  I’ve seen the process taken up and then fizzle out on a number of teams usually for the same set of reasons.  The following are some of the reasons why a code review process can fail to take hold within a team.

Never Following Through
This is the biggest moral killer while trying to establish a code review process on a team.  What will generally happen here is that when code is put up for review, the author simply takes no notice of the comments or suggests and checks in the code without modification or without any follow up changes.  When other developers are taking the time to examine code and make positive suggestions they will quickly realise if they are simply being ignored.

And this kind of behaviour is both demoralising and infectious.

If developers see their comments being ignored they will very soon start to wonder why they even bother.  And if other developers don’t have to take the time to act of good suggestions, why should they.  Pretty soon you have one of the two following situations – people don’t even bother posting because nothing will come from it or you have review after review being raised and all of them being left uncommented and open for everyone to see.

Developers need to know they have the time to respond to feedback.  If they feel under pressure to move onto the next thing then they won’t have the desire (or ability) to actually respond to suggestions.  While not always possible, in the long term quality should take priority over speed, and if suggestions improve the code there should be an environment where reacting to these is important.

Reviews should always be classed as ‘finished’ when the feedback has been acknowledged and by tracking the number of open reviews it allows people to see what feedback still needs to be acted upon easily and allows people who are slipping to be picked up sooner rather than later.

Lack of Buy In
This is one of the hardest hurdles to overcome.  When a team simply doesn’t buy-in to the idea of code reviews, it will be difficult to get anything useful out of a review.  People won’t post reviews (often stubbornly refusing) or when they are the reviews will be superficial at best – finding very little of note and eventually killing the process through sheer apathy.

This can often be an attitude that is based on other problems in the team.  I have rarely met a programmer who didn’t want to talk about code, or who didn’t want to offer you a suggestion on how something could be improved or optimised.  The lack of desire to talk about their work, when it’s across the entire team, could be systematic of wider problems in the team.

But if the attitude of the team is generally positive and code reviews are the only sticking point, then it’s going to be less of problem trying to resolve these when trying to introduce a process into the team.

If there is a general malaise towards a process, strong leadership is one of the best ways to get other members of the team interested in a process.  Notice that I didn’t say strong management.  Leadership is a whole different ball game.

You can also start with those who are interested, keeping reviews small but public (they should rarely be private anyway).  As other members of the team see people in animated discussions on how something was implemented, it’s a unique programmer who doesn’t want to get involved at some level.

Unrealistic Expectations
I’ll go on record and say that you will rarely find bugs in code you review, especially if you are reviewing all or most of the code being written.  Or, at least you shouldn’t because your ‘bugs to code written’ ratio shouldn’t be high enough otherwise you have more serious problems.  But when a team is new to code reviews they can often have an expectation that issues will be found and they will be found often.  Which can cause a team to question what they are getting out of reviewing everything that’s being written.

But I’ll also go on record and say that code reviews can prevent bugs.

Code reviews benefit teams in the long term more than they do in the short term.  You might not be finding bugs on all your reviews but over time, if people are talking about code and sharing their experience, your team on the whole will become better programmers.  They’ll think that little bit longer before committing code if their peers are going to be looking at it, and they’ll have their bad habits smoothed out review after review.

And all this will lead to less bugs being added to the code base – but this is one of the hardest things to grasp when a team starts out and is looking for short term gain.

Peer Fear
This can manifest in a number of ways.

The first is through a fear of criticism where a developer is simply fearful of the criticism they will receive when putting their code up for review.  Unfortunately this isn’t always in the hands of the poster.  If a team has even one aggressive reviewer, this can effect the entire process, with people hesitant or even refusing to post their code, and when one reviewer starts to control the process, the ability to share knowledge decreases drastically.

The other side of the coin is where developers are worried about disagreeing with each other.  Comments on reviews are not always correct and not always the best course of action.  But if the team has developed a routine of automatically acting on suggestions (either from a subset of developers or all of them) then in effect any productive discussion is squashed and if people are afraid to say no, the code base isn’t always going to go in a good direction.

When this happens, the process can quickly lose steam.  If reviews cannot start a worthwhile discussion then the worth of reviews will decrease drastically.  If people see the code base is not improving (or even worse, degrading) they will not continue to post up reviews and the process will simply die out.

This is usually a management issue.  Domineering peers will destroy any process, but this can usually be resolved by taking that peer to one side, discussing the reasons behind the process and explaining how they can still be part of it while not taking over the discussion.  For those times where one developer is lacking the confidence to disagree, then it will often fall to other developers or the team lead to do the disagreeing for them, at least from the start.  As they start to see that disagreeing isn’t a negative trait in a review, they will start to feel more comfortable in reviews and discussions will start to flow.

Solutions?
Everyone likes a quick fix solution, but as with anything worth doing, it’s not always easy to solve these problems, especially one as difficult as a lack of buy in into a new process.  But I’ve generally found that most problems, like many team issues, are beaten by strong leadership and by being open about why the process is being introduced and how, in the long term, the developers lives will improve as a direct result of being part of it.

But to be honest, if you’re trying to solve these problems and you don’t buy into it yourself, then you’re onto a losing streak either way.  If you don’t believe in the process, then chances are you’re not to worried if it falls by the way side.  Which is fair enough.  Code reviews are not a mandatory part of software development and if the literature on the benefits of code reviews doesn’t persuade you, then nothing will.

Though I’d recommend you read them all again.

Code Reviewer – 5 licences, 5 dollars, 5 days

I’m not one for bringing up promotions from companies but it was brought to my attention the other day that SmartBear are offering 5 licences for their Code Reviewer tool (a more light weight version of Code Collaborator) for the tiny sum of $5. And in the way that marketing departments like things to flow together, they are only doing this for 5 days – from July 13th to 17th. 5 licences for $5, only for 5 days.

I’m a fan of SmartBear’s tools, I’d go as far as to say that Code Collaborator is one of my favourite code review tool out there, but to be honest what makes me want to mention this is that they will donate all the money raised from this promotion to a start-up to help them keep their heads above water (as of writing this it’s not been announced which one).

They are also providing a mid-week Webinar that will step you through how to use Code Reviewer, so check that out if you need a bit more information about it.

Anyway, if this might persuade you to give code reviewing a go or could help you formalise you process a little bit more, then check it out and see if this is right for you.

Why We Use ReviewBoard For Code Reviews

I was going to write a couple of reviews covering the various code reviews tools I’ve tried over the past year or so, but the notes I made on each one seem to have mysteriously vanished. And since it wouldn’t be fair on the various tools we used to try and remember why we moved on, I’m just going to cover the one tool we stuck with… ReviewBoard.

Web Based
This was one of the features that I really wanted from a review tool. No installs, no applications that need to be running in the background, simply a web address to access and an e-mail address for notifications.

Easy To Use
When you first look at the UI it’s pretty obvious what you need to do and how you can review the work in front of you. As with any UI, it has it’s quirks but I’ve not had anyone say “I can’t use this!” like I did with some other tools we tried.

Conversations
It’s easy to have a conversation in ReviewBoard and this is what I always want to see when code is up for review. Clicking on other people’s comments and seeing a clear ‘Reply’ button and being able to see the entire thread on the main page allows people to dive right in.

Openness
ReviewBoard doesn’t block people who are not on the review from seeing the code and making their own comments. As long as they are signed up to the site they can see anything being reviewed and what has been reviewed in the past. If I’m writing an internal blog post about a feature or implementation I am working on, I always add a link to the code review allowing everyone to have their say. After all, there is a good chance that they might use the code at some point in the future, so why shouldn’t they?

Quick to Review
Once you get notified of a review you have been assigned, getting to that review is as quick as hitting the provided link and selecting the diff. to review. Everything is done in-page and with a click on a button your entire set of comments and reviews will be published for everyone to see. It can be hard to persuade people that reviewing code will make them more productive, and having a fast process can make the argument easier.

Actively Developed
Nothing is perfect and ReviewBoard does have its bugs, but when you can raise an issue on their bug tracker and know that someone will actually look at it you start to feel a little more secure about any problems you find.

Widely Used
Having only one group of people use a tool can sometime be a benefit as it can be tailored specifically for you, but that can be a dead-end. ReviewBoard is widely used by some pretty big companies and you know it stands up to a certain level of quality and that the development being done will be for a wide range of users.

Stable
I’ve used tools in the past that didn’t work. They would fall over, become unresponsive or simple not do what they should. I’ve not encountered any of that with ReviewBoard. A few people using it have found some issues that get in their way, but as I’ve already mentioned these are quickly noted and will hopefully be resolved in the future.

So those are some of the main reasons we’ve stuck with ReviewBoard. It’s very easy to get hold of and install, so I really would recommend that if you want to try something new, then give it a go.

What Else Did I Try?
While I don’t want to go into the whys and why-nots of each tool I tried, I thought I would list some of the tools we did try so you can give them a go. What might not have been suitable for us could pretty much fit the hole you are looking to fill, so go and check these out too.

SmartBear’s CodeCollaborator – Try this out, it’s fantastic
SmartBear’s Code Reviewer
CodeStriker

Thanks and remember to enjoy your code reviews!

Code Review Methods

In the last post I went over some of the reasons why code reviews are important in building a stable and secure project. I wanted to follow that by covering the different review methods we’ve used in the past and the ways in which each one worked or in some cases didn’t.

Paper Based Reviews
One of the simplest, and first, methods we used – simply take the code you want to review, print it out in whatever format you want (we found small A5 booklets worked best) pass them around and then start reviewing. Usually people would be given a couple of days to review the code before everyone involved would sit around a table and discuss what they had found. The advantage to this is that it gets people talking. You are sat around a table together and it’s a great way to get people participating. Unfortunately it also forces people into thinking they must contribute rather than simply being happy with what is in front of them and it’s a drain on resources and time. It’s surprising how long it takes to get code from an IDE into a suitable (and readable) format, especially if all you have is a word processor. And wasting all that paper is something that I simply don’t want to continue doing.

Forums
We also tried posting code onto an internal forum when the review didn’t justify a full sit-down session. This worked very well in making the code easily visible – even to people outside the review group though that might not be suitable in every case. It also reduced the time it takes to set up and cuts down on waste. But discussing specific parts of the code simply didn’t work. Referencing line 316 of PlayerCamera.cpp in a post makes it very hard for people to link to the work, and while forums are good for limited threaded discussions, trying to quote and re-quote just becomes hard to track, read and find out what needs to actually change.

Automated Code Reviews
By automated I mean using a tool that has been created specifically for code reviews, one which can display code correctly, generate or show diffs and allow people to reference files and lines of code easily. I won’t go over the actual tools we have used (I’ll do that next time) but suffice to say this is what we use and the method we have stuck with.

Of course, when using automated tools you have more scope as to when to do you reviews (do you review individual diffs, whole files or something in-between?) and I’ll spend the rest of this article going over the different methods we’ve used so far.

Blocking Code Reviews: Blocking code reviews are used when you want every piece of code reviewed before it is even committed to the repository. When a developer has made any changes to the code base they fire off a review and the developer cannot check in their work until everyone in the review group has flagged it as shippable. Any changes or requests need to be done before the code review can be submitted and checked into the repository.

This didn’t work. Developers need to get into a work-flow and having to constantly stop and wait for permission to continue or to review someone else’s code stops them in their tracks and is a serious drain on their productivity. And let’s face it. Game development might be a multi-million dollar industry but lives are not on the line if someone checks in something that might not be perfect.

Non-Blocking Code Reviews: The next obvious step. Every check-in needs to be reviewed so the review is generated, sent off to a selected group, but the developer is then free to commit the code with any changes or improvements being done in another commit. This is beneficial on two counts. The original developer is able to continue working, either on the same area of code or another without it conflicting on any waiting commits, and the reviewer is able to continue their work, leaving any reviews the need to complete for when it’s suitable, maybe performing a group of them in one go.

While this worked well in regards to making sure that everything is reviewed, we found that you needed to monitor the size of the review groups and to check that people are not being over-loaded. Filtering your reviews only to come to them at the end of the day with 10+ of them waiting for you can be quite disheartening.

Feature Based Reviews: This is generally where we sit when it comes to diff based code reviews and it puts the responsibility back into the hands of the developer. If the developer is working on a new feature then they are free to check in their changes with a view to putting the whole feature up for review when it is finished. This greatly reduces the number of reviews and for new code allows the developer to put the whole code into context. It’s very similar to paper based reviews but in a more suitable environment. But there is one caveat. It only works if there has been a decent design stage to the feature being written. Having people bring up basic design flaws when the thing it practically finished can be a killer blow but this is showing up the work flow process more than anything else.
This can obviously be extended to large scale changes that someone might be working on. The can check in their small changes, which a review done on the final product, maybe generating the diff based on a collection of commits rather than just one.

Bug fixes are another matter. Once the main implementation is done and people are into a bug fixing stage then these should be reviewed as and when they are being committed (usually in the same manner as the non-blocking reviews). This is especially important when it’s coming up to a milestone and is the way continually developed projects (for example with Middleware or Technology) will eventually spend most of their time.

Other Methods

Obviously there are other methods we could try.  Review buddies, review masters (one person assigned responsibility for checking all commits – sometimes this will usually be the lead or programming manager), e-mail based discussions, ”important changes only’ reviews, and plenty of other variations on the same theme.  We haven’t tried these because the automated reviews have been working so well for us.

Review buddies would be the one I’d like to try (alongside, rather than replacing, the automated review process) but I would usually have this done between two developers working on similar parts of the code – though I would hope these developers would be talking to each other already!

Review Groups
One thing I’ve not covered is who does these check-in reviews, which can be just as important as what is being reviewed.

Initially we used review groups, which contained 2/3 developers and were mixed quite randomly. Every review would be assigned to a random group and fired off. While this worked by spreading the work and the knowledge of the code base, it wasn’t producing the best results. Having one person involved in 2 out of 3 reviews for a particular feature, only for someone else to replace them on the last one would produce duplicated, or sometimes conflicting, results. And the new person isn’t as up-to-date with the feature as the original set of reviewers, which can limit the scope of the review and lead to it taking longer.

While this does spread the knowledge throughout the team, it isn’t necessary for everyone to know everything about the code base.

So now reviews are generally more focused, being sent directly to specific people who will have the best input on a particular feature. This does need monitoring to make sure people are not being over-loaded (or over-looked) but that’s a pretty simple management issues. People can also be asked whether they want to be involved in a certain review. Do they have a particular interest in that area, or does it use a new piece of technology that they would like experience with?

So that’s a brief over-view of the code review systems we’ve used over the past couple of years. We’ve settled on our automated code reviews as they produce the best results in the shortest time and give you much more scope in regards to how and what is reviewed. Choosing the correct piece of software to run this system is an important decision and I’ll go through the tools we used and most importantly the ones we stuck with in the next post.

I’ll Show You Mine If…

Code reviews have a bad reputation.  Claims of wasting time, pedantic reviewers and hot-shot programmers all crop up as reasons why teams shouldn’t and don’t bother reviewing any of the code that is being used in what could be a multi-million dollar project.  But why would you want to have a code review?  What does it bring to the project that would otherwise be missing – because that’s the point really, if it is a waste of time then should we be doing it at all?

The Reason For A Review
Every review needs a reason.  Throwing some code in front of a group of developers without purpose will generally end up with a bunch of incoherent comments, suggestions and ideas that might be good, but depending on the project or the time scale, totally irrelevant or unfeasible.  The following are just some of the reasons you might want people to look over a set of code.  There are, of course, many more reasons but I think the majority will fall under one or more of these banners, regardless of who is doing it or what you hope the outcome will be.

Finding Bugs
It’s funny, and it might just be me, but every time I’ve asked someone to put up something for the first time, at least one bug is always found.  It might be a misplaced define, or an = instead of an == or any number of things but there’s always one.  And I know that the time it took to post and complete that review will be much less than the time it takes for that bug to be found, a bug report written, the author to read the bug, find the code in question, do some tests and then finally fix it.

Obviously not every bug will be found by a review.  It might be 1 in 10, it might be worse or it might be better.  But the point is that bugs will be found that otherwise would sneak through into QA or milestone builds, and each one will take time to find, report and fix.  Time that could be better spent doing something much more productive.

Stability
While I can pretty much guarantee that a bug will be found in the first review someone does, I would also place bets on the number of bugs being found as the reviewing continues goes down.  The number of suggestions and improvements people make will become less demanding and more of a conversation.  This isn’t because the reviewers are losing heart, or that less code is being reviewed, it’s due to one simple fact – the build is become more and more stable as more reviews are done.

This could be down to a number of reasons, but one of the biggest reasons in my opinion is peer pressure.  Programmers are suddenly aware that they are not the only people who will see this code, that their peers will be looking at and critiquing the work being done.  And most people want to look good next to their peers.  This is not a bad thing despite the connotations ‘peer pressure’ can bring to the mind.  Mutual respect with your co-workers and the need to drive yourself forward are often driven by peer pressure, and when one or two people start to move forward, a good team will always follow.

Sharing Domain Knowledge
The more people know about a particular system the better.  Illness, team moves and resignations will always be part of the work environment and it means that the key team member you are relying on could be gone before you know it.  And if you need someone else to dive in there and fix a couple of issues that have been found by QA, how long do you think that will take?

Code reviews can help.  As code is written, modified or fixed it is being looked at by more than one person.  The author has to explain their decisions, how things work and how things plug together.  Because of this you might have 2 or 3 people on your team that are happy to dive in and help.  Even if you need two developers to pair because the knowledge is spread between them, it’s still better than having no-one know what the heck that particular piece of code is doing.

Sharing Experience
People learn best when they are surrounded by people they can learn from.  Books and blogs are excellent resources (as an avid reader of books I would say that), but there is no substitute for someone sharing their years of experience with you in a 5 minute conversation.  Having people suggest different ways to structure, optimise and write code might seem a little late when it’s already written, but it’s something people can take with them into their next task, rather than going back and re-writing large chunks of code.

And it works the other way too.  There is nothing better than graduates coming into a company with their crazy ideas about how code should be written or used and teaching the old guard a new trick or two (C++? Inheritance? Madness I tell you!).

Coding Standards
This is one of the reasons people hate code reviews.  They don’t want them to turn into conversations about where the bracket should go, or if that variable should have an ‘i_’ prefix or not.  And on the whole they are right, and if this is what your code reviews turn into then you are simply ‘doing it wrong’.

But this has its place.  Developers new to the team can be given documentation about coding standards, but it is one of those things that tend to easily slip, and there is no reason why this shouldn’t be picked up in a review.  If it continues and every review becomes about how the standards are not being met, then you have an issue that should be resolved outside the review before it makes the whole process a pointless and much less useful exercise.

Problems With Code Reviews
Of course code reviews have their problems but then what doesn’t?  Most of the time these have nothing to do with the actual review process and more to do with the management of the team, or what has come before.

  • Arrogant reviewers & victim syndrome – Code reviews are there to improve the code base for the benefit of the team.  They are not there to ‘go on the attack’, to rip someone’s code apart or to make someone feel like they are simply not good enough.  If you start to find that one or two programmers are simply making blanket statements or not taking anything on board then this is certainly a management issue and should be resolved outside the review process.
  • Pointless reviews – This was pretty much covered in the Coding Standards section, but there is always a chance that code reviews start to only skirt the surface of the code without looking at anything to deeply.  If this starts to happen, you need to reassess what each review is for and what each person’s role is.  And remember, there is nothing wrong with simply saying “This looks great”.
  • Lack of outcomes – People will argue, and people will disagree, especially when it comes to code.  “You should do it like this”, “I don’t want to change that because…”.  Discussions are great and should always be encouraged, but at some point there needs to be a decision.  You have to know who has the final say – whether this is a manager who is on every review, or a senior who has the last say.  A simple comment along the lines of “That’s a great suggestion for the future” can put an end to most arguments with most people being happy with the outcome.  If they are not then again this needs to be resolved outside the review process before it goes any further.
  • Lack of time – It’s an important milestone week, code needs to be fixed quickly and you don’t have time to review it.  This can happen and sometimes you need to take it on the chin.  But isn’t this the time when it’s even more important that everything is checked and double-checked?  If you have a milestone week and your changes are coming thick and fast, then maybe you don’t have time to review everything, but why is so much changing at such a critical time?