Weighing in on code reviews are all the rage.
Along the lines of getting a group to start having code reviews... when I hear that code reviews are too time consuming my reply is, "Hogwash! I can do a code review in 10 seconds without even scrolling down the source file."
Just look at the import list in this worst case example:
import com.mycompany.services.UserService;
import com.mycompany.services.UserServiceImpl;
import javax.persistence.EntityManager;
import javax.swing.JFrame;
import com.thirdcompany.COMBridge;
import javax.swing.JLabel;
Separation of Concerns - The presence of EntityManager (persistence) and a JFrame (user interface) means that code working with the database is intermingled with user interface display. Reject the code on grounds of poor separation of concerns.
Programming to Interfaces - The COMBridge import from a 3rd party package indicates that risk-laden dependencies have not been abstracted out of the core business logic. Reject the code on grounds of no programming to interfaces.
Broken Windows - Imagine that the JLabel import is gray in the IDE because it is unused. Code should compile with no warnings, and all warnings should be cleaned before the code review begins. Reject the code on grounds of sloppiness.
OK, so this is a little facetious. I don't really do code reviews in 10 seconds. The point is that there's a wealth of information available with little effort on the reader's part. Code reviews are not a big commitment. Hopefully, you'll have more than 10 seconds budgeted for this. Maybe you have as many as 30 seconds! In that case all you'll need to do is look at the type signatures within the class.Easy.
10 comments:
I love this :D
Just by glancing at your article I can see that I agree 100%. It's a thought I've had many times.
Perfect :-)
All of this kind of stuff is precisely what tools like FindBugs/CheckStyle exist for. Human beings shouldn't be wasting their time on this kind of mechanical code processing.
BTW, this kind of stuff is also why OSGi's modules are useful: by exposing an interface without having to expose its implementation, you can enforce a separation of layers.
@RobertFischer - Human beings shouldn't be wasting /my/ time with this type of stuff. Agree on both points as usual. Sadly, we're unable to use either of these two tools on our codebase right now.
Woot, great post! Incredible what those few lines, which are typically folded away, can reveal.
Aprpos, import statements are an indicator of bugs. There is research, I guess by Andreas Zeller and Thomas Zimmermann, that import statements are one of the maybe strongest indicators for past, present and future bugs. If you import the same packages as a buggy class, your class is going to be buggy as well.
I'd be interested to hear what other types of information can be gleaned from import statements. It's like a fun game of forensics. AND we could submit our findings back to the CodeNarc project as future static analysis rules.
ROFL - SPOT ON! :)
I agree code review is an easy job. From a general point of view it's easy to point out flaws in a design than doing it right... On the same page, a common WTF is the company paying big bucks for a security audit on an app designed by an all-junior (i.e. cheap) team.
Post a Comment