201
\$\begingroup\$

I applied for a job and they asked me to write code with the following requirements:

Get a "toolbar offer" description from http..update.utorrent.com/installoffer.php?offer=conduit. Parse the bencoded (see http://en.wikipedia.org/wiki/Bencode) response and extract the URL of the binary and its SHA1 hash. The hash is contained in the value of the key 'offer_hash'. The binary is in list for the key 'offer_urls' — please use the one prefixed with "http..download.utorrent.com/".

Download the binary and calculate its hash

Verify that the calculated SHA1 hash is identical to the one provided in the offer details. It is okay to use existing libraries for bencoding, hashing, etc.

What is wrong with my code? The recruiter said it didn't meet their bar.

# Script that verifies the correctness of a toolbar offers

import urllib, bencode , hashlib
from hashlib import sha1

url = "http://update.utorrent.com/installoffer.php?offer=conduit"

filename = "content.txt"

f= urllib.urlretrieve(url, filename)

#Bdecoding the response

with open (str(filename), 'rb') as myfile:
           data=myfile.read()
           decoded = bencode.bdecode(data)

# Returning the list for the key 'offer_urls'


list = decoded['offer_urls']

#print list


# Returning the URL of the binary that is prefixed with "http://download3.utorrent.com/"

length = len (list)

prefix = "http://download3.utorrent.com/"

i= -1

while i < length:
     i = i + 1
     if list[i].startswith(prefix) :
        break
binary= list[i]

print "The URL of the the binary is: " , binary


# Returning the sha1 hash contained in the value of the key 'offer_hash'


encrypted_hash = decoded['offer_hash']
sha1_hash1 = encrypted_hash.encode('hex')
print "The sha1 hash contained in the value of the key 'offer_hash' is: " , sha1_hash1 


# Downloading the binary and calculating its hash

urllib.urlretrieve ( binary , "utct2-en-conduit-20130523.exe")
file = "C:\Python27\utct2-en-conduit-20130523.exe"
with open (file, 'rb') as myfile:
           downloaded=myfile.read()

k = hashlib.sha1()
k.update(downloaded)
sha1file = k.hexdigest()
print "The sha1 hash of the downloaded binary is: " , sha1file

# Verify that the calculated sha1 hash is identical to the one provided in the offer details

if (sha1file == sha1_hash1) :
    print "Test result = Pass"
else :
    print "Test result = Fail"
\$\endgroup\$
  • 55
    \$\begingroup\$ A fairly simple problem with your code: Comments should explain why, not how. \$\endgroup\$ – AJMansfield Jun 27 '13 at 17:25
  • 2
    \$\begingroup\$ It does show your a pragmatist which is good when your working alone. \$\endgroup\$ – marabutt Jun 27 '13 at 23:44
348
\$\begingroup\$

I'm going to tell you the things in your code that made me wonder.

# Script that verifies the correctness of a toolbar offers

import urllib, bencode , hashlib

Extra space after bencode suggests lack of attention to detail.

from hashlib import sha1

url = "http://update.utorrent.com/installoffer.php?offer=conduit"

filename = "content.txt"

f= urllib.urlretrieve(url, filename)

Use of single-letter variables suggests lack of attention to clean code.

#Bdecoding the response

with open (str(filename), 'rb') as myfile:

filename is already a string. Converting it to a string again suggests confusion as to what is going on.

           data=myfile.read()
           decoded = bencode.bdecode(data)

Massive indentation suggests disregard for consistent indentation style.

Downloading the file to disk then reading it into memory is poor use of available APIs. You should download the file directly into memory.

# Returning the list for the key 'offer_urls'

Excess whitespace suggests someone is enter-happy.

list = decoded['offer_urls']

"Returning" is technically incorrect here. You aren't in a function, so you aren't returning anything. Use of list is a bad name for a variable since it's a built-in Python type.

#print list


# Returning the URL of the binary that is prefixed with "http://download3.utorrent.com/"

length = len (list)

Putting spaces after function calls is odd, and you don't do it consistently. There's also rarely a good reason to store the length of a list.

prefix = "http://download3.utorrent.com/"

i= -1

No space before the =.

while i < length:
     i = i + 1
     if list[i].startswith(prefix) :
        break
binary= list[i]

You are using a while loop when you should be using a for loop. It is also going to cause an IndexError, rather than fail gracefully, if the prefix isn't found.

print "The URL of the the binary is: " , binary


# Returning the sha1 hash contained in the value of the key 'offer_hash'


encrypted_hash = decoded['offer_hash']

Is that hash really encrypted?

sha1_hash1 = encrypted_hash.encode('hex')
print "The sha1 hash contained in the value of the key 'offer_hash' is: " , sha1_hash1 


# Downloading the binary and calculating its hash

urllib.urlretrieve ( binary , "utct2-en-conduit-20130523.exe")
file = "C:\Python27\utct2-en-conduit-20130523.exe"

Hard-coded filename will be easy to break. You shouldn't even need to do it. The string also contains "\" which should be escaped, or the whole string should be raw. i.e.

"C:\\Python27\\utct2-en-conduit-20130425.exe"

or

r"C:\Python27\utct2-en-conduit-20130425.exe"

or in most cases you can use the other slashes, even on Windows.

"C:/Python27/utct2-en-conduit-20130425.exe"

You got away with what you did, but that's mostly because you are lucky.

with open (file, 'rb') as myfile:
           downloaded=myfile.read()

This last piece of code did pretty much the same thing as a previous bit of code, but you didn't use a function.

k = hashlib.sha1()
k.update(downloaded)
sha1file = k.hexdigest()
print "The sha1 hash of the downloaded binary is: " , sha1file

# Verify that the calculated sha1 hash is identical to the one provided in the offer details

if (sha1file == sha1_hash1) :

You've put parentheses, which are not necessary.

    print "Test result = Pass"
else :
    print "Test result = Fail"

Your basic problems:

  1. Your style is poor and inconsistent
  2. A few things suggest you don't quite understand what's going on
  3. You haven't taken care to make the code readable

Here's my take on the problem:

# Script that verifies the correctness of a toolbar offers

import urllib
import bencode
import hashlib
import contextlib

# it's common practice to put input details like this
# as global constants at the start of your script
URL = "http://update.utorrent.com/installoffer.php?offer=conduit"
PREFIX = "http://download3.utorrent.com/"

# by breaking down your task into functions
# you can make the code easier to follow
def read_bencoded_url(url):
    with contextlib.closing(urllib.urlopen(url)) as offer_file:
        return bencode.bdecode(offer_file.read())

def find_string_with_prefix(strings, prefix):
    for string in strings:
        if string.startswith(prefix):
            return string
    else:
        raise Exception("Did not find prefix: {}".format(prefix))

# this function is a bit more complicated
# but it avoids saving the file to disk or loading it entirely into memory.
# instead it hashes it 4096 bytes at a time
def hash_of_url(url):
    hasher = hashlib.sha1()
    with contextlib.closing(urllib.urlopen(url)) as binary_file:
        while True:
            data = binary_file.read(4096)
            if not data:
                break
            hasher.update(data)
    return hasher.hexdigest()

# the actual high level logic just calls the functions
# this avoid obscuring the logic with lower level details
def main():
    decoded = read_bencoded_url(URL)
    binary = find_string_with_prefix(decoded['offer_urls'], PREFIX)
    reported_hash = decoded['offer_hash'].encode('hex')
    actual_hash = hash_of_url(binary)
    print "The sha1 hash contained in the value of the key 'offer_hash' is: ", reported_hash
    print "The sha1 hash of the downloaded binary is: " , actual_hash

    if reported_hash == actual_hash:
        print "Test result = Pass"
    else:
        print "Test result = Fail"

if __name__ == '__main__':
     main()
\$\endgroup\$
  • 107
    \$\begingroup\$ Harsh... but very well done. Hopefully just what the doctor ordered. \$\endgroup\$ – WernerCD Jun 27 '13 at 13:06
  • 38
    \$\begingroup\$ I was really confused by this answer until I realized I wasn't on StackOverflow anymore. \$\endgroup\$ – Mooing Duck Jun 27 '13 at 23:07
  • 20
    \$\begingroup\$ I wouldn't ding him on the extra parens. When you're dealing with multiple languages it's good to be defensive with paren usage, extra ones do no harm. \$\endgroup\$ – Loren Pechtel Jun 28 '13 at 0:02
  • 99
    \$\begingroup\$ I don't think this is too harsh by any means. If you upload your code, you should be GRATEFUL for a critique this thorough. Well done! \$\endgroup\$ – travisbartley Jun 28 '13 at 1:26
  • 33
    \$\begingroup\$ @WinstonEwert: Thanks a lot for taking the time to provide me with such a detailed answer!! It really helped! \$\endgroup\$ – user26614 Jul 1 '13 at 17:30
58
\$\begingroup\$

Run your code through pep8 and possibly a more pedantic static analyzer like pylint.

You will find these tools don't like some of your formatting and variable names. They will likely complain there are too many variables, branches, etc. because the code is not broken up into modularized functions.

\$\endgroup\$
42
\$\begingroup\$

If I was to take your code then multiply it out 100 or a thousand times into other files (the size of a regular commercial/enterprise application then it would be a dog's breakfast and not maintainable at all.

You need to take more care with your code and make it readable and consistent. Think of it as formatting your resume for a potential employer. Use consistent whitespace between characters, consistent comments above the code (not multiple line breaks between) and above all adopt one of the common style guides for the language. Winston posted an excellent example earlier.

This shows you can at least write code that reads and presents well. If you're coding in a professional environment, other people have to read the code you wrote as well and they need to understand it quickly. If they're scanning through the code then suddenly there's a random comment floating in the page it's a real distraction.

A good book to read is Clean Code by Robert Martin. You must read this as if your life depended on it.

\$\endgroup\$
  • 2
    \$\begingroup\$ xkcd.com/859 ) \$\endgroup\$ – jkd Aug 21 '17 at 3:18
27
\$\begingroup\$

Problems:

  1. I would say that import should be an efficient way of reusing the libraries.

    import urllib, bencode , hashlib
    from hashlib import sha1
    

    In the above line of code you have imported hashlib and sha1 from hashlib on the second line. There is nothing wrong in that, but when you just want to use only sha1, then no need to import the entire class.

    Same with other classes: how many members of the above mentioned libraries have you used in your code?

  2. All the constants should be predefined at the beginning of the script, not wherever required.

  3. What if the above file failed to open (in case file not present)? How do you deal with this exception?

    with open (str(filename), 'rb') as myfile:
           downloaded=myfile.read()
    

    Use try-except to catch exceptions.

  4. Filename is already a string.

  5. No need to use an extra variable to store the length of the list. Also there is a space in between the function len and its arg.

    length = len (list)
    
  6. Use a for loop for indexing, which is better than using a while loop.

  7. Meaningless variable names such as i, f, k.

  8. The tasks are not well-divided into different subroutines.

\$\endgroup\$
  • 1
    \$\begingroup\$ Problem 8 is the biggest problem. His solution is a typical case of a transactional script. I hate transactional scripts, and others do too. \$\endgroup\$ – U Mad Dec 5 '13 at 11:42

protected by syb0rg Feb 25 '14 at 21:41

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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