Tech:Reviewing Code/Example3

From Cyclopath

Date: Thu, 04 Dec 2008 17:37:54 -0600
From: Reid Priedhorsky <reid@umn.edu>
To: xxx
Subject: feedback on yyy


Hi xxx,

The quality of this code is a substantial improvement over your last 
check-in that I reviewed. Well done. Bring me more!

Some specific thoughts, both positive and negative, are below. As 
always, I'm happy to elaborate on anything that's unclear.

There are a few cleanups which I'd like you to do, explained below. I 
don't think they will take you too long, so please get them done ASAP. 
Can you do it by next Tuesday so I can include them in that week's 
release? Create a bug for them, or not, as you prefer.

1. Callback function in GetGeocode might have been a little more 
descriptively named (maybe just to "callback").

2. Removing unused variable address_choose_popup -- good find.

3. WFS_GetGeocode architecture -- nice; well-executed classic pattern.

4. UI.goto_popup_open() -- could leave off comment, since it doesn't add 
much new info.

5. You've got a few classes with nonstandard names (Address_GoTo_Popup, 
RouteLoc, maybe others?). Would you rename these to fit our coding 
style? (e.g. Address_GoTo_Popup -> Address_Go_To_Popup or 
Address_Goto_Popup, as you prefer).

6. Style -- you're still turning in code with nonstandard spacing 
occasionally; see for example Route.as line 69 and several other places 
in this region. Would you please review the diff for r8879 carefully and 
  correct the style where necessary.

7. Address_Choose.on_input_change() -- logic much cleaner, nice. Also 
submit_maybe().

8. Address_Choose.submit_maybe() like -- comparing against a UI string 
is error-prone; what if e.g. the colon is removed? Consider other ways 
to approach this; in this case, since it appears the string is set 
wholly within this file, one way is to define a string constant, set it 
once, and use it in both places.

8a. For items like this, I can't tell from the code alone if you agree 
or realize it's a bit troublesome. Consider adding comments to mark code 
you're not entirely happy with (you'll see many places in the app 
already marked "FIXME" or "hack" or somesuch); this will avoid my 
complaints, but more substantively, it makes it more clear to future 
readers your confidence level in a particular piece of code.

9. RouteLoc -- yes, this approach is fine for merging two classes. 
You've basically defined an abstract class. In a better language, you'd 
use markers like "abstract" on the methods to indicate that they must be 
subclassed. In ActionScript, you have to fake it: would you add 
G.assert(false) to both of the methods in this class? Also, I think you 
can omit the constructor if it's empty.

10. Please remove the ellipsis from "Go To" when it appears (I don't 
think it adds anything, and it's not used anywhere else in the UI).

11. Address_GoTo_Popup.mxml:

     a. Imports should be alphabetical; please fix.

     b. Indentation of method args is unstylistic in a few places; 
please fix. For example:

              this.wfs_active_alert.init('Finding address',
                                'Finding address. Please wait.',
                                wfs_active_geocode, true);

should change to:

              this.wfs_active_alert.init('Finding address',
                                         'Finding address. Please wait.',
                                         wfs_active_geocode,
                                         true);

     c. Always put variable declarations at the beginning of methods, 
because that's where they go logically regardless.

     d. Let's sit down together and I'll show you the style updates I 
want; there's several subtle things that have reasons behind them, and 
I'd like to explain my reasoning. That will be easier in person.

12. Location.as:

     a. I wonder if the argument to the constructor should be an array 
of  objects, rather than an array of arrays. That seems more robust.

13. The warning about city/zip ignored for points should probably appear 
in the Go To box.

14. Please work with Jordan to make sure your Go To box integrates into 
his new nav widget.

15. The label and point blend into the other map stuff. I'm not sure 
exactly what to do about it. It could be as simple as putting that layer 
on top? I don't know. Would you seek advice and figure out what to do or 
perhaps table it. There may be a bug already. I don't think this needs 
fixing immediately.

Thanks,

Reid