Python Forum
Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Inelegant code!
#1
I hope it's ok to ask for what may be basic help. I am a hobbyist and newish to Python, but used to code in COBOL many years ago. I'd like some help on how you would re-write the structure of my code.

I like to create code to protect my home network via my router and have written some code (below) that successfully reads and processes a file from a website. The file is a list of IP addresses of tor exit points. I can only download it once a day from the remote website so need to save it the first time so I can run the script more than once if I need to. The script has been running successfully for many months.

The code works but I recognise that is it completely inelegant. It drops in and out of if statement and doesn't save the file until the end. I'm hoping someone could give me a bit of help to structure the code in a more elegant pythonesque way. Should I be using Try statements to catch the != 200, but how big should the try statement be?

I realise this is only a snippet, but a response giving some structured pseudocode that I can copy would be very helpful.

TIA, Charles
##############################################################################
# Get tor file once a day from the internet and save it for another run
##############################################################################
print('>>tor')
count = 0
AddressFile = datePrefix + '-torlist.txt'

if os.path.isfile('html/' + AddressFile):
url = 'http://192.168.xxx.xxx/' + AddressFile
print('>>tor from file')
else:
url = 'https://www.xxxxxxxxx/torlist/?exit'
print('>>tor from web')
response = requests.get(url)
if response.status_code != 200:
print('Failed to get data:', response.status_code)
else:
wrapper = csv.reader(response.text.strip().split('\n'), delimiter=';')
for record in wrapper:
if record[0][0:1].isnumeric() and len(record[0]) != 39:
OutText = record[0] + "/32"
ip_list.append(IPNetwork(OutText))
count += 1
print('>>tor: ' + str(count))

if not os.path.isfile('html/' + AddressFile):
f = open('html/' + AddressFile, "w")
f.write(str(response.text))
f.close()
Reply
#2
This is just a fragment of your code; However, I would refactor as follows:
1) use logging instead of printing;
2) Lines No. 8-13: return url? these lines could be pushed to a function, e.g. def get_url(...):
3) requests.get -- may lead to an exception (e.g. if url is bad); you probably need to wrap it with try/except;
4) you don't need to wrap status_code checking with try/except (answer to your question);
5) csv.reader can raise an exception, if loaded content is not csv-compliant; probably, you need to put try/except here. Anyway, I would put these lines into some function.
6) Use with when opening files, e.g. with open(os.path.join('html', AddressFile) as out_file: (in this case you don't need to care about closing the file (out_file.close() isn't needed anymore))
7) Another option is to use pathlib when you need concatenate paths.
Reply
#3
One idea which might be worth of considering is to use 'x' (create) mode while opening the file. This will raise FileExistsError if file is already existing.

try:
    with open(filename, 'xt') as f:   # t for text mode, b for binary mode
        # do stuff
except FileExistsError:
    # do other stuff       
Also you may consider pepify/prettify the code using variable naming convetions and f-strings (address_file = f'{date_prefix}-torlist.txt')

Beware of .isnumeric expected behaviour as well:

>>> digits = '123'
>>> numerics = '一二三'   # '123'in Kanji
>>> decimals = '1²'
>>> digits.isnumeric()
True
>>> numerics.isnumeric()
True
>>> decimals.isnumeric()
True
I'm not 'in'-sane. Indeed, I am so far 'out' of sane that you appear a tiny blip on the distant coast of sanity. Bucky Katt, Get Fuzzy

Da Bishop: There's a dead bishop on the landing. I don't know who keeps bringing them in here. ....but society is to blame.
Reply


Forum Jump:

User Panel Messages

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