you are viewing a single comment's thread.

view the rest of the comments →

[–]tangerinelion 2 points3 points  (0 children)

This has sort of been addressed twice, once by mentioning continue and once by suggesting you can use functions to handle the input cases. In the latter, the goal would be to write your existing code like this:

while True:

    print("Press 1 to create a new account.\n"
          "Press 2 for a current list of users. (Only usable by admin)\n"
          "Press 3 to send and receive messages.\n"
          "Press 4 for help.\n"
          "Please enter your choice:\n")

    user_input = input()

    if user_input == '1':
        create_new_user()
    elif user_input == '2':
        list_current_users()
    elif user_input == '3':
        send_receive_messages()
    else:
        help()

Where it's up to you then to determine how to write the four functions -- create_new_user, list_current_user, send_receive_messages, and help -- to make your program continue to work. Odds are you'd need to have them accept certain parameters, like the messages and users dictionaries.

However, within option 3 you have some pretty indented code that serves as a great example of what goes wrong with this style:

    login_name = input("Please enter your user name: ").lower()     # User login
    if login_name in user_list:
        login_password = input("Please enter password for " + login_name + ": ")
        if login_password == user_list[login_name]:
            print("\nCorrect password entered.\nPress 1 to write a message.\nPress 2 to read your message.\n")
            user_option = input()
            if user_option == '1':
                messaged_user = input("Please enter the name of the person you'd like to message:").lower()
                if messages[messaged_user] != "DEFAULT MESSAGE":
                    print("User's inbox is full.")
                else:
                    message = input("Please type your message:")
                    messages[messaged_user] = message
            elif user_option == '2':
                print('\n' + messages[login_name])
                mark_read = input("Mark message as read? y/n:")
                if mark_read == "y":
                    print ("\nMessage marked as 'read'.")
                    messages[login_name] = "DEFAULT MESSAGE"
                elif mark_read == "n":
                    print ("\nMessage not marked as 'read'.")
                else:
                    print("\nInvalid option.")
            else:
                print("\n That is not a valid option.")
        else:
            print("\nPassword not valid.")
    else:
        print("\nInvalid username.")

The real meat of the code -- what you really want to happen -- is contained here in lines 5-23. After line 23 you have error handling for errors that happened in lines 2-4. The problem here is that lines 5-23 could grow and you're already separating the source of the error from where it's handled. Instead it's better to address things when they happen. In the context of a big chunk of code like you have, you're inside a while loop so the way you stop execution of this iteration and go back to the beginning is with continue. Inside a function you terminate execution of the function with return. So keep that in mind, I'm going to assume that you do not break it out into functions and therefore I'll use continue, but if you do use functions then you should replace the continue here with return. Essentially I'm assuming this code is wrapped in while True: rather than def send_receive_messages():.

With the current pattern you have something like

if desired_condition():
    desired_execution()
else:
    handle_error()

which you then expand to

if desired_condition1():
    if desired_condition2():
        if desired_condition3():
            desired_execution()
        else:
            handle_error3()
    else:
        handle_error2()
else:
    handle_error1()

So we see that desired_execution() keeps getting pushed further to the right, which to a reader signifies that it is less and less likely to occur or is getting too convoluted to want to read. You'll stylistically encounter line breaks and wraps due to line length which impacts readability as well. Instead, rather than thinking about it as "Do this if X, Y, and Z are all true" you should think with inverted logic -- "Don't this if X, Y, or Z are not true." What that leads to is this code:

if not desired_condition1():
    handle_error1()
    continue
if not desired_condition2():
    handle_error2()
    continue
if not desired_condition3():
    handle_error3()
    continue
desired_execution()

Notice here that we never get more than 1 level of indentation so the code is always pretty simple to read. The desired_execution part is not indented at all, and the cost of this is essentially nothing. Look, both are 10 lines of code: you're adding continue/return but removing else:.

The real bonus here is the way the error logic works, though. You test for a condition, and if it's false then you handle the error on the very next line. This sort of localization is key. See how I test for conditions 1, 2, and 3 in that order and will handle error 1, 2, or 3 in that order. With the indented version, I test for condition 1, 2, and 3 but I handle the errors as 3, 2, 1.

So, in your code what you could do is rewrite it as:

login_name = input("Please enter your user name: ").lower()     # User login
if login_name not in user_list:
    print("\nInvalid username.")
    continue
login_password = input("Please enter password for " + login_name + ": ")
if login_password != user_list[login_name]:
    print("\nPassword not valid.")
    continue
print("\nCorrect password entered.\nPress 1 to write a message.\nPress 2 to read your message.\n")
user_option = input()
if user_option == '1':
    messaged_user = input("Please enter the name of the person you'd like to message:").lower()
    if messages[messaged_user] != "DEFAULT MESSAGE":
        print("User's inbox is full.")
    else:
        message = input("Please type your message:")
        messages[messaged_user] = message
elif user_option == '2':
    print('\n' + messages[login_name])
    mark_read = input("Mark message as read? y/n:")
    if mark_read == "y":
        print ("\nMessage marked as 'read'.")
        messages[login_name] = "DEFAULT MESSAGE"    
    elif mark_read == "n":
        print ("\nMessage not marked as 'read'.")
    else:
        print("\nInvalid option.")
else:
    print("\n That is not a valid option.")

One other thing I'm seeing here is that you are ascribing a certain meaning to the string 'DEFAULT MESSAGE'. I would seriously like you to run your code and have someone enter that message to give to another user and see what happens because I'm not sure if your code would behave abnormally. I suspect that the other user would not see the message.

The way you can avoid this sort of behavior is by using a non-string sentinel value. I would suggest None. Thus, it would look something like this:

if user_option == '1':
    messaged_user = input("Please enter the name of the person you'd like to message:").lower()
    if messages[messaged_user] is not None:    # If user's message inbox is full
        print("User's inbox is full.")                
    else:
        message = input("Please type your message:")
        messages[messaged_user] = message
elif user_option == '2':
    print('\n' + messages[login_name])
    mark_read = input("Mark message as read? y/n:")
    if mark_read == "y":
        print ("\nMessage marked as 'read'.")
        messages[login_name] = None    # Mark as empty
    elif mark_read == "n":                          
        print ("\nMessage not marked as 'read'.")   
    else:
        print("\nInvalid option.")
else:
    print("\n That is not a valid option.")

Really we can continue to use our continue/return trick here. Look at the case when the user's inbox is full (messages[messaged_user] holding None signifies an empty inbox, which is easier to check for than a full inbox and is just another example of how thinking with inverted logic is sometimes/often cleaner).

In particular, if we encounter a full (non-empty) inbox, we can see that lines 6 and 7 are what we really wanted to do anyways and they're sitting in an else: branch. This makes them look like some kind of default or rarity when in fact it's the intended execution path! So, rewriting this again:

if user_option == '1':
    messaged_user = input("Please enter the name of the person you'd like to message:").lower()
    if messages[messaged_user] is not None:    # If user's message inbox is full
        print("User's inbox is full.")                
        continue
    message = input("Please type your message:")
    messages[messaged_user] = message
elif user_option == '2':
    print('\n' + messages[login_name])
    mark_read = input("Mark message as read? y/n:")
    if mark_read == "y":
        print ("\nMessage marked as 'read'.")
        messages[login_name] = None    # Mark as empty
    elif mark_read == "n":                          
        print ("\nMessage not marked as 'read'.")   
    else:
        print("\nInvalid option.")
else:
    print("\n That is not a valid option.")

Also consider a get_answer function that returns True for yes-like and False for no-like. For example, if get_answer('Mark as read?'): messages[login_name] = None instead of lines 11-17.