NYCPHP Meetup

[Tom Melendez]

Hi Peter,

On Tue, Dec 29, 2009 at 2:30 PM, Peter Becker <peterbsemail at gmail.com> wrote:
> Looking to get some views (and best practices) on code reviews.  I used to
> work at IBM on their early version of Websphere (as UI designer, not coder)
> where our group had code reviews on a regular basis.  I'm now managing a
> small dev team working on a new web site using Zend PHP/MySql and am curious
> to know what current practices are for reviews in this sort of environment.
> On the one hand it seems like it'd be somewhat redundant since we are a
> small team and the left hand does know what the right hand is doing, but on
> the other, I could see it being very useful to ensure that everyone is on
> the straight and narrow to everyone's benefit (particularly as the team
> grows).  Any good references as a starting point for approaches?  Appreciate
> the direction -
>
>

Let me just start out by saying that I'm not a huge fan of the general
"process" of code reviews as I know them:

1. Gathering a bunch of folks into a room to talk about a piece of
code is a time-waster IMO
- Unless they are specifically working on that portion of the project
they are out of context from the get-go

2. You find something wrong, then what?  And what if it is not
"really" wrong (Jones uses a selection sort but sould have used an
insertion sort, and no one told him beforehand).  Do we hold up the
project until the engs figure out what is best, even though the code
"works"?  Good luck explaining that to mgmt.  And politically, it can
make Jones look bad unnecessarily, yet, as the new guy on the team, he
might not be familiar with the standards, practices, etc. of your
group.

I see the typical code-review process as reactive rather than
proactive.  Your time is better spent investing in a build process
that provides code coverage, PMD, checkstyle/codesniffer reporting and
of course insisting that developers need to write unit tests.  All of
this will give you and mgmt a better sense of where the code stands in
terms of quality and is all reusable.  In addition, daily, automated
performance testing will tell you if you're about to release something
that won't stand up to your traffic.

With that said, I do believe in code reviews for two scenarios:
1. Senior members should review commits of junior members.  We do this
as a one-on-one mentoring scenario.
2. Something that is broken in production and needs to be fixed right
now.  When things are broken, people tend to get worked-up.  Having
someone look over what you're about to check in at that point is a
good idea IMO.

Thanks,

Tom
http://www.liphp.org

> _______________________________________________
> New York PHP Users Group Community Talk Mailing List
> http://lists.nyphp.org/mailman/listinfo/talk
>
> http://www.nyphp.org/Show-Participation
>