Code Review Practices

[John Stager]

Hello All,

 

I am trying to set up a process for code reviews at my work and I am looking for some opions on how other shops do things. 

 

Is there any software out there that you use to help re-enforce quality code?

 

Do you envolve all developer or have a select few doing the code reviews?

 

How often should the code reviews be done?

 

Thanks in advance.

--
John W Stager

[Viktor Klang]

Greetings John,

You cannot enforce what you cannot define.
So until you have a clear definition of "quality" of code, you are stuck.

Cheers,
-V

--
Viktor Klang
Rogue Software Architect

[John Stager]

Hello Viktor,

 

That is true, we are attempting to implement standards and are using "The Elements of Java Style" book as a starting point.  Once we have company wide standards I want an easy review process for each project to review code so that we can find poor implementations.

 

How do you define "quality" code or are you saying that it is extremely difficult to do so.

 

Thanks.

 

[Viktor Klang]

On Wed, May 7, 2008 at 1:43 PM, John Stager <john....@gmail.com> wrote:

Hello Viktor,

 

That is true, we are attempting to implement standards and are using "The Elements of Java Style" book as a starting point.  Once we have company wide standards I want an easy review process for each project to review code so that we can find poor implementations.

 

How do you define "quality" code or are you saying that it is extremely difficult to do so.

Hi John!

I'm saying that it's difficult to define.
Programming languages are not about function, they are about elegance, and we all have different ways of expressing us in.
And I think it's hard to have a really tight definition of code quality.

One approach that I have found useful is not to review someone elses code, but for someone else to explain their code to you.
Why they have chosen to write as they have, and what are the pros and cons of the code.

It's important to be able to criticise yourself _before_ you criticise others.

What did you plan to do?

Cheers,
-V
 

 

Thanks.

 

On 5/7/08, Viktor Klang <viktor...@gmail.com> wrote:

Greetings John,

You cannot enforce what you cannot define.
So until you have a clear definition of "quality" of code, you are stuck.

Cheers,
-V

On Wed, May 7, 2008 at 1:34 PM, John Stager <john....@gmail.com> wrote:

Hello All,

 

I am trying to set up a process for code reviews at my work and I am looking for some opions on how other shops do things. 

 

Is there any software out there that you use to help re-enforce quality code?

 

Do you envolve all developer or have a select few doing the code reviews?

 

How often should the code reviews be done?

 

Thanks in advance.

--
John W Stager

--

Viktor Klang
Rogue Software Architect

[John Stager]

Excellent point, coding is an art form.  I think that your suggestion about the developer explaining their code is an excellent idea and one that would be helpful.

 

I think that for now we are looking at a way of enforcing company standards.  We are not looking at being the "bad" guys, but to create an openess in the team for everyone to express there input and to improve the overall quality of the code we deliver.

[Luc Trudeau]

One way to get people involved is to make it part of the process.

If you are working with Iteration or sprints a good idea would be to enforce a certain number of work hours per iteration to improving the code base (peer reviews, refactoring...). We usually have a task in our sprint planning that's called code review.

Note that you also need to plan time for the rework or fixes that will need to be applied to the code.

--
Luc Trudeau

Ecole de Technologie Superieure

[Amarjeet Singh]

Besides getting into the subjective side of a review, you might want to incorporate some of these into the whole process:

- Make PMD or any other good static code analyzers a part of the build process.

- Make sure that the file format is agreed upon. If your version control is on unix/linux and you are editing files on lets say, a Windows machine, then CRLF is an issue. Most of the modern day editors figure out the original file type and keep it maintained across multiple saves.

- Review in a peer mode, and do it frequently in short sessions. For me, many a times I have worked on existing and large code base, and this would mean that the code needs to follow some of the framework patterns already pre-existing into the application, somewhere deep into its guts. Any new developer is not expected to know all this, but this could be a nightmare if the breaking of a pattern impacts application functionality. To give you hint, think about large finance applications. They have stringent guidelines for most of the things that are resource intensive, including DB access, network connections, etc., and must go through either aan already existing providers for the same, or a well document way of doing such a thing.

- Build openness into the team. Ask your developers difficult questions, like, "Do you think that the class that you just wrote would be able to withstand multiple thread access to its instances?".

- Do not kill the developer. Remember that there is an acute shortage for really good ones and if you spot someone who can be a better one, spend some time with him or her and get him or her up to speed. Building reliability into the team would mean that code reviews in the end finally meet the pre-defined goals set.

- For meeting non-functional requirements, make sure that your code stands the test of passing them too. But, do not stress upon upfront optimization. Early optimization is evil. Someone surely said that!

- Discourage things like nested ternaries, or smart short code, that is difficult to understand and dig; when its time to maintain the application, these shortcuts are a nightmare.

- Log, log and log. If logging is not done correctly, then on production, there is no way you would be able to know what happened if something went wrong. Its nice to have the rolled-over logs with you, as these are time machines that let you go back and dig the problem out.

- Appreciate good code. Show them a few slides as to how you code effectively and lead by example.

- And why just Java? There is a tonne of JavaScript in your applications, if it is fronted by a browser. Encourage commenting in your JS files too and teach the team OO JS techniques and JS namespacing. I would recommend "Pro JavaScript Techniques" from AsPress. [Hope I got the title right].

- I can go on and on...

Thanks!

Amarjeet

--
Amarjeet Singh
Phone: +91-98712-76661

[Peter Becker]

Very good points.

PMD is always my first choice and I make sure I have a set of hard rules that fail the build -- soft rules can be helpful, but if you mix the two types people don't obey the important rules anymore. I tend to start with a small set of rules that can fail the build (but shouldn't), then add a rule and fix the problems. Then repeat adding rules until you feel there are no rules left you care about. Spread over months -- otherwise you lose a sense of progress on the project (possibly even progress itself). Another set of rules can be there for explicit code reviews -- things that shouldn't be done but might be in exceptional circumstances. Making those hard with @SuppressWarning for the exceptions tends to very expensive to manage and there are usually many other things to do that add more value.

I'd add some more tools into the mix in the long run. I would consider adding FindBugs and/or Checkstyle, although if you are just starting to do this type of stuff you are probably better off first exploring PMD's possibilities before introducing more tools.

Dependency control can be interesting, too. JDepend is the first one that comes to mind. I used to play with XRadar a long time ago and it seemed quite interesting around that time -- but not good enough. It's passed the 1.0 now, so it might be worthwhile looking at again.

There are many other tools around, see e.g. http://java-source.net/open-source/code-analyzers

This might help, too: http://www.wakaleo.com/java-power-tools

I haven't read the book yet, but I tend to enjoy John's blog posts. [Insert pun on his name here]

HTH,
   Peter

[John Stager]

Thank you everyone, that is a amazing amount of information that I need to digest.

I knew of the FindBugs  but was unaware of the rest.  I will definitely look into the other mentioned including PMD. 

--
John W Stager

[mkpapp]

Hi John,

Findbugs is a good tool (as is PMD). You have some excellent
suggestions from the group. I would just add a few ideas:

If your project(s) fall into functional areas (e.g., I/O, database, et
al), establish 'owners' for each area have them review code relevant
to their particular area. More far-reaching code should be reviewed
by a rotating team of 2 to 3 people. Small changes can be reviewed
via the email system, or better yet, through the bug tracking system
(if you track all changes to the code via that mechanism). This is
very effective for distributed projects like Mozilla, but has been
very effective with internal projects I have been involved with. In-
person reviews should be limited to significant or fundamental changes
in the code. While they are excellent, people get burned out if they
are attending a lot of formal reviews on a daily basis.

As for coding standards, don't get me started. Just suffice it to say
that (IMHO) they should not read like Miss Manners' Guide to
Formatting ("don't use tabs, put braces here, indent like so") and
should actually be coding guides. Like when to use singletons or
factories. How (or if) DI should be used. Guidelines for using
external resources such as databases, email systems, etc. Guidelines
for how the team or company uses frameworks such as Spring, Struts,
Hibernate, et al. In other words, any engineer should be able to look
at these 'standards' and figure out how things work. Using a Wiki is
a great approach. Code review information and the use of the bug
tracking system is also useful. Just trying to make the point that so
many coding standards docs are no more than guides to coding grammar
and completely useless - they should get everyone on the same page (if
you pardon the pun).

On May 7, 6:20 pm, "John Stager" <john.sta...@gmail.com> wrote:
> Thank you everyone, that is a amazing amount of information that I need to
> digest.
>
> I knew of the FindBugs but was unaware of the rest. I will definitely look
> into the other mentioned including PMD.
>

> On Wed, May 7, 2008 at 6:58 PM, Peter Becker <peter.becker...@gmail.com>

> > On Wed, May 7, 2008 at 11:47 PM, Amarjeet Singh <amarjeet.aur...@gmail.com>

> > > On Wed, May 7, 2008 at 9:18 AM, Luc Trudeau <luc.trud...@gmail.com>

> > > wrote:
>
> > > > One way to get people involved is to make it part of the process.
>
> > > > If you are working with Iteration or sprints a good idea would be to
> > > > enforce a certain number of work hours per iteration to improving the code
> > > > base (peer reviews, refactoring...). We usually have a task in our sprint
> > > > planning that's called code review.
>
> > > > Note that you also need to plan time for the rework or fixes that will
> > > > need to be applied to the code.
>

> > > > On Wed, May 7, 2008 at 8:58 AM, John Stager <john.sta...@gmail.com>

> > > > wrote:
>
> > > > > Excellent point, coding is an art form. I think that your
> > > > > suggestion about the developer explaining their code is an excellent idea
> > > > > and one that would be helpful.
>
> > > > > I think that for now we are looking at a way of enforcing company
> > > > > standards. We are not looking at being the "bad" guys, but to create an
> > > > > openess in the team for everyone to express there input and to improve the
> > > > > overall quality of the code we deliver.
>
> > > > > Thanks.
>

> > > > > On 5/7/08, Viktor Klang <viktor.kl...@gmail.com> wrote:
>
> > > > > > On Wed, May 7, 2008 at 1:43 PM, John Stager <john.sta...@gmail.com>

> > > > > > wrote:
>
> > > > > > > Hello Viktor,
>
> > > > > > > That is true, we are attempting to implement standards and are
> > > > > > > using "The Elements of Java Style" book as a starting point. Once we have
> > > > > > > company wide standards I want an easy review process for each project to
> > > > > > > review code so that we can find poor implementations.
>
> > > > > > > How do you define "quality" code or are you saying that it is
> > > > > > > extremely difficult to do so.
>
> > > > > > Hi John!
>
> > > > > > I'm saying that it's difficult to define.
> > > > > > Programming languages are not about function, they are about
> > > > > > elegance, and we all have different ways of expressing us in.
> > > > > > And I think it's hard to have a really tight definition of code
> > > > > > quality.
>
> > > > > > One approach that I have found useful is not to review someone
> > > > > > elses code, but for someone else to explain their code to you.
> > > > > > Why they have chosen to write as they have, and what are the pros
> > > > > > and cons of the code.
>
> > > > > > It's important to be able to criticise yourself _before_ you
> > > > > > criticise others.
>
> > > > > > What did you plan to do?
>
> > > > > > Cheers,
> > > > > > -V
>
> > > > > > > Thanks.
>

[John Stager]

Excellent point, about people rotating.  This will not only remove the bottle neck of having one or two people reviewing code it will expose developers to areas that they may not have worked on before. 

 

My goal is to not add a lot of over head to the development process with meetings and extremely long processes to review code.  Neither do I want to make this a policing process, but I think that I can take these suggestions to make this process seamless. 

 

The problem with the application that we are working on is that it was create/maintained (still is) by a consulting company who have provided little to no design documentation (let alone little comments in the classes) on what the overall archecture is and how you should extend it.  So know that we are attempting to only review the area of the application under my teams control.

 

Does FindBugs and/or PMD make it easy to plug into an automated build with ANT?

 

--
John W Stager

[Peter Becker]

On Sat, May 10, 2008 at 12:18 AM, John Stager <john....@gmail.com> wrote:

Excellent point, about people rotating.  This will not only remove the bottle neck of having one or two people reviewing code it will expose developers to areas that they may not have worked on before. 

And it might solve the problem of those one or two people having their own blind spots. Everyone has.

 

My goal is to not add a lot of over head to the development process with meetings and extremely long processes to review code.  Neither do I want to make this a policing process, but I think that I can take these suggestions to make this process seamless. 

IMO the trick is to ease people into it so it can become second nature. If you start doing all at once the costs for the individual will always seem to outweigh the value. But if you start with those things that add value to the individual (by either catching problems or just adding a general sense of security and new boldness), then you can get people to actually like the tools.

 

The problem with the application that we are working on is that it was create/maintained (still is) by a consulting company who have provided little to no design documentation (let alone little comments in the classes) on what the overall archecture is and how you should extend it.  So know that we are attempting to only review the area of the application under my teams control.

Sounds bad. I don't really care much about comments in classes myself, but I like to see an overall architecture described in brief and good JavaDoc on interfaces (and plenty of interfaces of course). "Good JavaDoc" does not necessarily mean a lot (avoid the smell of turning method names in to sentences), but there are very important points that are most commonly completely undocumented -- e.g. how the null value is handled (allowed as parameter? possible as return value? what happens if a parameter is null?), what range an integer value can have or what other constraints on values are.

I would always include a check for good JavaDoc at the right spots into a manual code review -- otherwise you start repeating the same questions again and again.

 

Does FindBugs and/or PMD make it easy to plug into an automated build with ANT?

I'd call that the normal way of using them. You can IDE-plugins, but I rarely use those neither do I use the command line tools. Pretty much all the tools can create HTML reports from Ant, which you can then put somewhere accessible on your CI server.

  Peter

[Peter Becker]

I agree that focussing on the syntax is usually not a good idea when structure is so much more important. Unfortunately it is hard to ignore syntax derivations and if there are too many the code just starts looking messy.

A very good solution that I have used in two projects so far is to use an automated formatter in the IDE. Before each commit this formatter should be run, you can also run it server-side to validate that the formatting is still ok. Of course these formatters are never perfect, but since no two people agree on style it is often better just to blame the tool and live with it :-)

The ideal case I have found was this:
- everyone on the team uses Eclipse
- the official formatting is defined for the Eclipse formatter
- the project has the code formatter enabled as "Save Action"

That last thing means the formatter is run whenever a Java file is saved. That is a huge relief for me since I don't have to care about my own formatting nor about running something before a commit. Sometimes I will even write code without whitespace at all, knowing that I can fix it with a quick Ctrl+S. One less thing to care about.

Of course that requires using Eclipse throughout the project (ideally at least 3.3 -- the formatter is better than in 3.2, although the latter is still ok). Maybe other IDEs can do it too, but I suspect getting consistent formatting within different IDEs will be very tricky. I've never manged to get two tools to produce exactly the same output. And I haven't seen the "Save Actions" feature in other IDEs -- but I'm not really up-to-date with any other IDE at the moment.

But in any case: IMO there should be an automated formatter used (turning the syntax part of the coding style into a tool definition), it should be run regularly against the repository version (at least to check, automated commits are dangerous) and every developer should be able to run this formatter easily (IDE plugin/Ant task/command line depending on taste). You might find some complaints about the formatting not being perfect at first, but once the tool has been established people tend to shut up -- to get the same effect without automated formatting you'd have to have a very specific formatting guide, which is an annoying thing to have. "Code has to be formatted by tool X using the following definition file" is much nicer than N pages of description of what the code should look like.

Note that you still need to agree on naming schemes and all the higher-level stuff, but for some reason there is usually much less argument about those (apart maybe from the "LOGGER" vs. "logger" discussion :-) ). Might at least partially be a case of painting the bikeshed.

The goal has to be to identify a final solution -- these things are not that relevant to the project success but somehow keep creeping back onto you if you don't put a lid on. The more you can enforce by tools the less you get into discussions later on.

  Peter

[Brad Rymer]

Hi John,

I can recommend using Fisheye as a tool for examining your code base
and Crucible as a tool for conducting code reviews.

Both tools are made by Atlassian - atlassian.com

They are both web applications that make it easy for code base
examination and code reviews to be conducted on any computer. They
also support a query language where you can have reports like "Tell me
all the check-ins on the production branch that haven't been code
reviewed".

The fact that Crucible is a web-based tool means there is very little
hassle in initiating a review or annotating code changes with
comments. Getting developers to consistently perform code reviews in
the first place can be 80% of the battle.

I don't know if I've done a great job of explaining the features but I
encourage you to checkout the website. We've seen lots more code
reviews being performed since using these tools.

Cheers,

Brad.