7
\$\begingroup\$

I recently started to learn Python for the purpose of making my life as a network engineer easier when performing repetitive tasks.

The script below does parse a huge inventory.xml file I get from a reporting tool to find the ID of ports I want it to and then writes back to the XML file a structure representing a cable between the two network ports. I do convey the name of the ports I want to link in a basic CVS file with 2 columns.

<link>                                                                                                                                              
<portid1>ca1cfd39-cf8f-4d69-a52b-d06357da66b1</portid1>                                                                                             
<portid2>79d8c32a-8891-4bd4-a0a1-1a6e1b333a4b</portid2>                                                                                             
</link>

This is the code I tried to comment to the best of my ability:

#links.csv file shoud be made as follows : 
#port1,port2
#port1,port2
#   .
#   .
#   .
from lxml import etree
import csv
import fileinput

#preparing variables
result=[]
trigger=False
tree=etree.parse("inventory.xml")

#parsing the csv file containing the port names to link together
with open('links.csv', 'rb') as csvfile:
    fichier=csv.reader(csvfile)
    for row in fichier:
        name1=row[0] #name of the first port
        name2=row[1] 
        portid1=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
        portid2=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))
        portid1=portid1[2:-2] #output was surrounded by [""] so i cut the first two and last two
        portid2=portid2[2:-2]
        result.append(("<link>\n<portid1>{}</portid1>\n<portid2>{}</portid2>\n</link>\n").format(portid1, portid2)) #store data

result="".join(result)
#search the file until we find the <links> line, then we print the result
for line in fileinput.input('inventory.xml', inplace=1):
    if line.startswith('    <links>'):
        trigger=True
    else:
        if trigger:
            print result
        trigger=False
    if line.rstrip():
        print line, #wannabe pretty print

The goal of posting here is mainly to make my code as readable to others and as standard as possible, because I know the work I do will probably be read of tweaked by others in the future.

\$\endgroup\$
5
\$\begingroup\$

I don't know anything about phyton, but I will throw my 5 ct in anyway.

Adding horizontal spaces between assignments to variables/properties and vertical spacing between related parts will make your code more readable like so

for row in fichier:
        name1 = row[0] #name of the first port
        name2 = row[1] 

        portid1 = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
        portid2 = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))

        portid1 = portid1[2:-2] #output was surrounded by [""] so i cut the first two and last two
        portid2 = portid2[2:-2]

        result.append(("<link>\n<portid1>{}</portid1>\n<portid2>{}</portid2>\n</link>\n").format(portid1, portid2)) #store data

If you need a comment about a variable, then this variable is poorly named. So better name the variable in a meaningful name and remove the comment.

name1=row[0] #name of the first port
name2=row[1]  

vs.

first_port_name = row[0]
second_port_name = row[1]   

The same approach I would use for

portid1=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
portid2=str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))  

like so

first_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
second_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))

So the mentioned points together would look like so

for row in fichier:
        first_port_name = row[0]
        second_port_name = row[1] 

        first_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name1)) #searches for the port id in the .xml
        second_port_id = str(tree.xpath("//name[text()='%s']/parent::*/id/text()" %name2))

        first_port_id = first_port_id[2:-2] #output was surrounded by [""] so i cut the first two and last two
        second_port_id = second_port_id[2:-2]

        result.append(("<link>\n<portid1>{}</portid1>\n<portid2>{}</portid2>\n</link>\n").format(first_port_id , second_port_id )) #store data
\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer ! Being the "i understand it cause i wrote it myself" kind of guy, the naming convention you are proposing seems pretty good for future guys taking on the code. \$\endgroup\$ – RedPanda Jul 9 '15 at 9:51
  • 1
    \$\begingroup\$ @AlexandreGautier It's always easier to understand code that looks like you might expect. A surprise could break off a line of thought. \$\endgroup\$ – jacwah Jul 9 '15 at 10:00
4
\$\begingroup\$

Readability is of great importance in writing maintainable code. Follow common formatting conventions.

Put spaces around =. This will make the two sides of the assignment easier to distinguish, and the code will look less messy.

Break lines at 80 characters. This is the standard for most code and prevents editors from wrapping lines or me having to scroll horizontally to read.

Python has an official style guide – PEP 8. Try adhering to it.

If there are several <links> lines after each other in inventory.xml, the result is not printed until the end of the last <links> line. I'm not sure if this is intentional or not. If <links> is the last line of the file, the result is never printed (although if it would've, there would be no closing tag).

I recommend using lxml for the output too, this could simplify your code a bit. Take a look here for more info.

\$\endgroup\$
  • \$\begingroup\$ Will definitely take a look at the style standard and the lxml wat of output! thanks. As for the <links> line, it can't and will never be the last but the point you made is pretty good \$\endgroup\$ – RedPanda Jul 9 '15 at 9:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.