2
\$\begingroup\$

I wrote a dirty and ugly code to show in console how many bugs are left until the release of elementary codename Isis. The code works but it's pretty inefficient. What it does is:

  1. downloads the entire Launchpad site
  2. writes it down to a temp file
  3. scans for the appropriate lines in said file
  4. stores the number of bugs still open
  5. deletes the temp file
  6. shows the total number of bugs left

How could I make this code more efficient? For example, is there a way to get that data without having to download the entire site each time? Or to scan the downloaded data for the still opened bugs, without having to write it down to a temp file first?

I'm running python 2.7.3.

import urllib
import os

"""
Retrieves the number of bugs left until the arrival of elementary OS Isis.
"""

# Get Launchpad data.
f = urllib.urlopen("https://launchpad.net/elementary/+milestone/isis-beta1")
s = f.read()
f.close()

# Write data to temp file.
ff = open("temp.del", "w")
ff.write(s)
ff.close()

# Find number of bugs left.
temp = open("temp.del", "r")
for i, line in enumerate(temp):

    # Incomplete bugs.
    if 'span class="statusINCOMPLETE">' in line and i < 450:
        a = temp.next()
        b = a.split('<strong>')
        c = b[1].split('</strong>')
        b_incomp = int(c[0])

    # Confirmed bugs.
    if 'span class="statusCONFIRMED">' in line and i < 450:
        a = temp.next()
        b = a.split('<strong>')
        c = b[1].split('</strong>')
        b_conf = int(c[0])

    # In progress bugs.
    if 'span class="statusINPROGRESS">' in line and i < 450:
        a = temp.next()
        b = a.split('<strong>')
        c = b[1].split('</strong>')
        b_inprog = int(c[0])

# Delete temp file.
os.remove('temp.del')

# Print to console.
bugs = b_incomp + b_conf + b_inprog
print '\n%d bugs left until Isis. Quit moaning.\n' % bugs
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

To start off, I will say what I say on every question dealing with files: always use with syntax.

By using this syntax you lessen the change for bugs; especially the bug that happens when you forget to call close() on an open file pointer. Your code shows this bug. You open the temp.del file, but never call close on it. Because of this, when you program finishes running, you have an active file pointer that the OS needs to clean up.

To fix this, simply do this:

with open('temp.del', 'r') as file:
    # Do stuff.

Once this block of code finishes, file will automatically get closed.


To answer you question whether or not its necessary to write the temporary file, take a look a the urllib.urlopen documentation. It says:

Except for the info(), getcode() and geturl() methods, these methods have the same interface as for file objects — see section File Objects in this manual. (It is not a built-in file object, however, so it can’t be used at those few places where a true built-in file object is required.)

This means that you should be able to iterate through this just like you would a file. Which, in your case, means you do not have to save the temporary file. Unfortunately, in Python 2.7, the return object from urllib.urlopen is not a valid context manager (i.e doesn't implement __enter__ and __exit__). So we have to use open and close:

file = urllib.urlopen("https://launchpad.net/elementary/+milestone/isis-beta1")
for line in file:

Now onto improvements:

  1. In each of your if statements, you have the condition and i < 450. You can bring this out into its own statement, and because you don't do anything after line 450, you can simply break if it evaluates to true:

    for index, line in enumerate(file):
        if index >= 450:
            break
    
  2. Getting the bug counts can be done easier using a regex:

    import re
    
    # Grabs all the numerical data, converts them to ints, then sums them together.
    bugs += sum(map(int, re.findall('\d+', next_line)))
    
  3. Each if statement holds the same code: you get the next line, split it, and then assign a value to a certain variable. After you finish the bugs, you add them all together. You don't differentiate after you parse them. You can combine these into a single if statement and simply keep a single running total like so:

    if any(span in line for span in ['span class="statusINCOMPLETE">',
                                     'span class="statusCONFIRMED">',
                                     'span class="statusINPROGRESS">']):
    
        bugs += sum(map(int, re.findall('\d+', next_line)))
    

    If you need to differentiate the different kind of bugs, you can use a dict:

    bug_types = {'span class="statusINCOMPLETE">': 0,
                 'span class="statusCONFIRMED">': 0,
                 'span class="statusINPROGRESS">': 0}
    
    for key in bug_types:
        if key in line:
            bug_types[key] += sum(map(int, re.findall('\d+', next_line))))
    
  4. When formatting string output, convention says to use the str.format() function. The syntax look like this:

    print '{} {}!'.format('Hello', 'World')
    

If you take all of my suggestions into account, here is your new code:

import urllib
import os
import re

bug_types = {'span class="statusINCOMPLETE">': 0,
    'span class="statusCONFIRMED">': 0,
    'span class="statusINPROGRESS">': 0}

# Even though we have to use `open` and `close` and can simply enclose the next
# for loop with the calls, I still would load the file into a temporary array.
# Then iterate over that.
file = urllib.urlopen("https://launchpad.net/elementary/+milestone/isis-beta1")    
lines = [str(line) for line in file]
file.close()

# Because of the way `urlopen` streams the data, we cannot use the `next` command.
# Therefore, we need to use a flag to remember what kind of bug we found on the 
# previous line.
bug_type = ''
for index, line in enumerate(lines):
    if index >= 450:
        break

    if bug_type:
        bug_types[bug_type] += sum(map(int, re.findall('\d+', line)))
        bug_type = ''
    else:
        for key in bug_types:
            if key in line:
                bug_type = key
                break

print('\n{} bugs left until Isis. Quit moaning.\n'.format(sum(bug_types.values())))
\$\endgroup\$
6
  • \$\begingroup\$ Thank you very much for the answer @Darin. Have you tried running this code? I get a number of errors, one of which I can't fix. 1st: the quotation mark is misplaced in 'span class="statusINPROGRESS">: 0'. 2nd there's a syntax error in the last line with .print(sum(... 3rd I'm getting a AttributeError: addinfourl instance has no attribute 'exit' error in the with urllib.urlopen( line. \$\endgroup\$
    – Gabriel
    Commented Jun 6, 2014 at 18:02
  • 1
    \$\begingroup\$ I actually did not have time to run the code; I was hoping my copy-pasting skills would be sharp :P However, I have updated the code and it works on my maching (Python 3.4). I am currently working on fixing up a small issue I noticed. \$\endgroup\$ Commented Jun 6, 2014 at 18:19
  • \$\begingroup\$ Ok, I'll wait until you update the answer. I'm running python 2.7.3, do you think it'll work? \$\endgroup\$
    – Gabriel
    Commented Jun 6, 2014 at 18:43
  • \$\begingroup\$ Ok this should be the final version. I am assuming that there will be only one type of bug per line. \$\endgroup\$ Commented Jun 6, 2014 at 19:12
  • \$\begingroup\$ I keep getting the AttributeError: addinfourl instance has no attribute '__exit__' error in the with urllib.urlopen( line. Could this be related to my version of python (2.7.3) versus yours? \$\endgroup\$
    – Gabriel
    Commented Jun 6, 2014 at 19:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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