5
\$\begingroup\$
#loop to get input that is not blank, strips input of whitespace
def ckin(a):    
    while a=='':
        a=input().strip()
    return  a

#creates list with includes user admin
user_names = ['admin']  

#Ask for 5 names to store as usernames
print('Please enter 5 user names: ')    

#if username is valid, add to list
for i in range(0,5) 
    lib_name=input().strip()
    lib_name=ckin(lib_name)
    user_names.append(lib_name)

#asks for login name, must be valid
print('Enter login name:')  
log_name=input().strip()
log_name=ckin(log_name)

#Create a test list that is lower case to compare login
#against without altering the cases used by user to creat username list

test=[name.lower() for name in user_names] 

#if login name not in list continue to prompt
while log_name.lower() not in test:
    print('Enter login name:')
    log_name=input().strip()
    log_name=ckin(log_name)

#find login name in test, then return either response for admin, or login    name using 
#case formatting that original used for original entry
for i in range(len(test)):
    if log_name.lower() == 'admin':
        print('Hello admin, would you like to see a status report?')
        break
    elif log_name.lower() == test[i]:
        print('Hello '+user_names[i]+', thank you for logging in again')
        break

Looking for general review self-teaching, Day 4

I haven't learned much but I keep attempting to do what I'm learning with user inputs instead of set entries

My goal here was to be able to create a list that keeps the original entry cases to be used for responding. I created a second list in lower cases to compare to and then responded using the cases the entries were stored as.

Any comments are greatly appreciated!

\$\endgroup\$
0

2 Answers 2

8
\$\begingroup\$

You can reduce your code duplication a bit. First, the way you are currently using the ckin function is like this:

name = input("Enter a name").strip()
name = ckin(name)

Wouldn't it be nicer to be able to do:

name = ask_user("Enter a name: ")

For this, just modify your function like this:

def ask_user(message=""):
    user_input = ""
    while not user_input:
        user_input = input(message).strip()
    return user_input

Note that I renamed it (to make it clearer what it does), used the fact that empty strings are falsy and added whitespace around the = operator, as recommended by Python's official style-guide, PEP8.


Going further, you should get into the habit of defining clearly named functions for separate things.

One such concern is getting a pre-populated username list. Currently this list is populated with user input, but this might change in the future. So it would be nice to bundle that part all together in one function:

def get_users():
    # creates a users set, which always includes admin
    users = {'admin'}

    #Ask for 5 names to store as usernames
    print('Please enter 5 unique user names: ')
    while len(users) < 6:
        users.add(ask_user("Enter a valid user name: ").lower())
    return users

Note that I made the datastructure a set here, which gives you \$\mathcal{O}(1)\$ time complexity for in tests. I also made sure that there are actually 5 + 1 users (in other words, user names need to be unique). I also put the str.lower transformation here so we don't need to do it later and only store the canonical user name.


Next is a login function, that asks for a username until a valid one is entered:

def login(users):
    user_name = None
    while user_name not in users:
        user_name = ask_user("Enter login name: ").lower()
    return user_name

And then, the greeting. Instead of iterating over all users until you reached the user that is currently logging in, use the fact that we already know that the user is in users (since we explicitly checked for that in login). So this becomes a simple if..else check:

def print_greeting(user_name):
    if user_name == 'admin':
        print('Hello admin, would you like to see a status report?')
    else:
        print(f'Hello {user_name}, thank you for logging in again')

Here I used an f-string, as introduced in Python 3.6, to make the string formatting a bit easier.


And finally, tying it all together in a main function:

def main():
    users = get_users()
    user_name = login(users)
    print_greeting(user_name)

if __name__ == "__main__":
    main()

Note that I call this main function only under a if __name__ == "__main__": guard. This ensures that the main function does not run if you import from this script in another script.

\$\endgroup\$
12
  • \$\begingroup\$ Thank you this is exciting! This is my first time seeing the while not and I sort of grasp the idea of main here, definitely going to review this and integrate these concepts. \$\endgroup\$ Commented Aug 22, 2018 at 14:52
  • \$\begingroup\$ could you breakdown the use of ="" in this portion of the code, I understand what is being done, checked, and returned just not what is happening with the ="" in the ask_user function \$\endgroup\$ Commented Aug 22, 2018 at 21:42
  • \$\begingroup\$ @anthonyvalva: It just sets user_input to a value where the while loop's condition is met, so the user is asked for the first time. I also could have put user_input = input(message).strip() there as well, but that would then be repeated (so if you ever e.g. want to change that the user input is not only stripped, but also lowercased, you would need to remember to change it in both places, making it a bit harder to maintain). \$\endgroup\$
    – Graipher
    Commented Aug 22, 2018 at 21:44
  • \$\begingroup\$ I'm sorry I was actually reffering to the message='' portion what does this do? \$\endgroup\$ Commented Aug 22, 2018 at 21:53
  • 1
    \$\begingroup\$ @anthonyvalva The message='' default keyword argument is used as input prompt in the call of the input() built-in instead of using a print() statement beforehand. \$\endgroup\$ Commented Aug 23, 2018 at 8:38
7
\$\begingroup\$

General

Read and familiarize yourself with

  • PEP8
  • Functions
  • The if __name__ == '__main__': check

Make sure, the function and variable names convey their purpose.

Specific

The user input check could be done a little more pythonic:

def get_user_name():
    user_name = None
    while not user_name:
        user_name = input().strip()
    return user_name

And thus, the first manual reading can be omitted:

for i in range(5):
    # lib_name = input().strip()  # Useless
    lib_name = get_user_name()
    user_names.append(lib_name)

Same here:

print('Enter login name:')  
log_name = get_user_name()

For quicker lookup of data, use a hasing container:

known_user_names = frozenset(name.lower() for name in user_names)

#if login name not in list continue to prompt
while log_name.lower() not in known_user_names:
    print('Unknown user. Enter valid login name:')  # Add an error hint to the user.
    log_name = get_user_name()

The loop here is useless:

for i in range(len(test)):
    if log_name.lower() == 'admin':
        print('Hello admin, would you like to see a status report?')
        break
    elif log_name.lower() == test[i]:
        print('Hello '+user_names[i]+', thank you for logging in again')
        break

Above you already made sure, that the user name is known.

if log_name.lower() == 'admin':
    print('Hello admin, would you like to see a status report?')
else:
    print('Hello', log_name + ', thank you for logging in again')
\$\endgroup\$
2
  • \$\begingroup\$ going to review all this and applyt it, greatly appreciate your time! \$\endgroup\$ Commented Aug 22, 2018 at 14:31
  • \$\begingroup\$ Maybe wait for some other reviews first. \$\endgroup\$ Commented Aug 22, 2018 at 14:32

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.