Python Forum
Thread Rating:
  • 1 Vote(s) - 2 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Code Review?
#1
I'm back.  I have my first  _working_ python script! Woot!  It takes a Cisco ASA config, and converts certain aspects (network objects, services objects, access-list)(the ones I need)  to a Fortinet stanza. (Vendor A to Vendor B).

So it works.  But in the interest of being a n00b, I'd like feedback & constructive criticism.  (You can't hurt my feelings, the ex-wife got those along with the house).

It's ~650 lines.  Post it here in the forum?  provide a google drive link?  What's proper forum etiquette? 

I was looking at other peoples code and snippets, and just a bit of self critique:::

1) I don't have a _main_... but it works, so do I _need_ one, or is it simply best_practice to have one?  
-- yea, but it works, so obviously not REQUIRED

2) I couldn't get   (var_1, var_2 +=1 ) so I increment each on it's own line
-- wasn't sure what to google exactly for what I was trying to do

3) I create an object of an existing object simply to make it more readable 
-- having too long a name or non-intuitive name made it confusing to debug
--here's what I did ::: acl_line = original_lines[counter].split()  & used acl_line rather than original_lines
 
4) Variables -- I saw a code snippet saying certain things should be set to Global.
-- really not sure why or how some objects are known in a function, while only some are passed
-- RTFM time?

5) I have more If/elif/elif/else statements than Canada has Tim Horton's. 
-- it's okay because it's a parser and you have to do all those checks... or hey idiot, there's a better way called XYZ
-- I couldn't find a "CASE" statement in python, is it called something else?

6) I wrote a line de-duplicator - is there a better way 
-- actually thought i handled it pretty well....wait, what are you doing with that pin ??? <POP!!>

So, post code or no?

Thanks, 
PappaBear

Code now at::: https://github.com/PappaBearTroy/FWConverter
Reply
#2
A few random things addressed:

Github is a good idea, especially for so much code, and it's probably a good idea to post it on the site in code tags as well (for the lazy among us).
A clear entry point, using __main__ or not, helps readability.
If you're using the global keyword, you'll 99.9% be told you shouldn't (and hopefully what to do instead).
Long if/elif/else stuff can sometimes be simplified to a dictionary. You'll surely get more comments once you post the code. Python does not have switch/case/match as some other languages do.
Reply
#3
Interesting...Dictionaries... Did not know, but may be very useful as the app grows!

Thanks!

Wow, not sure I used GitHub correctly... another thing to learn...

But I believe you can now see the code at ::: https://github.com/PappaBearTroy/FWConverter
Reply
#4
I'm back.  I have my first  _working_ python script! Woot!  It takes a Cisco ASA config, and converts certain aspects (network objects, services objects, access-list)(the ones I need)  to a Fortinet stanza. (Vendor A to Vendor B).

So it works.  But in the interest of being a n00b, I'd like feedback & constructive criticism.  (You can't hurt my feelings, the ex-wife got those along with the house).

I was looking at other peoples code and snippets, and just a bit of self critique:::

1) I don't have a _main_... but it works, so do I _need_ one, or is it simply best_practice to have one?  
-- yea, but it works, so obviously not REQUIRED

2) I couldn't get   (var_1, var_2 +=1 ) so I increment each on it's own line
-- wasn't sure what to google exactly for what I was trying to do

3) I create an object of an existing object simply to make it more readable 
-- having too long a name or non-intuitive name made it confusing to debug
--here's what I did ::: acl_line = original_lines[counter].split()  & used acl_line rather than original_lines
 
4) Variables -- I saw a code snippet saying certain things should be set to Global.
-- really not sure why or how some objects are known in a function, while only some are passed
-- RTFM time?

5) I have more If/elif/elif/else statements than Canada has Tim Horton's. 
-- it's okay because it's a parser and you have to do all those checks... or hey idiot, there's a better way called XYZ
-- I couldn't find a "CASE" statement in python, someone mentioned dictionaries

6) I wrote a line de-duplicator - is there a better way 
-- actually thought i handled it pretty well....wait, what are you doing with that pin ??? <POP!!>

So, post code or no?

Thanks, 
PappaBear

Code now at::: https://github.com/PappaBearTroy/FWConverter
Reply
#5
To start with - you have a huge number of prints. Make one huge multi-line string constant, preferably  - in separate module
LET_ME_OVERWHELM_YOU = '''
Starting Firewall Conversion
Current version is {version}
.....
'''
And then print it with one statement
print(LET_ME_OVERWHELM_YOU.format(version=VERSION))
(constant name - well, you overwhelmed me  Cool)
Test everything in a Python shell (iPython, Azure Notebook, etc.)
  • Someone gave you an advice you liked? Test it - maybe the advice was actually bad.
  • Someone gave you an advice you think is bad? Test it before arguing - maybe it was good.
  • You posted a claim that something you did not test works? Be prepared to eat your hat.
Reply
#6
Print one multi-line constant because it's resource intensive to call print, or simply "cleaner" or easier to update, etc?
Appreciate the feedback, but not sure _why_ multiple prints are bad.  Seems like an easy "fix" so not arguing :-)
Reply
#7
Merged threads.

(Apr-18-2017, 07:45 PM)PappaBear Wrote: not sure _why_ multiple prints are bad
Prints can in some sense be considered expensive, but we usually just mean readability. Unless performance is bugging you, it's usually not worth the effort to optimize, especially (it seems) for what you're doing.

And I see now what you mean about not having a __main__. Having one would definitely help your code, as well as (as mentioned above) breaking things up into multiple files.

Some other comments (non-exhaustive):
Remove pass lines that don't do anything.
You have a lot of commented-out code. I recommend not having that at all, and if you really do need it, include comments so that a month or more later you know if it was commented out by accident, the code is no longer needed, or what. (Git and other version control systems can help with this as well).
I don't have time at the moment to read through the "### This is where the main program lives" part but that definitely looks fishy with the "counter".
Having consistent code style is a good idea. PEP 8 is a decent default, and documenting deviations is a good idea.
You use extra parentheses with your ifs; sometimes unneeded parens help with readability, but the whole expression that the if-block is using shouldn't be in parens.
Consider using a with-block for opened files.
Reply
#8
(Apr-18-2017, 07:45 PM)PappaBear Wrote: Print one multi-line constant because it's resource intensive to call print, or simply "cleaner" or easier to update, etc?
Appreciate the feedback, but not sure _why_ multiple prints are bad.  Seems like an easy "fix" so not arguing :-)

No, because your code looks overcrowded. And, frankly, 41 lines of print spaghetti just reduce motivation to read the code.

Besides, imagine that you'd like to clarify some points. Doing it in un-interrupted text would be easier than re-arranging prints.

Another issue - string concatenations. They are OK here and there, but you are overdoing it, and in general it's a dangerous practice - you may inadvertently insert a dictionary or list variable into expression - causing exception. 

Formatting sentences separates text from variables, and is much safer - formatting, even in the simplest form,  will implicitly convert your values to strings.

One more point - Python does not go heavy on parenthesis. But even without those, consider
if ((current_line[0] == "object") and (current_line[1] == "network")):
against
if current_line[:2] == ['object', 'network']:
The last (for now) but not the least - you use only double-quotes, and then escape them in strings. Single and double quotes are equal in Python - so if you surround your strings with single quotes, you don't have to escape double within
Test everything in a Python shell (iPython, Azure Notebook, etc.)
  • Someone gave you an advice you liked? Test it - maybe the advice was actually bad.
  • Someone gave you an advice you think is bad? Test it before arguing - maybe it was good.
  • You posted a claim that something you did not test works? Be prepared to eat your hat.
Reply
#9
Great feedback and mucho appreciatedo! 

double quotes -- The Fortinet FW requires a double quote around (some) objects.  But I was not aware Python sees " & ' equal, so good to know.

# of prints -- easy enough to change, I think I couldn't get the print to do so in a column, and took the lazy/easy path and wrote the lines individual

comments -- yes, will clean up.  I've also been seeing some code puts the comment after the line rather than above.  I find when it's after the line, I have to scroll to (to the right) too much

formating -- No idea what you're saying here...see the link, will RTFM

parenthesis  -- a product of me having multiple ifs, multiple items...will clean up

 if currentl_line[:2] 
-- I do not understand the syntax -- what the colon 2 is doing here.   Is that the following _2 items_ need to be in before the 3rd item(position 2) ? or is it those items need to be before _position 2_ ?  what is :2 representing?  


Really do appreciate the feedback... the more the better.
Reply
#10
Quote:
if currentl_line[:2]
-- I do not understand the syntax -- what the colon 2 is doing here.   Is that the following _2 items_ need to be in before the 3rd item(position 2) ? or is it those items need to be before _position 2_ ?  what is :2 representi
Its a slice

the : represents its a slice and not getting index [2]
Its more like current_line[X:2:X]  [start:stop:step] but you dont need to specify the beginning index of 0, you just leave it empty.

In this case there is a 2 at stop which means it will get indexes 0 and 1
Recommended Tutorials:
Reply


Possibly Related Threads…
Thread Author Replies Views Last Post
  Python best library for Excel reports & review of existing code MasterOfDestr 4 497 Feb-14-2024, 03:39 PM
Last Post: MasterOfDestr
  Review my code: convert a HTTP date header to a datetime object stevendaprano 1 1,912 Dec-17-2022, 12:24 AM
Last Post: snippsat
  Code review for S3 object creation beherap 0 2,115 Mar-29-2018, 01:31 PM
Last Post: beherap

Forum Jump:

User Panel Messages

Announcements
Announcement #1 8/1/2020
Announcement #2 8/2/2020
Announcement #3 8/6/2020