In the first post I shared with you my code to calculate tunnel numbers in Cisco SD-WAN. I’m a beginner in Python so I thought it would be a great learning experience to have someone experienced in Python, such as Rodrigo, take a look at the code and come up with improvements. As I like to share knowledge, I’m taking this journey with you all. Let’s get started!
You may recall that I had a function to calculate the tunnel number. It looked like this:
def calculate_tunnel_number(interface_name:str) -> int: <SNIP> return total_score
Rodrigo’s comment was that the function name is excellent as it is clear what the function does. However, my return statement returns total_score which is not clear what it does. It would be better to return tunnel_number which is what the function is calculating.
The next comment is that when splitting things and it is known how many pieces you have, it is better to unpack them, that is, assign the unwanted piece to a throwaway variable rather than using indexing. My code looked something like this:
interface_number = split_interface(interface_name)[1]
It would be better to do something like this:
_, interface_number = split_interface(interface_name)[1]
The first variable, a underscore, is used for the part of the interface name that we don’t intend to do anything with.
The next thing is to avoid duplicated code inside of conditional statements. For example, this piece of code was part of both the code I had in my if and then the else statement:
interface_number_list_int = [int(number) for number in interface_number_list]
As this part needs to be done in both cases, it does not need to and shouldn’t be part of a conditional statement. This is also the Don’t Repeat Yourself (DRY) methodology. Furthermore, the list comprehension could have been written more efficiently since the intermediate list I had created is not really needed. This is a little beyond my current understanding of Python, though.
In my function, I had a docstring, that is, a multi-line comment that gives information on what the function does. This text is then displayed in a REPL or IDE. It looked like this:
"""Calculate the tunnel number in Cisco SD-WAN The number in the interface is multiplied by 1, 10, or 100 depending on position. If subinterface is used, subinterface number is multiplied by 1000"""
PEP 257 defines that multi-line docstrings should start with a line, then an empty line, and then the rest of the text. The docstring should probably look something like this:
"""Calculate tunnel number in Cisco SD-WAN Keyword arguments: interface_name -- the interface name of tunnel source """
Then there was a comment on this sequence of if statements:
# Depending on length of list, perform different calculations if len(interface_number_list_int) == 3: total_score += 1 * interface_number_list_int[2] + 10 * interface_number_list_int[1] + 100 * interface_number_list_int[0] elif len(interface_number_list) == 2: total_score += 1 * interface_number_list_int[1] + 10 * interface_number_list_int[0] else: total_score += 1 * interface_number_list_int[0]
Here I’m manually checking length of list and doing different calculations. What if the length was 4 or 5? It would be more efficient to use a loop here.
This is what the final code would look like based on the cleanup:
from netutils.interface import split_interface def calculate_tunnel_number(interface_name:str) -> int: """Calculate the tunnel number in Cisco SD-WAN from the interface name. The number in the interface is multiplied by 1, 10, or 100 depending on position. If subinterface is used, subinterface number is multiplied by 1000. """ # Keep track of total score for tunnel number tunnel_number = 0 # First get interface number by splitting the interface name _, interface_number = split_interface(interface_name) # Check if it is a subinterface by looking for a dot # If dot is found, take number after dot and multiply by 1000 if "." in interface_number: # Split string to get subinterface number and convert to integer interface_number, subinterface = interface_number.split(".") tunnel_number += int(subinterface) * 1000 # Convert list containing strings to integers using list comprehension over parts splitted on / interface_number_list_int = [int(number) for number in interface_number.split("/")] # Convert integers in the interface number to a base-10 integer for exp, digit in enumerate(reversed(interface_number_list_int)): tunnel_number += 10 ** exp * digit return tunnel_number tunnel_number = calculate_tunnel_number("GigabitEthernet0/0/1.404") print(tunnel_number)
As you can see, this definitely looks cleaner and more efficient. I hope this post has been informative and a big thank you to Rodrigo for helping me with the code!
Great pair of blogs. As a relative newcomer to python great to read your first blog and then read the follow up review from an experienced programmer.
Thanks, Mike! It’s great seeing how others approach the same problem. As I don’t know the language that well yet it’s usually a lot of steps to get to where I want to when I try to translate of what goes on in my head to the code. Stay tuned for part 3!
Very clean write-up! Glad I could be helpful 🙂