2
\$\begingroup\$

I had this as a project last school year and decided to try and optimize it. I'm curious to see how much I have learned; is this a good way of doing it?

import os
dict = {
#   "empId": ["title", name]
    "000001": ["ceo", "joe", "smith"],
    "000002": ["co", "bob", "konn"],
    "000003": ["lead designer", "koss", "klan"],
 
}
dict2 = {
    
}
 
def searchEmpId(empID):
    key = empID
    if key in dict:
        print(" Name: {} {}\n Position: {}\n Employee ID: {}".format(dict[key][1], dict[key][2], dict[key][0], key))
    print("\n")
    input("Press enter to return to main menu")
    menu()
 
def searchName(Name):
    for x in range(1, len(dict)):
        key = str("0"*(6-len(str(x)))) + str(x)
        if Name in dict[key]:
            print(" Name: {} {}\n Position: {}\n Employee ID: {}".format(dict[key][1], dict[key][2], dict[key][0], key))
    print("\n")
    input("Press enter to return to main menu")
    menu()
 
def add(Position, First, Last):
    dict.update({str(str("0"*(6-len(str(len(dict)+1)))) + str(len(dict)+1)): [Position, First, Last]})
    print("\n")
    input("Press enter to return to main menu")
    menu()
 
def reset():
    dict2.update(dict.copy())
    dict.clear()
    for x in range(0, len(dict2)):
        res = list(dict2.keys())[0]
        add(dict2[res][0], dict2[res][1], dict2[res][2])
        dict2.pop(res)
 
def remove(empID):
    dict.pop(empID)
    reset()
    print("\n")
    input("Press enter to return to main menu")
    menu()
 
def listDict():
    os.system("clear")
    for x in range(1, len(dict)+1):
        key = list(dict.keys())[x-1]
        print(" Name: {} {}\n Position: {}\n Employee ID: {} \n".format(dict[key][1], dict[key][2], dict[key][0], key))
    input("Press enter to return to main menu")
    menu()
 
def menu():
    os.system("clear")
    print("Welcome to the employee dictionary")
    print('\n To search for an employee by id type "1" ')
    print(' To search for an employee by name type "2" ')
    print(' To add an employee type "3" ')
    print(' To remove an employee type "4" ')
    print(' To list the dictionary type "5" \n')
    selection = input(">>  ")
    if selection == "1":
        os.system("clear")
        searchEmpId(str(input("Employee ID: ")))
    if selection == "2":
        os.system("clear")
        searchName(str(input("First name or last name(but not both): ")))
    if selection == "3":
        os.system("clear")
        add(str(input("Position: ")), str(input("First name: ")), str(input("Last Name: ")))
    if selection == "4":
        os.system("clear")
        remove(str(input("Employee ID: ")))
    if selection == "5":
        listDict()
menu()
\$\endgroup\$
0

2 Answers 2

5
\$\begingroup\$

Starting with the last line:

menu()

Standard practice is to put this in a "main guard", so that the file is usable from other programs:

if __name__ == '__main__':
    menu()

This bit looks fragile:

    key = str("0"*(6-len(str(x)))) + str(x)

There's an assumption there that all employee ids are 6 digits (and that's a convoluted way to format - much simpler would be key = f'{x:06u}').


Recursively calling menu() as the last action of each user function means that we constantly recurse into that function. Simply looping is more memory-efficient.


        os.system("clear")

That's risky. We're using whatever PATH we inherited, which might not include a clear command - or worse, it might include one that does something completely unexpected.


What's the reset() function for? It seems to replace dict with a copy of itself, but changing the employee numbers. I don't think that's something you would really want to do in a real organisation where they are used as identifiers in databases such as payroll, expenses, access tokens, etc. If it really is necessary, perhaps a better name might be rekey()? Even then it ought to have a comment explaining what it's for.


if selection == "1":
    ⋮
if selection == "2":
    ⋮
if selection == "3":
    ⋮
if selection == "4":
    ⋮
if selection == "5":
    ⋮

This kind of switch/case is normally represented in Python using an action dictionary. If you really want to use if, it's better to use elif to skip the conditions following the match.


for x in range(1, len(dict)+1):
    key = list(dict.keys())[x-1]

Why offset everything by 1? We could as easily write for x in range(0, len(dict) and then simply use x rather than x-1. But in any case for x in range() is a code smell; here we just want to iterate over all of dict's elements:

for key,value in dict.items():
    print(f" Name: {value[1]} {value[2]}\n Position: {value[0]}\n Employee ID: {key} \n")

This is a partial review; I ran out of time here.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ "Recursively calling menu() as the last action of each user function means that we constantly recurse into that function." I killed one of my first projects by using recursion when I should have used loops. I'd recommend you, Histic, to really listen to this one so you don't make the same mistake I did. \$\endgroup\$
    – Peilonrayz
    Jul 27, 2021 at 20:57
  • \$\begingroup\$ the reset is for when you remove someone the number are still all in order to instead of 000001 000003 and such but if the real world i suppose you wouldn't do that but just did that for this ill take into account useing a loop instead that actually makes more sense now that you said that and the key part ive nerver seen that so thanks and i was using clear as on linux its just the terminal clear command and dont know a better solution then just printing "\n" a hundred times which seems like a worse idea \$\endgroup\$
    – Histic
    Jul 27, 2021 at 21:15
  • \$\begingroup\$ Really, for platform-independent and terminal-independent screen handling, you'll want to be using Python's curses module. Clearing the screen is then done using curses.window.clear(). \$\endgroup\$ Jul 28, 2021 at 5:58
  • \$\begingroup\$ Ah, the reset() method does renumbering - perhaps that suggests a better name for the function? It does seem an odd thing to do, as the employee number is (in database terms) the primary key, so changing it would break every employee reference in the company (e.g. payroll, access tokens, .... - it's a long list!) \$\endgroup\$ Jul 28, 2021 at 6:49
  • \$\begingroup\$ yea i had just did it as to see if i can but yeah it wouldnt be done in the real world \$\endgroup\$
    – Histic
    Jul 28, 2021 at 16:30
2
\$\begingroup\$

I agree with Toby Speight on recursion, id and reset issues, but also I should mention:

Name consistency

PEP8 recommends snake_lower_case for variable and function names. Of course, you can use something else, like lowerCamelCase for variables and functions and UpperCamelCase for arguments; but don't mix styles, you're puzzling the reader - and yourself in a first place. Position and empID shouldn't be used together.

os.system("clear") is bad

First, it works only on some OSes (in Windows it should be os.system("cls")). Second, it has a security flaw mentioned by Toby Speight. Third, it uses some very complex OS mechanics - more complex than all other code here. My advice: if you want to make a beautiful cross-platform application, you should use GUI; if you want to make it console, use curses; and if you're only learning - just accept that it would be a bit ugly and add two new lines instead of os.system("clear").

Group same code together (and get rid of it)

All selections call os.system("clear") before actual work, mostly on menu(), but listDict has it inside; all of them prompt input("Press enter to return to main menu") after work. You can call os.system("clear") once before ifs and, after you get read of recursion, prompt for enter once after ifs.

dict is a built-in name

Don't redefine built-ins if you aren't sure what are you doing. Rename dict into something else - like employees.

dict2 is used only in one function

Declare it inside that function as a local variable, don't clog the global namespace.

Use proper collections

list is intended to keep many values, processed in a uniform way. A group of several values of different meaning should be kept in a tuple with namedtuple, object and even dict for more advanced ways to store data like that. I don't see anything like

for s in dict[0]:
    process(s)

in the code, but you're addressing different parts of employee data in different manner. It should be a tuple:

dict = {
    "000001": ("ceo", "joe", "smith") ...

Make use of loops over collections and destructuring

Check out this way of listing all employees (assuming using tuples and renaming dict):

for id, (position, first, last) in employees.items():
    print(" Name: {} {}\n Position: {}\n Employee ID: {} \n".format(first, last, position, id))

or with f-strings:

for id, (position, first, last) in employees.items():
    print(f" Name: {first} {last}\n Position: {position}\n Employee ID: {id}\n")
\$\endgroup\$
7
  • \$\begingroup\$ Good continuation of my answer - thank you! \$\endgroup\$ Jul 28, 2021 at 8:49
  • \$\begingroup\$ for this part of your response for id, (position, first, last) in employees.items(): print(" Name: {first} {last}\n Position: {position}\n Employee ID: {id}\n") im confused what id is suppose to equal cause it just prints out {first} instead of the name also whats wrong with position and empID or is it just because of imporper naming \$\endgroup\$
    – Histic
    Jul 28, 2021 at 17:13
  • \$\begingroup\$ also im not sure about do gui or curses but if i were to go with gui what do you recomend \$\endgroup\$
    – Histic
    Jul 28, 2021 at 17:42
  • \$\begingroup\$ dict.items() returns tuples of (key, value). RTM. \$\endgroup\$ Jul 28, 2021 at 19:02
  • \$\begingroup\$ My recommendation is not to go for beauty while learning basics. \$\endgroup\$ Jul 28, 2021 at 19:03

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.