There's an old adage that goes something like: 'Do not talk about
religion or politics'. Why? Because these subjects are full of strong
opinions but are thin on objective answers. One person's certainty is
another person's skepticism; someone else's common sense just appears as
an a prior
bias to those who see matters differently. Sadly, conversing these
controversial subjects can generate more heat than light. All too
often people can get so wound up that they forget that the outcome of
their "discussion" has no bearing on their life expectancy, their
salary, their chances to win x- factor, getting that dream date,
winning the lottery, finding a cure for climate change or whatever it is
they regard as important!
Similarly, in the world of software engineering code reviews can end up
in pointless engagements of conflict. Developers can bicker over silly
little things, offend each other and occasionally catch a bug that
probably would have being caught in QA anyway - that conflict free zone around the corner!
Now don't get me wrong, there are perfectly valid reasons why you may think code reviews are a good idea for your project:
- Catching bugs sooner means less cost to your project. You don't have
to release a fix patch because it's has been caught in development
phase - yippee!
- Code becomes more maintable. That crazy 200 line method that Jonny
was writing with a hangover has being caughted before it has the chance
to make itself at home deep in your code base.
- Knowledge is spread across your team. There are no longer large
blocks of code that only one person knows about. And we all know, when
that one person talks about taking a two month holiday everyone panics!
- Developers make more of an effort. If a developer knows someone else
is going to pass judgement on his work, he's more likely to put that
line of javadoc to clarify when an exception will be thrown.
However, it would be naive to think that code reviews don't cause problems. In fact, they cause so many problems many 21st century projects don't do them. I think they have a place but there needs to be some thought regarding how and when they are done so that they are beneficial as opposed to a nuisance. Here are some guidlines...
Ensure you have tested your code before you asked someone else to look
at it. Catch your own bugs and deal with them before someone else does.
- 2. Automate as much you can
There are several very good tools for Java such as PMD, Checkstyle, Findbugs
etc What is the point getting a human to spend time reviewing code
when these tools can very quickly identify many things the human would
waste time moaning about? I am going to say that again. What is the point getting a human to spend time reviewing code when
these tools can very quickly identify many things the human would waste
time moaning about?
When using these tools, it's important to use a common set of rules for
them. This ensures your code is at some sort of agreed standard and much
of what used to happend in an old fashioned 20th century code review,
won't need to happen. Ideally, these tools should be run on every
check in of code by a hook from your version control system. If code is
really bad - it will be prevented from being checked in. Billy the
developer is prevented from checking in the rubbish he wrote (when he
had killer migraine) that he is too embarrassed to look at. You are
actually doing him favours, not just your team.
In some of the earlier Java projects I worked on, the reviews happened way
too late. You were reviewing code when the actual design was flawed. A
design pattern was misunderstood, some nasty dependencies were
introduced or a developer just went way off on a tangent. The review
would bring up these points. The proverbial retort was: 'This is a code review not a design a review!'
. A mess inevitably ensued. To stop these problems we changed things
so that anyone asked to review code would also be involved - in some way
- in either the design or the design review. In fact, we got much more
bang from the buck from design reviews than code reviews. Designs were of a much higher quality and those late nasty surprises stopped.
- 4. Agree a style guide (and a dictionary)
Even with the automated tooling (such as Checkstyle, Findbugs etc), to
avoid unnecessary conflict on style, your project should have a style
guide. Stick to the industry standard java conventions - where
possible. Try to have a 'dictionary' for all the concepts your project
introduces. This means, when code refers to them it's easier to check
that the usage and context is correct.
If all your developers are using Eclipse (and happy using it) something like Jupiter
makes sense. You can navigate your way through code, debug code and
essentially leverage everything the Eclipse IDE does to make your life
looking at code easier when reviewing code. However, if everyone is not
on the same IDE (or the IDE is not making your life easier) consider
something like Review Board.
- 6. Remember every Project is different.
You may have done something in a previous project that worked. But
remember, every project is different. The other project had a certain
architecture (may have been highly concurrent, highly distributed), had a
certain culture (everyone may have enjoyed using eclipse) and used
certain tools (maven or ant). Does the new one tick the same boxes? Remember, different things work for different projects.
- 7. Remember give and take
When reviewing be positive, be meticulous but do not be pedantic. Will
tiny trivial things that get on your nerves make a project fail or cost
your company money? Probably not. Put things in perspective. Remember
to be open to other ideas and to change your own mind rather than
getting hung up changing someone else's.
In my experience, what I call 'buddy reviews' (others call 'over the shoulder') have worked really
well. A buddy review consists of meeting up with another team member
informally every day or two and having a quick glance (5 - 10 mins) at
each other's code at your desk or their's. This approach means:
- Problems are caught very early
- You are always up to speed as to what is going on
- Reviews are always very short because you are only looking at new code since the last catch up
- Because the setting is informal - there is no nervous tension. They're fun!
- You can exchange ideas - regularly.
When Tech Leading, buddy reviewing your team members is a great way of
seeing if anyone on the team is running into trouble early rather than
late. You can help people and get an idea of everyone's progress all at
the same time. And because of the regular nature of buddy reviews,
they become habitual and actually get done. Something we can't say for
many other 21st century code reviews!
In summary, if your project is going to engage in code reviews, they
should be fast, effective and not waste people's time. As argued in
this post, it is really important to think about how they are organised to ensure that does not happen.
'Til the next time - take care of yourselves.
Source:http://java.dzone.com/articles/code-reviews-21st-century
