APIReview
From APIDesign
API Review is an essential element of Teamwork for everyone designing APIs in a group. The NetBeans project uses process described at netbeans:APIReviews. Now, when I am working on Truffle API, I need to transfer that knowledge to new team.
Contents |
Pitfalls of API Reviews
There are two extreme positions one should avoid when performing API or architecture review. There is too little to review or there is too much to review.
Show me the Code!
When proposing small change or enhancement, it is preferrable to do it with an API Patch to existing codebase. As Linus Torwalds once put it: talk is cheap, show me the code!
Often, rather than describing how you want to add one method into an existing class, it is easier to prepare the patch. Such patch should include documentation and a test to show what is the new capability of the API that wasn't available before.
Evaluating such patch is usually straightforward, reviewers know what to focus on and, if the documentation is good they know what is the purpose of the test.
Also creating such patch is usually easier than long prose sections describing of what could be done without showing how. At the end most of the prose sections should become part of the Javadoc documentation - thus one can include them in the patch as well.
In short, when proposing small, compatible additions to existing APIs, doing that as an API Patch is the best style of review. NetBeans call that Fast Track APIReview.
Don't bother me with Code!
On the other hand, some API changes can become huge. If it takes more than a day-week to code their idea, introduce completely new subsystem, change the way things use to work, it is good practise to do two rounds of view. NetBeans calls the process Standard review and it is composed of inception and final review rounds.
The purpose of APIReview/architecture review is to review the architecture, not the actual code. However if the whole code has already been written, how can the reviewer influence the overall architecture? By commenting about wrong indentation, suggesting better variable names? Well, that isn't an architecture!
The danger of asking for review late is something called emmotional attachment to the code. The author of the code have had spent enormous amount of time preparing it and probably put a lot of effort into its design. Any significant change in architecture basically says that the effort wasn't successful. That isn't going to make the author happy.
For these reasons the NetBeans recommends to start with usecases and review them as soon as possible. Once there is an agreement on the usecases - e.g. the raw direction for solving them is understood, it is much more likely the result that comes out of the full development is going to be generally acceptable.
One Day Preparation
I usually try to spend about a day to prepare the review material. If I can finish the whole work, then it is eligible for fasttrack review - if not, it is probably better to perform deeper/standard review. Which is fine as I usually only have: API methods, their documentation and tests showing the sample usage (but failing). That is often the best way to review of the concepts. There is some material, but not too much, so the advices from the APIReview can still be easily accumulated into the design.
Sometimes it make take longer to prepare the inception material. Maybe even a week, but spending more than a week on a preparation of inception materials is a way too much.
What if it is Too Late?
The question is what to do with reviews that qualify for a standard review and are way too big? I don't know for sure. Return back and start from scratch? Probably useless, there is usually something good in such reviews.
Cherry pick the good parts and approve them first? Yeah, that is probably what I'll try: otherwise I am afraid that by commenting on details of completely mis-designed subsystems, there is a high chance that the needed architecture changes would be lost in discussion about naming and code formatting.
Wish me luck.
