[Ns-developers] code reviews: a short guide

[Tommaso Pecorella]

Hi,

sorry to contradict Mathieu, but this isn't a rant. Maybe it started as one, and I also know who he had in mind when he wrote it. But reading it it doesn't seems a rant at all. It's more a "know your reviewer and learn to live with him - howto".

Now, I can't agree more with what he wrote. Sadly enough I know by personal experience what does it mean to be on the two sides of the table (the examiner and the student), so I know what's in each mind. Well, I pretend to know at least.

Point however is: this isn't a rant. It's an howto, and he implicitly stated that in the subject. Post it in the wiki! I guess noone would mind about having it there, and it might also be useful for the future contributors.

On the other hand, I see in what he wrote the hidden problems of reviews, and tbh we should streamline a bit the review process in order to not have misunderstandings about some major points.

A code review in my opinion can be split into 3 different parts:
1) low level: coding style.
2) middle level: coding issues
3) high level: code design.

Part 1) can be easily solved by a generic statement: "follow the friggin' coding style conventions or I'm gonna kick you". There's no need to loose time by checking code lines one by one. Moreover we have a fantastic perl script to do that. It could miss some small bits but in my personal opinion there's no need to loose too much time on it.
Hence, if a reviewer spots a coding style violation, he/she should not even bother pointing out where is the violation. The contributor will learn to be more careful next time.
BTW, about this: we really should pass the whole ns-3 repository trough that perl script. Enforcing coding style works if there's no violation in the already-existing code. Otherwise it's an empty statement. How can we reply to a contributor stating "I'll not follow conventions since the code base isn't following them either" ?

Part 2) that requires some time, and usually include bad coding, use of wrong constructs (like using a bad container) and so on. That is a time-consuming task and imho it's the most important part a reviewer should focus on. Bad coding means bad code maintenance.

Part 3) this is the trickiest part, as it involves design choices. We can't enforce design choices too much, beside the most obvious one, like "no multiple inheritance". The rest of the design is a matter of choice. Point is: a design is not easily "seen" from the raw code, and that's why some people invented UML.

I'm not saying to use UML in ns-3, but maybe we should ask for a design document for some contributed code. That should ease the reviewer process and can focus the review on the design rather than the code. When the design is clear, the code will follow. Some could object that the design is in the APIs, but I strongly disagree. The APIs are a consequence of the design, they tell you what the code can do, not why the coder thinks they could do and why that's important.

We have to face the fact that right now ns-3 have mostly well-known protocols implementations, or stuff that we all know very well (e.g., random number generators). There was no need for a design rationale behind those. If I decide to add a new protocol to ns-3 (e.g., DVB-RCS) I don't need to explain why an API is there. Is there because the standard is telling so. However for some other code we need a design rationale, so the reviewer can understand what's the problem the code is addressing, why the implementer decided to do something, etc.

Discussing about design is important as much as discussing about good coding, but it's a higher level discussion and it's kinda different. If you concentrate on the APIs you'll probably miss the points and both the reviewers and the contributors will end up in frustration.

So, beside asking Mathieu to post his "guide" to the wiki, I'd like to propose to ask the contributors to add a rationale document in the patch. The rationale doc could or could not be stripped from the actual ns.-3 distribution. FlowMonitor have one for example (well, kinda).

The doc could or could not have a predefined format, but what I'd like to see in it is:
1) Rationale
- what is the issue this code is trying to solve
- what are the major design choices (if any)

2) Use case
- what the user is expected to do with the code

3) Potential issues
- what is the plan to expand the code to support more scenarios and how that could affect the existing code design

And that (at the end) leads me to asking Mathieu to add another "suggestion" to his guide.

- never try to address future, unknown or potential user's need. Focus on right now. If you don't have a clear use case, then you're either doing something wrong or you're using in a bad way your time and the reviewer's time. Or both.

I did that horrible mistake once and Mathieu's comments forced me to think twice to what I was doing. However we both used our time in a bad way. We both was discussing about APIs at that time, if we'd was discussing about design all would had been shorter and simpler.
In that case Mathieu wasn't happy about the APIs, but he wasn't happy about the design behind those APIs for real, and he was damn right. In all honesty I have to thank him for pushing me to the right track.


Anyway, at the end we always learn something new, and that's the good part. Discussions can be harsh or unpleasant, but if at the end of the day you've got something new good, it's never wasted time.


Best regards,

Tommaso


On 10/mar/2010, at 10.05, Mathieu Lacage wrote:

> hi,
> 
> A very personal code review rant. I could post that to a blog if I had
> one but I don't so, here it is for everyone's reading pleasure: the next
> time I have to start a review for a new contributor, I will include a
> link to the archived version of that email in my comments :)
> 
> First, some obvious things:
> 
> - produce minimal patches: a minimal patch is a patch which does not
> include any extra un-needed changes to get the feature you want/need in.
> Minimal patches are important because they make it easier for the
> reviewer to focus on what really matters and to avoid being distracted.
> 
> - produce self-contained patches: all the information needed to perform
> the review should be included in the patch. Of course, this means that
> the patch, once applied, should produce a buildable tree but this
> includes example code, explanations about how the code is expected to
> work, how it's expected to be used, etc. This is also important to allow
> the reviewer to focus on what really matters.
> 
> - make sure that the new code is clean from a coding style perspective:
> use our coding style script.
> 
> Now, a couple of things you need to understand before you start the
> review process:
> 
> - there are very few reviewers: few people care enough about helping
> others merge their code to invest energy in reviewing the code of
> others. So, reviewer time is usually a resource more scarce than
> contributor time. This is also why reviewers like simple code with
> little functionality which is neither optimized nor smart: it takes less
> time to review simple code than to review fancy complex code.
> 
> - reviewers are stupid: they don't know what you want to do. They don't
> understand it. If they do understand what you want to do, they are going
> to forget about it soon so, your job is to make these stupid people
> understand all the fancy things you came up with and that is going to
> take quite a bit of your energy.
> 
> - a review for a new feature can take many iterations to converge to a
> version of the code which is merged. 5, 8 iterations is not un-heard of,
> especially, for large, new features. This is why it's best to try to
> split your large new feature in lots of small independent
> patches/features because the number of iterations for each small feature
> will be smaller.
> 
> - it's important to clearly address each comment made by a reviewer:
> either state why you think you can do something differently or make the
> suggested change and post a new patch only once you have addressed all
> comments (exceptions to this rule can of course be made if addressing
> all the comments takes you more than 1 week, then, it's probably best to
> submit an unfinished patch if you state it clearly).
> 
> - I know that addressing all the comments of a code review can take a
> lot of unplanned energy and thus lead to a lot of frustration,
> especially when the number of review iterations grows. It is important
> to understand that doing a code review takes a lot of time for code
> reviewers too so, it's unpleasant for everyone.
> 
> - pushing hard for a merge deadline is best avoided: it's better to
> simply work on the code until everyone thinks it's ready or until
> everyone thinks it can't be improved much more. Of course, it's not
> always possible to do this in which case, it's best to start the review
> as soon as possible, possibly weeks before the deadline.
> 
> Finally, a couple of things I usually look at when doing a review:
> 
> - I always start with API and, then, move on to the implementation
> itself. Sometimes, when the code is big or I don't understand the
> implementation, I also look at the implementation early to try to
> understand how all the pieces are supposed to hold together.
> 
> - If I don't understand an API, I try to go back to the user-visible
> usecases so, the review might start with questions about the usecases
> you are trying to provide code for to better understand your problem
> space.
> 
> - I can be convinced to not care too much (sometimes, not at all) about
> implementation when the API looks right and there is ample automatic
> test coverage. Sometimes, making the API look right is sufficient to
> convince me.
> 
> 
> Mathieu
> 

--------------------------------------------------------------

The nice thing about standards is that there are so many to choose from.
And if you really don't like all the standards you just have to wait another year until the one arises you are looking for.
-- A. Tanenbaum, "Introduction to Computer Networks"

--------------------------------------------------------------

Tommaso Pecorella - Ph.D.

Assistant professor
Dpt. Elettronica e Telecomunicazioni
Università di Firenze

CNIT - Università di Firenze Unit

via di S. Marta 3
50139, Firenze
ITALY

email: tommaso.pecorella at unifi.it
       tommaso.pecorella at cnit.it

phone : +39-055-4796412
mobile: +39-320-4379803
fax   : +39-055-494569