I wrote a function that downloads a file https://www.ohjelmointiputka.net/tiedostot/junar.zip if it not already downloaded, unzips it and returns the content of junar1.in found in this zip. I have PEP8 complaints about the length of lines that I would like to fix. Is there a way to make the code more readable?

My code :

import os.path
import urllib.request
import shutil
import zipfile

if not os.path.isfile('/tmp/junar.zip'):
url = 'https://www.ohjelmointiputka.net/tiedostot/junar.zip'
with urllib.request.urlopen(url) as response, open('junar.zip', 'wb') as out:
data = response.read() # a bytes object
out.write(data)
shutil.move('junar.zip','/tmp')
with zipfile.ZipFile('/tmp/junar.zip', 'r') as zip_ref:
zip_ref.extractall('/tmp/')
with open('/tmp/junar1.in') as f:

• Hey juniorprogrammer, I edited your question to make it more to the "CodeReview Format"' please check it out. Feel free to rollback the edit if you don't like it, but I think your question will receive more attention this way :) Oct 23 '19 at 15:10
• You seem to have omitted the import statements when pasting here - could you please reinstate them? Thanks. Oct 23 '19 at 15:26
• True. I edited my code. Oct 23 '19 at 15:58

Let's start refactoring/optimizations:

• urllib should be replaced with requests library which is the de facto standard for making HTTP requests in Python and has reach and flexible interface.
• instead of moving from intermediate location (shutil.move('junar.zip','/tmp')) we can just save the downloaded zip file to a destination path with open('/tmp/junar.zip', 'wb') as out
• decompose the initial function into 2 separate routines: one for downloading zipfile from specified location/url and the other - for reading a specified (passed as an argument) zipfile's member/inner file
• reading from zipfile.ZipFile.open directly to avoid intermediate extraction. Otherwise zipfile contents should be extracted at once, then - just reading a regular files being extracted (with adjusting the "reading" function)

From theory to practice:

import os.path
import requests
import zipfile
import warnings

if not os.path.isfile('/tmp/junar.zip'):
with open('/tmp/junar.zip', 'wb') as out:
out.write(requests.get(url).content)

with zipfile.ZipFile('/tmp/junar.zip') as zip_file:
with zip_file.open(filename) as f:

# Testing
url = 'https://www.ohjelmointiputka.net/tiedostot/junar.zip'


The actual output (until the input url is accessible):

10
6
1
4
10
7
2
3
9
5
8

• That is fine with me - I have deleted mine. Please feel free to add your point 3 back. If you do please add the rational that you stated in the comments, thank you. Oct 23 '19 at 20:13
• @Peilonrayz, mine were deleted. As for turning back the 3rd thesis, I'm not sure whether it's worthwhile as it evoked a negative "reaction". I didn't quite follow why just optionally logging failed downloading attempts (logger.warning(...)) can cause such a critics. In my understanding, if it happened once, then, hypothetically that fragment could be negatively perceived by someone else again. So maybe we may leave it uncovered/hidden. Oct 23 '19 at 20:45
• Where to log information can be subjective, but ultimately alerting the user that a different behavior will occur due to X condition is never a bad thing. I think the sticking point may have been using warning which implies that something needs attention when maybe info might be a bit more appropriate. However, I stand by the premise that having alerting is better than none :) Oct 24 '19 at 13:54

# PEP-8 Line Length

For particularly lengthy lines, you can always use parentheses to wrap them instead of the usual \ sign:

x = (1 + 2 + 3 +
4 + 5 + 6)


# Function Refactor

I would skip the step in your if statement where you use shutil.move, just save the file in /tmp directly:

def download_and_return_content():
if not os.path.isfile('/tmp/junar.zip'):
url = 'https://www.ohjelmointiputka.net/tiedostot/junar.zip'
with urllib.request.urlopen(url) as response, open('/tmp/junar.zip', 'wb') as out:
data = response.read() # a bytes object
out.write(data)


Furthermore, if you are just looking to extract a single file, you can open one of the archives directly using ZipFile.open

    with ZipFile('/tmp/junar.zip') as myzip:
with myzip.open('junar1.in') as f:


ZipFile can also take a file-like object, so you can use a BytesIO object to hold your zip-file bytes, since /tmp implies you might not need to hold onto this data:

from io import BytesIO

tmp_file = BytesIO()

url = 'https://www.ohjelmointiputka.net/tiedostot/junar.zip'
with urllib.request.urlopen(url) as response:

tmp_file.seek(0)

with ZipFile(tmp_file) as myzip:
with myzip.open('junar1.in') as fh:


Lastly, the if check implies that maybe you want to cache the data somehow. You could in theory use BytesIO as a mutable default. You can use BytesIO.tell() as your check if it has content:

def get_content(tmp=BytesIO()):
# buffer is at position 0, it has not been read or written to
# therefore it is probably empty
if not tmp.tell():
tmp.truncate() # just in case
url = 'https://www.ohjelmointiputka.net/tiedostot/junar.zip'
with urllib.request.urlopen(url) as response:

# set buffer to position 0 to read content
tmp.seek(0)

with ZipFile(tmp) as myzip:
# this will move the buffer to a non-zero position
# so, now tmp.tell() will be non-zero and will pass the
# if check on the next function call
with myzip.open('junar1.in') as fh:


As a caveat, there are caching libraries in python that can accomplish this as well, I'm just not familiar enough with them to suggest any in a meaningful way.

Before everybody grabs their torches and pitchforks, the non-mutable-default way (usually mutable defaults are seen as bad design) could look something like this:

# refactor into two functions, one that does the actual urllib call
# for you to retrieve your data
def get_data(tmp=None):
tmp = tmp or BytesIO()

with urllib.request.urlopen(url) as response:

return tmp

# and one to actually extract the file
def extract_file(tmp=None):
tmp = tmp or get_data()

tmp.seek(0)

with ZipFile(tmp) as myzip:
with myzip.open('junar1.in') as fh:

# now you can hold that BytesIO object
tmp_file = get_data()

content = extract_file(tmp_file)
# returns b'10\r\n6\r\n1\r\n4\r\n10\r\n7\r\n2\r\n3\r\n9\r\n5\r\n8\r\n'

# and if you want to write that temp file somewhere
with open('/tmp/somefile.zip', 'wb') as fh:
tmp_file.seek(0)