10
\$\begingroup\$

I want to condense down some of this, is there anything I can change to make it better or shorter?

It works now but I don't know what else I can change, are any of my if statements changeable to be shorter?

def main():
    #displays company name and report
    print("Helena Hockey Haven")
    print("sales report")
    print()
    get_data()
    get_employees()
    get_sales()

#opens the file in read
def get_data():
    sales_numbers = open('SalesData.txt','r')
    num_list = sales_numbers.readlines()
    sales_numbers.close()
    index = 0
    while index < len(num_list):
        num_list[index] = num_list[index].rstrip('\n')
        index += 1
    return num_list
 #gets info from employees and calculates commission and bonuses   
def get_employees():
    line = get_data()
    index = 0
    total = 0
    average = 0
    commission = 0
    bonus = 0
    while index < len(line):
        name = line[index]
        if index % 13 == 0 and index != 0:
            #if statement to determine commission
            print ("Total sales: $"+str(round(total,2)))
            if total < 3000 :
                commission = total * .015
            elif total < 5000 :
                commission = total * .025
            elif total < 7000 :
                commission = total * .04
            else :
                commission = total * .0635
            print ("Total Commission: $"+str(round(commission,2)))
            if total < 5500 :
                #bonus payment determined and told if not acheived
                print ("No Bonus pay.")
                #brackets made in print statements to seperate the employees
                print ('|-------------------------------------------------------------------------------------------------|')
            else :
                bonus = total * .0365
                print ("Bonus pay: $"+str(round(bonus,2)))
                print('|-------------------------------------------------------------------------------------------------|')
            total = 0
        if " " in line[index]  :
            split = name.split(', ')
            print (split[1]+' '+split[0])
        if "." in line[index]  :
            total += float(line[index])
        index += 1
    print("Total sales: $"+str(round(total,2)))
    print("Total Commission: $"+str(round(commission,2)))
    #sales bonus calculation based on sales over 5500
    if total < 5500 :
        print ("No Bonus pay.")
        print('|-------------------------------------------------------------------------------------------------|')
    else :
        bonus = total * .0365
        print ("Bonus pay: $"+str(round(bonus,2)))
        print('|-------------------------------------------------------------------------------------------------|')
 #total average sales calculated
def get_sales():
    #seperates sales totals from individual employees, shows these are the totals
    print ()
    print ('[REPORT TOTALS]')
    print ()
    #employee with the most sales from the data is displayed 
    total = 0
    best_total = 0
    index = 0
    line = get_data()
    while index < len(line):
        name = line[index]
        if index % 13 == 0 and index != 0:
            if total > best_total:
                best_total = total
                employee=top
            total = 0
        if " " in line[index]  :
            split = name.split(', ')
            top=(split[1]+' '+split[0])
        if "." in line[index]  :
            total += float(line[index])
        index += 1
        #the employees name and sales numbers are printed 
    print(employee+" had the highest sales! $"+str(round(best_total,2)))
    print()
    print('|-------------------------------------------------------------------------------------------------|')
#the average sales are calculated and displayed
    index = 0
    total = 0
    average_sales = 0
    while index < len(line):
        if "." in line[index]  :
            total += float(line[index])
        index += 1
    average_sales+= total
    average_sales= average_sales/(round(index/13,0))
    print ("Average Sales: $"+str(round(average_sales,2)))
    print ('|-------------------------------------------------------------------------------------------------|')
main()
\$\endgroup\$
2
  • 2
    \$\begingroup\$ You do have some strange and redundant logic here. Could you please provide some lines from your SalesData.txt file, as that might illuminate some of the issues better. \$\endgroup\$
    – holroy
    Commented Dec 6, 2015 at 1:25
  • \$\begingroup\$ Are you using Python 2 or 3? It makes a small difference. \$\endgroup\$ Commented Dec 7, 2015 at 9:45

3 Answers 3

8
\$\begingroup\$

Code Review

Use the context manager to open files

And return list comprehensions, or better, generators - Instead of:

def get_data():
    sales_numbers = open('SalesData.txt','r')
    num_list = sales_numbers.readlines()
    sales_numbers.close()
    index = 0
    while index < len(num_list):
        num_list[index] = num_list[index].rstrip('\n')
        index += 1
    return num_list

do this (and use universal newlines mode with the U flag):

def get_data():    
    with open('SalesData.txt','rU') as sales_numbers:
        return [line.strip() for line in sales_numbers]

or perhaps a generator (doesn't materialize the whole thing in a list, but file stays open until it's exhausted):

def get_data():    
    with open('SalesData.txt','rU') as sales_numbers:
        for line in sales_numbers:
            yield line.strip()

Interesting issue raised by another answer:

They suggest map after calling readlines, which in Python 3 would return an iterator over a fully materialized dataset. The yield suggestion given above works fine in conjunction with using the file as an iterator (which makes sense, since you can think of a generator as a function that freezes at yield), but map does not - the file object gets closed when leaving the scope of the function:

>>> def fun2():
...     with open('foo', 'rU') as f:
...         return map(str.strip, f)
... 
>>> list(fun2())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: I/O operation on closed file.

Misc

line = get_data() # I don't think this is semantically correct

You can set all of these to zero on the same line:

index = total = average = commission = bonus = 0

Python's for loops seem to escape you?

while index < len(line):
    name = line[index]

do this instead:

for name in get_data():

Use newlines:

These are redundant calls to the print function:

print("Helena Hockey Haven")
print("sales report")
print()

Instead, call it once, using newline characters:

print("Helena Hockey Haven\nsales report\n")

or maybe just use a multiline string:

print("""Helena Hockey Haven
sales report
""")

Conclusion

That's enough for now. Please ask if you have further questions.

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

Based on your code, I'll make the assuption that your SalesData.txt file looks like the following:

Name1, Surname1
followed by 12 floats
Name2, Surname2
followed by 12 floats
etc.

If the content differs from that, you may need to adapt the code I'll provide slightly. But the speech remains valid.

Make efficient use of resources

Your get_data function only loads the file in memory and return its content. It is called 3 times but you should only call it once and use its result in get_employees and get_sales. Moreover, your first call to get_data just discard its result right away. All in all, this is a waste of computing resources.

def main():
    #displays company name and report
    print("Helena Hockey Haven")
    print("sales report")
    print()
    sales_content = get_data()
    get_employees(sales_content)
    get_sales(sales_content)

Is a better approach. You thus need to modify get_employees and get_sales so they accept the result of get_data as a parameter.

Do all the computations you need at once

You spend three loops computing totals. 2 aiming at getting a total for each employee and one getting the overall total to compute the average sales. Computing once and interpreting the results after that would, again, be more efficient.

Considering that it is also better practice to separate presentation from computation, you could perform the computation of the total for each employee while reading the file and then interpreting the results.

For this purpose, you can use a dictionnary to map an employee name to its total. And, in order to simplify managing the initialization of the totals, it is best to use an collections.defaultdict.

Combined to @chicks and @AaronHall advise of using with, get_data becomes:

from collections import defaultdict

def parse_sales_data(filename):
    sales = defaultdict(float)
    with open(filename) as f:
        for index, line in enumerate(f):
            line = line.strip()
            if index % 13 == 0:
                # Every 13 lines is the section for a new employee. Parse its name
                name, surname = line.split(', ')
                employee = surname + ' ' + name
            else:
                # Otherwise calculate total sales for the current employee
                sales[employee] += float(line)
    return sales

Note the use of enumerate which is prefered over a while loop. Note also the use of a paramter to get the file name. It is prefered for testing purposes since you can jump into an interactive session and try calling the function with various file names to test the behaviour and assert coherent results.

Separate presentation from computation

Now that we read the file only once and directly parse its content into a meaningful data structure. We can write a fonction that present the content of this data structure to the user:

def analyse_sales(sales_data):
    top_sales = 0.0
    for employee, sales in sales_data.items():
        # Check if employee of the month
        if sales > top_sales:
            top_sales = sales
            top_employee = employee

        # Compute commission
        if sales < 3000 :
            commission = sales * .015
        elif sales < 5000 :
            commission = sales * .025
        elif sales < 7000 :
            commission = sales * .04
        else :
            commission = sales * .0635

        # Compute bonus
        bonus = None if sales < 5500 else sales * 0.0365

        # Print infos for the employee
        print(employee)
        print("Total sales: ${:.2f}".format(sales))
        print("Total Commission: ${:.2f}".format(commission))
        if bonus is None:
            print("No bonus pay.")
        else:
            print("Bonus pay: ${:.2f}".format(bonus))
        print('|-------------------------------------------------------------------------------------------------|')
    # All employees stats have been printed, print top and average stats
    print()
    print('[REPORT TOTALS]')
    print()
    print("{} had the highest sales! ${:.2f}".format(top_employee, top_sales))
    print()
    print('|-------------------------------------------------------------------------------------------------|')
    print ("Average Sales: ${:.2f}".format(sum(sales_data.values())/len(sales_data)))
    print('|-------------------------------------------------------------------------------------------------|')

Note the use of format and the formatting string syntax to pretty print float values.

I also used sum purposefully in the end to show you that builtin functions already exists to achieve what you are looking for; even though I could have computed the sum in the for loop since I did need to iterate over the dictionnary.

Nitpicks

You put a space before : after an if or an else but not after while or def. First of, get consistent, choose one or the other, not both. Second, do not put that space in, it's unnecessary and not common at all.

You also put an unnecessary space between print and the following open parenthesis.

And any time you repeat portions of code, ask yourself if this is really necessary (hint, most of the time it isn't). And try to come up with a more abstrct approach.

Conclusion

Using both parse_sales_data and analyse_sales that I provided, your main becomes:

def main():
    #displays company name and report
    print("Helena Hockey Haven")
    print("sales report")
    print()
    analyse_sales(parse_sales_data('SalesData.txt'))
\$\endgroup\$
1
  • \$\begingroup\$ Not bad advice, so plus 1 as I begin working towards my sportsmanship badge... \$\endgroup\$
    – Aaron Hall
    Commented Dec 6, 2015 at 19:35
1
\$\begingroup\$

I was able to shorter one function for you:

#!/usr/bin/env python                                                                                                              

import string                                                                                                                      

def get_data():                                                                                                                    
        with open('sales_data.txt') as sales_h:                                                                                    
                num_list = sales_h.readlines()                                                                                     

        num_list = map(lambda x: x.rstrip('\n'), num_list);                                                                        
        return num_list                                                                                                            

print get_data() 

Notes:

  • Using with eliminates the need for close
  • using map to loop through the num_list eliminates the need for the for loop and the index variable
\$\endgroup\$
4
  • 1
    \$\begingroup\$ readlines() loads the entire file into memory at once, and if they're using map and Python 3 (which I suspect they are) you're semantically misinforming - they're not returning a list, but an iterator. And the lambda is unnecessary, you can pass str.rstrip directly to map, and by default it removes whitespace, which appears to be the semantic intent. \$\endgroup\$
    – Aaron Hall
    Commented Dec 6, 2015 at 2:22
  • \$\begingroup\$ @AaronHall That may be true in Python 3 but not Python 2 where readlines returns a list directly. \$\endgroup\$ Commented Dec 7, 2015 at 9:45
  • \$\begingroup\$ The fact that readlines returns a list directly is why it has to load the entire file into memory at once - and this is an inefficiency, it is better to use the file object as an iterator. \$\endgroup\$
    – Aaron Hall
    Commented Dec 7, 2015 at 12:03
  • \$\begingroup\$ I fully admit there are better ways to do this than I did. I'm a python beginner myself. When I started this there were no answers to the question and it seemed like something the OP would appreciate. There's obviously much better advice in the other answers. \$\endgroup\$
    – chicks
    Commented Dec 7, 2015 at 13:56

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.