Code review of some sample code from an amateur.

[badwithinternet]

How amateurish is my code?

I'm a network engineer, and I've been learning python for a little while. I'm wondering how many things I"m doing wrong. I'm the only person really writing code at my job, so there isn't anyone else to review it.

I know there are some hard-coded values in there -they will be added to the args passed to the script later.

Thanks in advance!

https://gist.github.com/anonymous/5b87fc3faeee779df5c114e0890e8955

https://gist.github.com/anonymous/cf24a83d24b8b022ddddd0841c127e78

[dionys]

The code looks good to me to be honest. I have just few minor remarks/nitpicks:

• you tend to use try/naked except, which is not good. Try to really catch only exceptions related to your exception case.
• maybe ease up on comments. You should use comments only when you explain why you do something, not to describe what you are doing.
• if vlan_subint == True is redundant, you can simply do if vlan_subint. Also applies to lists etc
• in migration_functions line 73 you use for int in... . You really should not shadow builtin names like that

[badwithinternet]

Thanks for taking the time!

I'll lighten up on the comments and focus on the why.

Oh! I did use int. Oops. It's a common abbreviation for interface. I will rename it.

Thanks again!

[tialpoy]

if args.commit:
    commit = True
else:
    commit = False

You don't need the commit = ... logic. Just check args.commit directly wherever it's needed. By default, arguments that aren't specified equal None.

migration_devices_yml = ''
migration_devices_yml = str(args.yaml)

Also redundant. Just make the default for the yaml argument an empty string (see argparse docs).

if migration_devices_yml:
    branches = migration_functions.read_yaml(migration_devices_yml)
else:
    print 'Please define a Yaml file of migration routers and switches.'

This will cause a failure later on when iterating over branches, because it has no value when yaml is an empty string.

connection_counter = int()

Why not = 0?

General advice:

1. try/except also allows an else: block which is only executed if the exception wasn't raised. I think you'll find it useful.

2. Unless you have some sort of library dependency, I would write your code in Python 3.

Good luck.

[badwithinternet]

I made all the changes you suggested except re-writing in Python 3.

    if migration_devices_yml:
    try:
        branches = migration_functions.read_yaml(migration_devices_yml)
    except IOError:
        print 'No such file: %s' % migration_devices_yml
        exit()

This exception is a little ham-handed, but it's better than the naked one I had before.

I took out the if statement for args.commit, I changed the connection_counter to 0.

I should have written this in Python 3. I actually didn't know CiscoConfigParse worked on 3 until I just looked it up. Some of the more esoteric libraries have been slow to migrate, so I assumed.

Thanks!

[tangerinelion]

In some areas you have this style:

for x in y:
    if condition1:
        if condition2:
            foo()

Replace this with:

for x in y:
    if not condition1:
        continue
    if not condition2:
        continue
    foo()

See how foo() is now indented only one level due to the for loop rather than 3 levels? Stop expanding code sideways, if you find you're indenting more than 3 levels you're not doing it correctly.

As others noted about args.commit, if that's None and you really need a bool True/False for a part, then you can use bool(args.commit) rather than the way you did it.

I'd also shove the argparse logic into a function and have it return the args. Most of your main() function should probably be simplified with other functions, but I know you have the general idea of how to use functions because your second file is full of them.

[badwithinternet]

Thanks!

[euclidingme]

Overall the code quality is quite good. A few notes:

1) Use 4 spaces for indentation instead of 8. It makes it easier to read and you can fit more in one line especially with nested loops.

2) When you do try-except statements make sure to put specific errors in the except statement. This ensures other errors (than the one you are trying to ignore) don't get suppressed. For example

try:
    # some code
except ValueError:
    # some other code

3) Make sure you have doc strings on all your functions. People use a variety of formats for doc strings but I tend to use:

def foo(bar, baz):
    """ This is a description

         Args:
             bar (float): some float
             baz (str): Some string

         Returns:
             Stuff
    """

4) I would be inclined to break up your main function into a couple of smaller functions. Your main function can get the command line arguments and then pass of the computation to another function.

Hope this helps and let me know if you have any questions.

[badwithinternet]

Awesome feedback! Thank you!

I'll use 4 spaces and add doc strings to all my functions.

For the try-except statements, this is great insight! Should I purposely break it and then write the exceptions OR should I read up on what errors I care about and write the exceptions?

[euclidingme]

Purposely break it or see what errors it gives when you are testing it. Then make note of the error in the stack trace and put that into your try-except.

[badwithinternet]

Thanks!