Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link
added more code improvements + explanation
Source Link
Ben A
  • 10.8k
  • 5
  • 37
  • 102

I'm only going to be commenting on 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 a tab (44 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 "name " +f"name '"\"{}"'.format(cell_content)}\""
 else:
 s = 0
 string = "" 
 anycells = ["any","Any", "ANY"] # tests if the soure, destination or service has any ==> returns nothing
 if str(x[0]).lower() in== anycells"any":
 return " " 
 else:
 if len(x) == 1: # case where tethe cell has one line (or a word with no spaces)
 string = f"{cell_name + " " +} '"\"{}"'.format(case)}\""
 else: # case where multilines/mutli spaces
 while s < len(x):
 listelement = x[s]
 string += f"{cell_name + '}.' + {str(s + 1) + ' ' +} '"\"{}"'.format(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+= 1
if __name__ == '__main__':
 main()

I'm only going to be commenting on 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 a tab (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.

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 "name " + '"{}"'.format(cell_content)
 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 = cell_name + " " + '"{}"'.format(case)
 else: # case where multilines/mutli spaces
 while s < len(x):
 listelement = x[s]
 string += cell_name + '.' + str(s + 1) + ' ' + '"{}"'.format(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()

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()
deleted 1 character in body
Source Link
Ben A
  • 10.8k
  • 5
  • 37
  • 102
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 "name " + '"{}"'.format(cell_content)
 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 = cell_name + " " + '"{}"'.format(case)
 else: # case where multilines/mutli spaces
 while s < len(x):
 listelement = x[s]
 string += cell_name + '.' + str(s + 1) + ' ' + '"{}"'.format(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()
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 "name " + '"{}"'.format(cell_content)
 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 = cell_name + " " + '"{}"'.format(case)
 else: # case where multilines/mutli spaces
 while s < len(x):
 listelement = x[s]
 string += cell_name + '.' + str(s + 1) + ' ' + '"{}"'.format(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()
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 "name " + '"{}"'.format(cell_content)
 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 = cell_name + " " + '"{}"'.format(case)
 else: # case where multilines/mutli spaces
 while s < len(x):
 listelement = x[s]
 string += cell_name + '.' + str(s + 1) + ' ' + '"{}"'.format(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()
Source Link
Ben A
  • 10.8k
  • 5
  • 37
  • 102
Loading
lang-py

AltStyle によって変換されたページ (->オリジナル) /