3
\$\begingroup\$

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


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('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:
        return f.read()
\$\endgroup\$
  • 1
    \$\begingroup\$ 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 :) \$\endgroup\$ – IEatBagels Oct 23 '19 at 15:10
  • 1
    \$\begingroup\$ You seem to have omitted the import statements when pasting here - could you please reinstate them? Thanks. \$\endgroup\$ – Toby Speight Oct 23 '19 at 15:26
  • \$\begingroup\$ True. I edited my code. \$\endgroup\$ – juniorprogrammer Oct 23 '19 at 15:58
1
\$\begingroup\$

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


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


def read_zipfile_item(filename):
    with zipfile.ZipFile('/tmp/junar.zip') as zip_file:
        with zip_file.open(filename) as f:
            return f.read().decode('utf8')

# Testing
url = 'https://www.ohjelmointiputka.net/tiedostot/junar.zip'
download_zipfile(url=url)
print(read_zipfile_item('junar1.in'))

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

10
6
1
4
10
7
2
3
9
5
8
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Peilonrayz Oct 23 '19 at 20:13
  • \$\begingroup\$ @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. \$\endgroup\$ – RomanPerekhrest Oct 23 '19 at 20:45
  • \$\begingroup\$ 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 :) \$\endgroup\$ – C.Nivs Oct 24 '19 at 13:54
2
\$\begingroup\$

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:
            return f.read()

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

def download_and_return_content():
    # your temporary file-handle
    tmp_file = BytesIO()

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

    tmp_file.seek(0)

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

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:
            tmp.write(response.read())

    # 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:
            return fh.read()

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:
        tmp.write(response.read())

    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:
            return fh.read()


# 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)
    fh.write(tmp_file.read())

Of course this all depends on what you need that zipfile for, but this cuts down on the amount of reads and writes you are doing under the hood.

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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