'. '

APIReview

From APIDesign

(Difference between revisions)
Jump to: navigation, search
Current revision (13:31, 17 July 2016) (edit) (undo)
(What if it is Too Late?)
 
(10 intermediate revisions not shown.)
Line 3: Line 3:
== Pitfalls of API Reviews ==
== Pitfalls of API Reviews ==
-
There are two extreme position one should avoid when performing API or architecture review.
+
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.
There is too little to review or there is too much to review.
==== Show me the Code! ====
==== Show me the Code! ====
-
When proposing small change or enhancement, it is preferrable to do it with a patch to existing
+
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!''
codebase. As Linus Torwalds once put it: ''talk is cheap, show me the code!''
Line 18: Line 18:
documentation is [[good]] they know what is the purpose of the test.
documentation is [[good]] they know what is the purpose of the test.
-
Also creating such patch is usually easier than long propse sections describing of what
+
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
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.
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
In short, when proposing small, compatible additions to existing APIs, doing that
-
as a patch is the best style of review. [[NetBeans]] call that '''Fast Track''' [[netbeans:APIReview]].
+
as an [[API Patch]] is the best style of review. [[NetBeans]] call that [[netbeans:APIReviewSteps#Fast-Track_Review|Fast Track APIReview]].
==== Don't bother me with Code! ====
==== Don't bother me with Code! ====
Line 43: Line 43:
author happy.
author happy.
-
For these reasons the [[NetBeans]] recommends to start with [[UseCases]] and review them
+
For these reasons the [[NetBeans]] recommends to start with [[usecase]]s and review them
-
as soon as possible. Once there is an agreement on them and thus the raw direction for
+
as soon as possible. Once there is an agreement on the [[usecase]]s - e.g. the raw direction for
-
solving them is understood, it is much more likely the result that comes out of that
+
solving them is understood, it is much more likely the result that comes out of the full development
-
is going to be generaly acceptable.
+
is going to be generally acceptable.
==== One Day Preparation ====
==== One Day Preparation ====
Line 52: Line 52:
[[I]] usually try to spend about a day to prepare the review material. If [[I]]
[[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,
can finish the whole work, then it is eligible for ''fasttrack review'' - if not,
-
it is probably to do deeper/standard review and as such [[I]] take what [[I]] have:
+
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 and ask for
+
[[API]] methods, their documentation and tests showing the sample usage (but failing).
-
review of such concept. Then [[I]] feel [[I]] have something material, but not that
+
That is often the best way to
-
much, so the advices [[I]] can still be easily accumulated into the design.
+
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, maybe even a week, but
+
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 too much.
+
spending more than a week on a preparation of inception materials is a way too much.
== What if it is Too Late? ==
== What if it is Too Late? ==
Line 64: Line 65:
The question is what to do with reviews that qualify for a standard review and are
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?
way too big? [[I]] don't know for sure. Return back and start from scratch?
-
Probably useless. Cherry pick the [[good]] parts and approve them first? Yeah,
+
Probably useless, there is usually something good in such reviews.
-
that is probably what [[I]] try: otherwise [[I]] am afraid that by commenting on
+
 
-
details, the high level changes in architecture directions would be lost.
+
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 [[I|me]] luck.
Wish [[I|me]] luck.

Current revision

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.

Personal tools
buy