Reinier Zwitserloot wrote: > In regards to Robert Fischer's 'all parameters must be final': That's > exactly the kind of language war that should not be the focus of code > reviews. Broach the subject with the team, and if there's clear > concensus that one way is the right way, then, sure, from that moment > onwards, enforce it in code reviews, but if there's no consensus, then > don't bother. > I like all code styles that match mine!
really, this is what code formatters are for... I fail to see the benefit of using final in a parameter list. Regards, Kirk > > On Feb 12, 3:45 pm, jvb <[email protected]> wrote: > >> Thanks for the information. >> >> In what you describe (and this seems to be what most of the tools >> assume as well) the review comes before the commit (at least before >> the "real" commit on the trunk or whatever). Currently we all freely >> commit without first checking with somebody if we are allowed to or >> whatever. This also means that we currently usually don't branch for >> that reason, we commit directly on the main trunk. We do make branches >> of course, but for other reasons (to support older versions etc). >> >> I'm a bit afraid that this will have a huge impact on the way we >> currently work. Won't it slow us down, certainly because it is >> probably not necessary to have everything code reviewed? Could a code >> review process which reviews code after the commit (but before >> releasing to for example the testing team) also work? Or would this >> lead to a process which gets too informal? >> >> Jan >> >> On Feb 12, 11:56 am, Reinier Zwitserloot <[email protected]> wrote: >> >> >>> 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 -~----------~----~----~----~------~----~------~--~---
