4
\$\begingroup\$

I started to learn Python not long ago to automate certain tasks I work on. I want to optimize a script I wrote that takes a line in an Excel file and prints a pattern of text based on this line's content. Here's an example of output from my code:

mgmt_cli add access-rule layer "network" position bottom name "thirdline" source.1 "Value1front" source.2 "Value2front" source.3 "x3front" destination "Dest1front" service "HTTP" track.type "Log" action "Accept"

How would you optimize this code?

Link to the example file (just click Valider et Télécharger le Fichier) http://dl.free.fr/hFlGmywIN
Link to full code : https://pastebin.com/CR5SXHRQ

import xlrd
import re
loc = ("Dir\StackoverflowExample.xlsx")

wb = xlrd.open_workbook(loc) 
sheet = wb.sheet_by_index(0) 
sheet.cell_value(0, 0) 

# Extracting number of rows 

def CaseMulti(CellContent, CellName):
 #takes a cell and a string as parameters, prints a pattern like Cell.X ="string", X starts with 0 and ends at the number of line in the Cell
 case = str(CellContent)  
 x= re.split ("\s", case)   #splits the cell contents in a list using regex where space is the separator for the list's elements
 if name == CellContent:  # deals with the cell with spaces between objects no clue how it work but it does
  return "name " + '"{}"'.format(CellContent)
 else:
  s=0
  string =""
  anycells = ["any","Any", "ANY"]  # tests if the soure, destination or service has any ==> returns nothing
  if str(x[0]) in anycells :
   return " " 
  else:
   if len(x)==1:    # case where te cell has one line (or a word with no spaces)
    string= CellName + " " +'"{}"'.format(case)

   else:            # case where multilines/mutli spaces
    while s<len (x) :
     listelement = x[s]
     string = string + CellName+'.'+str(s+1)+ ' '  +'"{}"'.format(listelement)
     s=s+1

   return string

for numberofrows in range (5):

 print "Rule " + str (numberofrows)
 name= sheet.cell_value(numberofrows,1)
 source = sheet.cell_value(numberofrows,2)
 destination = sheet.cell_value(numberofrows,3)
 protocole = sheet.cell_value(numberofrows,4)
 log = sheet.cell_value(numberofrows,5)
 action = sheet.cell_value(numberofrows,6)
 #firewall = sheet.cell_value(numberofrows,7)

 try:
  print ("mgmt_cli add access-rule layer \"network\" position bottom " + str(CaseMulti(name,"name")) +  str(CaseMulti(source, " source")) + str(CaseMulti(destination, " destination")) +  str(CaseMulti(protocole, " service")) +   ' track.type "Log" ' + str(CaseMulti(action, " action")) )
 except :
  numberofrows +=1
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

STYLING

According to PEP-8, there are a few improvements to be made to your code:

  • Imports: Your imports should go in alphabetical order
  • Variable/Function Names: You should use snake_case instead of camelCase
  • Parameter Names: This may just be how I view your code, but how your parameters are named look like your taking in objects. You should use lowercase variables for better readability.
  • Spacing: You should use 4 spaces for each indent. For variable declaration and assignment, the = should be separated by a space on each side for readability, like so: s=0 to s = 0.
  • Main Guard: You should wrap any regular code in a if 'name == 'main' guard, to make sure that the code is only being run when that file is the main file, and not being imported by anything.

STRING FORMATTING

All throughout your code, you put strings together using + and .format. Luckily there is an easier way, by putting an f before the string like so: f"content here". This erases the need to have verbose + in your strings, and you no longer have to cast int to str with this method, you can just write the code in the brackets {your code here}.

Example:

return "name " + '"{}"'.format(cell_content)

to

return f"name \"{cell_content}\""

Here is the updated code:

import re
import xlrd

loc = ("Dir\StackoverflowExample.xlsx")

wb = xlrd.open_workbook(loc) 
sheet = wb.sheet_by_index(0) 
sheet.cell_value(0, 0) 

# Extracting number of rows 

def case_multi(cell_content, cell_name):
    #takes a cell and a string as parameters, prints a pattern like Cell.X ="string", X starts with 0 and ends at the number of line in the Cell
    case = str(cell_content)  
    x = re.split ("\s", case)   #splits the cell contents in a list using regex where space is the separator for the list's elements
    if name == cell_content:  # deals with the cell with spaces between objects no clue how it work but it does
        return f"name \"{cell_content}\""
    else:
        s = 0
        string = "" 
    if str(x[0]).lower() == "any":
        return " " 
    else:
        if len(x) == 1: # case where the cell has one line (or a word with no spaces)
            string = f"{cell_name} \"{case}\""
        else: # case where multilines/mutli spaces
            while s < len(x):
                listelement = x[s]
                string += f"{cell_name}.{str(s + 1)} \"{listelement}\""
                s += 1

        return string

def main():

    for number_of_rows in range(5):

        print "Rule " + str(number_of_rows)
        name= sheet.cell_value(number_of_rows, 1)
        source = sheet.cell_value(number_of_rows, 2)
        destination = sheet.cell_value(number_of_rows, 3)
        protocole = sheet.cell_value(number_of_rows, 4)
        log = sheet.cell_value(number_of_rows, 5)
        action = sheet.cell_value(number_of_rows, 6)
        #firewall = sheet.cell_value(number_of_rows, 7)

        try:
            print ("mgmt_cli add access-rule layer \"network\" position bottom " + str(case_multi(name,"name")) +  str(case_multi(source, " source")) + str(case_multi(destination, " destination")) +  str(case_multi(protocole, " service")) +   ' track.type "Log" ' + str(case_multi(action, " action")) )
        except:
            number_of_rows += 1

if __name__ == '__main__':
    main()
\$\endgroup\$
3
  • \$\begingroup\$ To be clear, PEP-8 specifies that indentation should be 4 spaces per level, not a TAB character. This is an important convention to follow, since indentation matters in Python. \$\endgroup\$ Commented Jul 5, 2019 at 3:26
  • \$\begingroup\$ @200_success Is a tab not already 4 spaces? I already had the clarification next to it I just assumed they were the same length \$\endgroup\$
    – Linny
    Commented Jul 5, 2019 at 3:27
  • \$\begingroup\$ I would avoid any mention of "tab", since that just adds a possible misinterpretation: an ASCII character 9, and configuring your text editor or terminal to use 4-character-wide tabstops. \$\endgroup\$ Commented Jul 5, 2019 at 3:31
-1
\$\begingroup\$

It seems a main thing you need to learn about is f-strings

These are an update to the old format method.

print "Rule " + str (numberofrows)

becomes

print(f"Rule str(numberofrows)")

and

"name " + '"{}"'.format(CellContent)

becomes

f"name {CellContent}"

NOTE Python 3 uses () for print statements

It is also worth using a PEP8 checker (such as black - pip install black) to make your code more readable


You could also change things like

s=s+1

to

s += 1

Rather than saying

anycells = ["any","Any", "ANY"]  
if str(x[0]) in anycells
...

You could say

if str(x[0]).lower() == "any"

Are you sure this is your code? This looks very much like Python 2 code which hasn't been used for over 6 years. I am curious why you would write something in Python 2 still? It seems odd to learn Python 2 as a new beginner

\$\endgroup\$
1
  • \$\begingroup\$ thank you for your input. Yes this is my real code and it's in python 2, last time I coded was in Turbo Pascal that's why it looks very old \$\endgroup\$
    – pythonoob
    Commented Jul 4, 2019 at 22:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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