In my opinion, code reviews work best when you keep it informal and don't have a checklist of specific issues. Just look at the code, try to understand what it does, and then nitpick at it. If you think it could be expressed easier, then either make a suggestion or just make a fix. Whatever you see that seems like it could be better - say it. Don't get into a style war, but there's a difference between 'I think that open bracket would look better on the same line as the if block' (don't mention that unless the team decided to do it one way), and "you're doing a .containsKey() check and then .get()ing the key. Don't do that - just get the key and compare to null afterwards, because that's much easier to fit into a multithreading model." (do mention that, especially if you know you're already using this map in a multi-threaded environment, or that it might be likely, and even if you don't, there are going to be situations where it is relevant, and when faced with a choice, doing it the same way every time is better).
Code reviews should be a two-way street. If you think something doesn't fit in the architectural plans, then open a discussion, and if the two of you can't figure it out, take it to the group (there's a lot of advantages to working with a small group, so use it!) If you as a reviewer make changes instead of suggestions (which should definitely be allowed), always send it back upstream for a counter- review by the original programmer. Use a code review system and do code reviews personally, not in a group. Basically, anybody that has a commit, toss it in the 'to-be- reviewed' branch so the reviewer can pull it, or just push a git branch over to the reviewer, and the reviewer then gets back to the original programmer with either: - a signoff, - a signoff with conditions (minor nits that you trust the programmer can fix without needing another review) - a date to meet up and discuss issues face to face - a realization that this is particularly crucial to the entire group. This may indeed warrant sitting everyone in front of a projector. But should be a very rare event. There are a bunch of tools out there. I know google takes their own code review process rather seriously, but I'm not sure if the tool they use (isn't it called mondrian, written in python by Guido van Rossum?) is available for use outside of google. A DVCS can help a lot by allowing people to use 1 branch per thingie-to-be-code-reviewed, and to allow people to push and pull these branches around without needing a central server. I strongly recommend git or hg (and, eventhough hg has some sway in java land, git does seem to have the momentum). And, whatever you do, make sure everyone agrees that life is better with the code reviews compared to without them. If its a management dictatorial decision, then programmers WILL find a way around it, -or- if they don't like each other, are going to use it as a forum for asinine nitpicking instead of constructive criticism. On Feb 12, 11:01 am, jvb <[email protected]> wrote: > In our company, we are thinking about introducing some formalized form > of peer code review. We (the Java guys) are a small team of 5 > developers only, and in practice we already discuss each others code > all the time (informally). There are of course some highly specialized > parts of the codebase that are almost exclusively known by one or two > persons only, certainly because we don't work on a single application > but more a whole suite of applications (used to manage satellite earth > stations). > > The person who wants to introduce this idea of a more formalized peer > code review comes from a different enviroment (mainframe stuff) where > the write/compile/test cycle was much slower. In his environment it > was obvious that code review (even before compiling if I understand > correctly) is very usefull to find bugs which would otherwize slow > down the already slow write/compile/test cycle even more. > > Now I'm thinking how we can translate this to our java development > environment. We are obviously not going to review code to find compile > errors ;-) > > - What kind of errors should we focus on when doing code review > (style? concurrency issues? use of patterns? whether it fits in the > overall architecture? whether the code actually implements the > requirements? ...) > - How do we best organize these peer reviews such that they will > provide a real benefit, knowing that our day to day way of working > already implies working with each others code a lot? > - Should we be focussing on the parts of the code that are less known > by everybody? > - Should we be sitting together around a screen/projector, or should > we simply assign parts of the codebase to be reviewed by certain > developers on their own? > - Should we be looking at tool support? > - ... > > Does anybody has any experience and/or usefull information to share? > > many thanks in advance, > Jan --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "The Java Posse" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/javaposse?hl=en -~----------~----~----~----~------~----~------~--~---
