[Tutor] Code Review?

[Peter Otten]

Brian Carpio wrote:

> Hi,
> 
> Hopefully I am allowed to ask this here. I am pretty new to python I've
> only been writing code for about 6 months now strictly for system
> administration purposes; however I have now decided to write something
> "real" that others might benefit from but I am looking for someone to take
> a look at my code and give me an idea on some areas where I need
> improvement. I certainly don't expect anyone to rewrite my code but if
> someone is willing to give it a quick look and tell me things like focus
> more on XX or you really need to learn YY because its obvious you don't
> have a clue (lol) those types of comments would be more then welcome.
> 
> I've ran my code through pylint and am getting a score of 7-9 with all of
> my scripts, but a program can only tell you so much having a real person
> point out some of my shortcomings would be amazing. Sometimes you don't
> know you don't know something until someone tells you "Hey you don't know
> XX or YY go learn it".
> 
> https://github.com/bcarpio/mongodb-enc

[I took a look at mongodb_node_classifier.py]

Random remarks:

- pylint probably "told" you: use self-explanatory names rather than i, d, 
or n.
- Split your code in smaller dedicated functions. This simplifies writing 
unittests, allows reuse and keeps control flow manageable as your script 
grows.
- Add a comment that explains what the script does; repeat for each 
function.
- Use main() to read the configuration/commandline and put the real meat 
into a dedicated function. 
- Prefer raising exceptions over sys.exit() in the code that does the real 
work (main() may convert these into sys.exit).

Here's a sketch for a revised main():

def main():
    node_name = sys.argv[1]
    filename = ...
    conn_spec = read_config(filename)
    connection = open_connection(con_spec)
    try:
        process(connection, node_name)
    except InheritanceError as err:
        sys.exit(str(err))

(You should choose a name that is more descriptive than process())

- Use 'expr', not 'expr == True' in boolean contexts
- Show that you know None is a singleton and prefer 'expr is None' over 
'expr == None'
- I found the while loop hard to understand. I have a hunch that it can be 
simplified.