x1
cs682, Spring 2009 - code review
CS682 - code review
Jean-do Sifantus talk March 4.
From
Effective Code Reviews Without the Pain
By Robert Bogue
Code reviews in most organizations are a painful experience for
everyone involved. The developer often feels like it's a bashing
session designed to beat out their will. The development leads are
often confused as to what is important to point out and what
isn't. And other developers that may be involved often use this as a
chance to show how much better they can be by pointing out possible
issues in someone else's code.
Code reviews, however, don't have to be painful.
We will point Jean-do to two files from each team.
Here are two files of mine. The first is a Visual
Basic Excel macro that models the mortality of birds flying through a
windfarm. It works, but it hasn't been extensively reviewed or
refactored.
The second is an example I've used in a one week Java for Programmer's
course offered occasionally by SERL. It's been reworked and shaped often.
Here are my comments on two source files from each project, as of
April 29. You can
tell by how nit-picky the comments are that on the whole I was very
pleased with the readability and the style.
If you look at these links in the future you may well find problems
fixed - or moved, since line numbers might change.
From TBS
-
StudentController.java
- No license information, no header comments.
-
imports well managed (no whatever.* )
- MVC architecture very clear (this class name, and lines 44, 45)
- I'd have put debugging code (lines 51-72) deep in the file, in a
separate debugging section.
- (trivial) prettyprinting error at line 69
- F1 key event seems to be commented out on lines 95-103 (but you
wouldn't know that if you saw only lines 94-102). And it's still
documented in the javadoc (which is otherwise excellent).
- Lines 180ff:
if else if ... nested pretty
deep. I can see that the function is to set the
value of c, and then use the value in line 204. Is there
a way to rewrite this, with premature return statements,
or as a dispatch table? Maybe not, maybe not worth the effort.
- Prettyprinting errors line 524, lines 321 vs 373.
-
Graph.java
- No license information. I thought that was automatic.
- Line 3 I agree. I've stumbled over that before. It's always a
good idea to import conservatively.
- State variables for scoring seem to be interspersed with those
for the graph itself. Same is true for scoring methods. Should the
scoring code be somewhere else? Subclass? Decorator?
-
Line 576: Why does method
maxOrgPathLength() return
minOrgPath ? Is this an error, or just meant to confuse
the reader?
- Line 784: How soon is soon?
From See3PO
-
Why is there a dll in
Pathfinding ?
-
ImageToArray.cs
-
No license information (and lisence (sic) is misspelled in svn
properties list).
-
The
m_ prefix is a well known (Microsoft) convention. Do
you honor it everywhere? Do you say so anywhere?
- Variable
num_width declared several times (lines
22, 48)
- No
/// comments
- Our
main host program.
- Lines 64-66. Blank lines needed? Align equal signs?
- Line 74
Drive method. Seems to drive just
forward (line 83). Can the Queue of
MoveCommands actually mix directions? Only the forwards seem to
have any meaning.
- Line 103. Switch statement with just one case and a default?
From VisitME
- index.php
- File should know its own name (in this case,
index.php.
- It looks as if the values for the variable arguments on lines
36-38 come from the required
common.php . It'd be nice
to say so, for newbies like me.
- Lines 45-49. Is there any chance that this loop is infinite? What
if facebook is uncooperative? Same at lines 80-83.
- Comments at lines 97-102 should be visible on all lines.
- Lines 226-227. Why does code inside
if ($debug)
need to be commented out?
-
else logic on lines 290-293 is easy to lose after
long if block. Reverse the logic.
-
Nice prettyprinting seems to stop at line 297.
- canvas.tpl
- What does this file do? (.tpl extension is new to me)
- Variables seem to be supported (
$host_url on line
33) so I think they could (and thus should) be used for the constants
specifying various widths and heights.
- Internationalizing this code/page would be difficult.
- Line 159. Should "charged" be "charge"? If this wording comes
from Kayak then acknowledge source so it can be checked periodically
(by hand) for updating. Or get it at runtime.
Links
Back to the CS682 home page.