Tech:Reviewing Code/Example1

From Cyclopath

Date: Tue, 19 Aug 2008 14:52:11 -0500
From: Reid Priedhorsky <reid@umn.edu>
To: xxx
Subject: feedback on changes


Hi xxx,

Here's some feedback on your latest changes (through r8209):

What I liked:

1. Good progress. You've efficiently completed a number of key tasks.

2. I was quite impressed by the special case for node id finding you 
came up with the other day. A key skill for a programmer is to know 
which esoteric problems may arise, so they can be worked around.

3. You anticipated and solved the the re-geocoding problem before I 
could complain about it.

4. The overall architecture for achieving #3 was sound.

5. In particularly, I liked how you moved the Route-specific hack from 
WFS_GetGeocode.as to Route.as.

Possibilities for improvement:

1. Use util.xa_set() to set XML attributes in Python. It has some extra 
smarts to do the right thing with certain data types.

2. There was quite a lot of redundant, and apparently copy-n-pasted, 
code. This is the most important problem with this commit; programs 
should generally contain very little redundant code, and certainly not 
in the volume found in this commit. Some examples:
    - e.g.: SOAP namespace repeated a dozen times
    - e.g.: re-geocoding with a lower threshold repeated earlier code
    - e.g.: excessive re-conversion of result count to int
    - e.g.: Route.wfs_getgeocode_on_finish() had awkward logic leading 
to much redundant code
    - e.g.: ditto for Address_Choose.submit_maybe()
    - e.g.: the latter two methods shared lots of identical code

3. I removed the logic to re-geocode with lower threshold. I'm not 
convinced it's needed, particularly before beta, and it also required a 
rewrite due to its being copy-paste code.

4. Obsolete error check/message not removed: "Can't parse address. Did 
you forget to give a city name or put a comma before it?"

5. Order of Python imports sometimes did not follow style guide.

6. The Microsoft username/password should have been placed in CONFIG 
rather than buried inside the code.

7. Don't use standard functions as variable names (len, zip).

8. Don't leave debugging code laying around (printing XML results). In 
key places it may be useful to leave it in and commented, and perhaps 
even active. Consider carefully whether the debugging code is useful to 
keep around before leaving it.

9. I found a for loop which was written quite awkwardly. Here it is:

      for result in results:
          print result['version']
          point = result['astext']
          (x, y) = util.wkt_point_to_xy(point)
          if (len(from_list) < 5):
             from_list.append(...)

    The reason it is awkward is that if there are more than 5 hits, the 
loop still runs to completion but does nothing after the first hit. This 
is bizarre. Some better possibilities:

    a. put a LIMIT clause in the SQL (elegant, but you can't tell how 
many hits there were)

    b. check result count & complain (this is what I did, with a 
generous LIMIT to prevent loading the whole database)

    c. use Python list slicing:

       for result in results[:5]:
          ...

    d. manually stop the loop when the limit is reached:

       for result in results:
          ...
          if (len(from_list) > 5):
             break

10. You're still sometimes using spacing contrary to style guide: e.g.,

        if(from_req && to_req){
	  ...
        }

     rather than

        if (from_req && to_req) {
           ...
        }

I would like you to review the relevant code subsequent to my changes. 
If things don't make sense, please ask and I will be happy to explain.

Also, please make sure that your node_id work in r8242 still works after 
merging from the trunk; if not, please fix. Let me know when I can merge 
that.

Lastly, please let me know if you don't understand something on this or 
previous feedback. I'd be more than happy to go over it with you.

Thanks for all your hard work!

Reid