1
\$\begingroup\$

This function of my program is searching a file that is formatted as first_name last_name rate hours:

John Smith 25 80
Mary Johnson 15.5 60.0
...

With one space between each item.

The user enters the first name and last name of the employee and while the line is not equal to null it searchs for the employee. If its found then found=True and it gets the rate and hours from that line and breaks out of the loop. Then if found=True, it will print the name of the employee and their gross pay (rate * hours), else it says employee is not found. The code is working in my program but I will be using something very similar later on for adding/modifying/deleting new employee functions.

# 'print_emp', display only a single employee's data chosen by the user displayed as
# firstname, lastname, grosspay (on one line of output)
def print_emp():
    menu_name = ' '*int(OFFSET/2) + "EMPLOYEE LOOKUP"
    dotted = (OFFSET+len(menu_name))*'-'
    found = False

    try:
        file= open('employees.txt')
        line = file.readline()
        name = input('Give me an employee to search for ')
        # run thru while loop & search for employee
        while line != ' ':
            line = line.split(' ') #split into a list
            if name == line[0]+' '+line[1]:
                found = True
                rate = float(line[2])
                hours = float(line[3])
                break
            line = file.readline() # read subsequent records
        # close file
        file.close()
        if found:
            print(name, '$%4.2f' % (rate*hours))
        else:
            print('Employee is not listed')

    except FileNotFoundError:
        print("Error: File not found.")
\$\endgroup\$
  • \$\begingroup\$ How many employees are there? How often is this search function used? (I.e. does it make sense to load the whole file in a data structure into memory?) \$\endgroup\$ – Graipher Jul 12 '16 at 18:54
  • \$\begingroup\$ @Graipher there aren't a lot of employees, about 5, but more can be added and deleted \$\endgroup\$ – ByteSettlement Jul 12 '16 at 19:00
2
\$\begingroup\$

You could use the with..as construct to not worry anymore about closing the file.

Also iterating over the file and reading in the data can be simplified.

Also, you should not shadow the reserved name file.

menu_name and dotted seem not to be used atm.

Implementing these changes I get the following code:

def print_emp():
    # menu_name = ' '*int(OFFSET/2) + "EMPLOYEE LOOKUP"
    # dotted = (OFFSET+len(menu_name))*'-'
    search_name = input('Give me an employee to search for: ')
    try:
        with open('employees.txt') as employees:
            for line in employees:
                if not line:
                    continue
                first_name, last_name, rate, hours = line.split()
                name = "{} {}".format(first_name, last_name)
                rate, hours = map(float, (rate, hours))
                if name == search_name:
                    print("{} ${:4.2f}".format(name, rate*hours))
                    break
            else:  # Will be triggered if the loop was not interrupted with break
                print('Employee is not listed')
    except FileNotFoundError:
        print("Error: File not found.")

Now, you could actually separate the concerns here. Write a function that reads all employees, another that finds a name in all employees and a third that prints it. This can be efficiently implemented using classes. I would probably also not fail and continue if the file is missing:

class Employee:
    def __init__(self, name, rate, hours):
        self.name = name
        self.rate = rate
        self.hours = hours

    def __repr__(self):
        return "name: {}, ${:4.2f}/hour, {} hours".format(self.name, self.rate, self.hours)

    def __str__(self):
        return "{} ${:4.2f}".format(self.name, self.rate*self.hours)

def read_employees(file_name):
    employees = {}
    with open(file_name) as employees:
        for line in employees:
            *name, rate, hours = line.split()
            name = "{} {}".format(name)
            rate, hours = map(float, [rate, hours])
            if name in employees:
                raise ValueError("Employee with name {} already exists!".format(name))
            employees[name] = Employee(name, rate, hours)
    return employees

if __name__ == "__main__":
    employees = read_employees('employees.txt')
    name = input('Give me an employee to search for: ')
    print(employees[name])

This code will raise a KeyError if the name does not exist, To prevent this you can use this:

if __name__ == "__main__":
    employees = read_employees('employees.txt')
    name = input('Give me an employee to search for: ')
    try:
        print(employees[name])
    except KeyError:
        print('Employee is not listed')

Or even this to keep on asking until a valid name is given:

if __name__ == "__main__":
    employees = read_employees('employees.txt')
    name = input('Give me an employee to search for: ')
    while name not in employees:
        print('Employee is not listed, try again')
        name = input('Give me an employee to search for: ')
    print(employees[name])

This more modular design allows you now to easily implement a write_employees(employees, file_name) function to write the dict back to a file, delete employees (del employees[to_delete]), modify them (employees[name].rate = 10), etc.

\$\endgroup\$
  • \$\begingroup\$ EDIT: Nevermind my else statement was in the wrong place. \$\endgroup\$ – ByteSettlement Jul 12 '16 at 19:20
  • \$\begingroup\$ Yes that looks a lot better with classes, but I haven't learned classes yet and I'm excited to, thank you \$\endgroup\$ – ByteSettlement Jul 12 '16 at 19:36
  • \$\begingroup\$ @ByteSettlement Well, sounds like a good time to learn about them :) \$\endgroup\$ – Graipher Jul 12 '16 at 19:41
  • \$\begingroup\$ can you explain the purpose of 'map' and why you used it in this situation? \$\endgroup\$ – ByteSettlement Jul 12 '16 at 19:54
  • \$\begingroup\$ I think it makes it more succinct. But you could also use this: rate, hours = [float(x) for x in [rate, hours]] or even just: rate, hours = float(rate), float(hours), which is basically what you already used. \$\endgroup\$ – Graipher Jul 12 '16 at 19:58
1
\$\begingroup\$

Graipher's answer covers many good points (which I won't repeat), but you can also simplify the file parsing by using the csv module provided by the Python standard library:

with open('employees.txt', newline='') as file:
    reader = csv.reader(file, delimiter=' ')
        for line in reader:
            # use `line` as normal
\$\endgroup\$
  • \$\begingroup\$ Yeah, this is also nice if you want to keep using the line[0], line[1]... syntax. I couldn’t resist the nice tuple unpacking features of python3, though. \$\endgroup\$ – Graipher Jul 12 '16 at 19:52

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.