+1 for using style-enforcing tools, and the missing part of the Java compiler (PMD/Findbugs). Insofar as they're being used, though, you need to decide what is and is not part of your style.
I, for instance, consider "final" parameters to be mandatory. The fact that parameters aren't de facto "final" by default I consider a glaring failure in the Java language. This has caused people to react violently on both sides, and the final question is whether that's an acceptable deviation in style. To answer that question, remember to see the forest for the trees -- Why do we really care? How is thinking/talking about this particular issue making the business more profitable? Without a clear answer to those questions, the style question doesn't matter, and it should be left aside. Sometimes that does matter: for instance, double-checked locking, dereferencing possibly null variables, and methods with huge numbers of parameters are all places that can cause hard-to-track-down bugs and ruin the application. Don't use style-enforcing tools to force everyone's code to look identical. Instead, use Jalopy[1] or something along those lines to deal with people who just can't get over their obsession with tabs vs. spaces, where braces land, as well as expanding imports, etc. It's not simply worth the effort trying to enforce some kind of standard when the code can be cleaned up automatically. Getting back to the original topic, code reviews are good, but I've never seen the "mandatory pre-check-in review" work for long. I've heard of places where it's worked well, but these places seem downright mythical to me because I've never seen it last for myself. Sooner or later, people get lazy about doing them, and code just gets pushed in because it "has to go out". For more involved sit-down-drag-out code reviewing, I'd suggest having people present code that they didn't write and don't have familiarity with. Expect the person to have a firm grasp of the code, though, which may mean sitting down with the original author and hacking through it. By not having a developer present their own code, you're removing a huge temptation for the code review to become defensive and/or antagonistic. Starting with code identified as bad through some kind of objective metric (crap4j or even just using warnings from CheckStyle/FindBugs) is probably a good way to go, because it also draws attention to what's going wrong in the codebase. And it sets a known bar of quality to avoid having your code end up in front of the code review. :) If you read nothing else from the e-mail, read this: *If you're talking style in your code reviews, you're doing something very, very wrong*. [1] http://www.triemax.com/products/jalopy/ ~~ Robert Fischer. Grails Trainining http://GroovyMag.com/training Smokejumper Consulting http://SmokejumperIT.com Enfranchised Mind Blog http://EnfranchisedMind.com/blog Check out my book, "Grails Persistence with GORM and GSQL"! http://www.smokejumperit.com/redirect.html Luc Trudeau wrote: > IMHO > > You should not waste time reviewing things that tools can find for you. > Before setting up a code review process, I would start with : > > * Define a coding format and use Checkstyle to enforce it (IDE can > help you out with this) > * Enforce a 0 compiler warning policy > * Use PMD and Findbugs to look at the code. You will also need to > define what FindBugs/PMD issues you're willing to live with. > > After you set these foundations, I would use crap4j to list the crap in > the code base. And I review that starting with the crappiest. > > > In order to keep this answer short, I'm not going to address TDD. Which > can also be good for reviewing good and share knowledge. > > > > On Thu, Feb 12, 2009 at 5:01 AM, jvb <[email protected] > <mailto:[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 > > > > > -- > Luc Trudeau > -- > A computer scientist is someone who, when told to 'Go to Hell', sees the > 'go to', rather than the destination, as harmful. > > --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
