[Gdal-dev] GDAL Code Reviews - SoC and beyond

[Chris Howell]

Hi Frank,

It's probably a good idea once you initiate code reviews to keep them 
occurring regularly. I would suggest on every checkin. That's what we do 
in our office and it makes a huge difference on the quality of the code 
that is produced as well as a good learning opportunity for all.

Cheers
Chris

Tamas Szekeres wrote:
> Frank,
>
> Do you have some principles to follow in the course of the review from
> the aspect of the gdal project?
>
> Best regards,
>
> Tamas
>
>
>
> 2007/7/3, Frank Warmerdam <warmerdam at pobox.com>:
>> Folks,
>>
>> A decade ago when I worked in an office, we had developed a practice of
>> regular code reviews.  These were, I found, an excellent educational
>> tool for sharing best practice, and also helpful in identifying problems
>> in the reviewed code.
>>
>> As part of our commitment to our Summer of Code students, I'd like to
>> introduce this practice to GDAL.  While my initial aim is to give our
>> SoC students an opportunity to review other code, and to be reviewed,
>> I also am hopeful that we could continue the practice after this point.
>>
>> My concept of code reviews is that we would:
>>
>>   o Pick a particular module of code, hopefully around 1000 lines of
>>     code and not more than 2000.
>>
>>   o Get four reviewers to spend an hour each pre-reviewing the code, and
>>     making notes on points for discussion.
>>
>>   o Getting together in IRC, following along "line by line", likely 
>> using
>>     a line numbered view like:
>>      http://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_minixml.cpp
>>
>>   o Spend about 1 hour reviewing in IRC, lead by a lead reviewer who
>>     keeps things moving.
>>
>>   o The Reviewee would be responsible for recording action items.
>>
>>   o The Reviewee would be expected to act on the suggestions, making the
>>     updated version available (along with the action items) within 
>> one week
>>     of the review for offline followup by reviewers.
>>
>> One challenge for code reviews is getting reviewers willing to commit 
>> the
>> two hours needed for the process.  I would suggest that we have our paid
>> maintainer (Mateusz) as one reviewer.  That for the SoC reviews we 
>> include
>> at on other SoC student as a reviewer, and that the mentors have to 
>> review
>> at least their own mentee.  That should give us 3 of 4 reviewers.  We 
>> will
>> call for volunteers for the fourth spot.
>>
>> Any opinions on this idea?  Are there any SoC students who can't 
>> can't access
>> IRC?
>>
>> Are there any SoC students who will have code available for review 
>> reasonably
>> soon?    I would like to suggest that we pick some other piece of code
>> initially for review.  As I would like to lead the first review, it 
>> should be
>> someone elses code.  I'll ask around a bit for some recently 
>> contributed code
>> by another author which can be the reviewed module.
>>
>> Best regards,
>> -- 
>> ---------------------------------------+-------------------------------------- 
>>
>> I set the clouds in motion - turn up   | Frank Warmerdam, 
>> warmerdam at pobox.com
>> light and sound - activate the windows | http://pobox.com/~warmerdam
>> and watch the world go round - Rush    | President OSGeo, 
>> http://osgeo.org
>>
>> _______________________________________________
>> Gdal-dev mailing list
>> Gdal-dev at lists.maptools.org
>> http://lists.maptools.org/mailman/listinfo/gdal-dev
>>
> _______________________________________________
> Gdal-dev mailing list
> Gdal-dev at lists.maptools.org
> http://lists.maptools.org/mailman/listinfo/gdal-dev
>
>