6
\$\begingroup\$

I'm taking a text file that looks like this:

77, 66, 80, 81
40,  5, 35, -1
51, 58, 62, 34
 0, -1, 21, 18
61, 69, 58, 49
81, 82, 90, 76
44, 51, 60, -1
64, 63, 60, 66
-1, 38, 41, 50
69, 80, 72, 75

and I'm counting which numbers fall in a certain bracket:

f = open("//path to file", "r")
text = f.read()
split = text.split()

greater70=0
sixtys=0
fiftys=0
fortys=0
nomarks=0

for word in split:
    temp = word.replace(',', '')
    number = int(temp)
    if number > 70:
        greater70+=1
    if number >= 60 and number <= 69:
        sixtys+=1    
    if number >= 50 and number <= 59:
        fiftys+=1
    if number >= 40 and number <= 49:
        fortys+=1 
    if number == -1:
        nomarks+=1           

print greater70 
print sixtys
print fiftys
print fortys
print nomarks

Have I done it in an efficient way sensible way or do experienced Python users think it looks a little convoluted?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Is 70 meant to be ignored or is that a mistake? \$\endgroup\$ – crdx Jun 7 '13 at 9:32
  • \$\begingroup\$ I'd be tempted to divide by 10 and use counter. \$\endgroup\$ – greybeard Nov 24 '16 at 6:18
4
\$\begingroup\$

Notes:

  • Creating so many variables is a warning sign that they should be grouped somehow (an object, a dictionary, ...). I'd use a dictionary here.

  • Consider learning some functional programming (as opposed to imperative programming) to apply it when it improves the code (IMO: most of the time). Here it means basically using collections.counter and generator expressions.

  • Don't read whole files, read them line by line: for line in file_descriptor.

  • There are programmatic ways of detecting intervals of numbers (simple loop, binary search). However, since you have here only 5 intervals it's probably fine just to write a conditional as you did. If you had, let's say 20 intervals, a conditional branch would be a very sad thing to see.

  • Conditional branches are more clear when you use non-overlapping conditions with if/elif/else (effectively working as expressions instead of statements).

I'd write:

from collections import Counter

def get_interval(x):
    if x > 70:
        return "greater70"
    elif 60 <= x < 70:
        return "sixtys"    
    elif x >= 50:
        return "fiftys"
    elif x >= 40:
        return "fortys" 
    elif x == -1:
        return "nomarks"   

with open("inputfile.txt") as file:
    counter = Counter(get_interval(int(n)) for line in file for n in line.split(","))
# Counter({'sixtys': 10, 'greater70': 10, None: 7, ..., 'fortys': 4})
\$\endgroup\$
2
  • \$\begingroup\$ Aren't those for clauses the wrong way around? I.e. shouldn't it be Counter(get_interval(int(n)) for n in line.split(",") for line in file)? \$\endgroup\$ – user9876 Jun 23 '13 at 23:42
  • \$\begingroup\$ @user9876: no, I am pretty sure it's correct (the output is real, I run that code). A list-comprehension is written in the same order you'd write a normal for-loop. \$\endgroup\$ – tokland Jun 24 '13 at 8:17
1
\$\begingroup\$

Looks good, but there are a few things I've noticed that could be better:

  • You should use the with statement when working with files. This ensures that the file will be closed properly in any case.
  • I would split the input string on ',' instead of replacing the comma with a space. This way, something like '-7,67,34,80' will be handled correctly. (It's a bit tricky because you want to split on newlines too, though. I did it using a list comprehension.)

    int() will ignore any whitespace before or after the number, so int(' 50') works just fine.

  • You can write those conditions as 60 <= number <= 69.
  • Since the numbers can only fall into one of the five categories, it's clearer (and more efficient) to use elif instead of if.
  • Assignments with = or += should have spaces around them in Python. (See PEP 8 for a Python style guide.)

This is how the changes would look:

with open("//path to file", "r") as f:
    split = [word for line in f for word in line.split(',')]

greater70 = 0
sixties = 0
fifties = 0
forties = 0
nomarks = 0

for word in split:
    number = int(word)
    if number > 70:
        greater70 += 1
    elif 60 <= number <= 69:
        sixties += 1    
    elif 50 <= number <= 59:
        fifties += 1
    elif 40 <= number <= 49:
        forties += 1 
    elif number == -1:
        nomarks += 1           

print greater70 
print sixties
print fifties
print forties
print nomarks

You might also want to think about how to avoid the repetition of code when you're calculating and printing the 40s, 50s and 60s (which, by the way, are spelt forties, fifties, sixties).

I also noticed that 70 falls into none of the categories, is that the way it's supposed to be?

\$\endgroup\$
0
0
\$\begingroup\$

Few points i have noticed in your code, that i hope, will help you.

1) Try changing your conditions like this: (no need to check two conditions)

if number > 69:
    greater70+=1
elif number > 59:
    sixtys+=1    
elif number > 49:
    fiftys+=1
elif number > 40:
    fortys+=1 
elif number == -1:
    nomarks+=1 

2) Always take care of closing file handler, or better to use with statements as other expert suggested.

\$\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.