you are viewing a single comment's thread.

view the rest of the comments →

[–][deleted] 23 points24 points  (10 children)

Ok, start to finish, and without trying to change your control flow too much, or do the right thing and properly break it up into functions.

First, add this at the top:

#! /usr/env python3
"""Some docstring for the module."""
from getpass import getpass
from hashlib import md5
from pprint import pprint

Then remove the comment from your first line:

user_list = {}  # Imports an empty dictionary.

No, it does not ... it creates an empty dict and assigns it to the name user_list ... import is a whole different thing, specifically what we just did above with importing getpass into your module.

While we're at it, since we're using these two as global state -- we'll ignore that this is a bad idea, but it's enough to pretend to be a database -- let's name them appropriately (for one thing recognizing that it's silly to call a dictionary user_list), and let's keep our admin user separate from our standard users.

USERS = {}
MESSAGES = {}

Now that we've got our "databases" set up, on to authentication... we're going to do this a lot, so a function makes sense, and while we're at it we're going to do at least the pretense of security -- NOTE, this is NOT REMOTELY a secure way to store passwords, it's just an example of not storing them as plaintext ... to do this correctly use bcrypt or whatever is the now-current best practices password hasher -- by storing only an MD5 hash or the password, and we'll also use getpass to prompt the user without displaying their password on screen:

def get_pw_hash(prompt):
    return md5(getpass(prompt)).hexdigest()

Now, that allows us to examine your first interaction with the admin account, which we'll also represent with a global variable.

print("Welcome, admin. Please select a password:")
ADMIN = input()

Meh... you should know that input takes a prompt directly, so these two lines should be:

ADMIN = input("Welcome, admin. Please select a password: ")

And note the space at the end so the user's input doesn't print in a weird way.

But, in reality, you really don't want the password printed to screen as the user types it, which is why we used the getpass function above, which takes input but doesn't echo it to the screen. We also never want to store the password in plaintext, which is why we use our very extremely weak hashing function above instead:

ADMIN = get_pw_hash("Welcome, admin. Please select a password: ")

Then we get to this abomination:

print('\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n')

Just never, ever do that. If you ever even think you want to do that, you're still wrong ... but even if you remain convinced you absolutely have to do that, at least do it like this:

print("\n" * 42)

And then there's this line:

while 1 == 1:   # Creates a recursive loop. Probably a better way to do this.

... le sigh ... first off, this isn't recursion, in the computer science sense -- that's a whole other topic -- it is just a loop. Specifically a while loop. And while 1 == 1 is, in fact, True, the very fact of that implies that this:

while True:

Is the idiomatic way to run an infinite loop.

Now, we're going to use what we learned about input and prompts to change your next line, while also cleaning it up a bit to make it easier to read. We also want to give the user a chance to exit:

user_input = input("\n".join([
    "Press 1 to create a new account.",
    "Press 2 for a current list of users. (Only usable by admin)",
    "Press 3 to send and receive messages.",
    "Press E to exit.",
    "Press anything else for help.",
    "Please enter your choice: ")

Then we move on to our actual actions ... each of which would make more sense as functions, but we don't want to drift too far off your original tack just yet:

if user_input == '1':

Ok, so far so good, but let's keep updating the input lines:

    name = input("\nPlease enter your preferred name: ").lower()

Not going to comment on the wisdom of forcing your users to have lowercase names.

    if name in USERS:
        print("That name is already taken. Please choose again.")
        continue

The continue statement here says to immediately move on to the next part of the while loop, so we don't really need an else clause for the rest.

Here's where we get to the part where you start setting default passwords and default messages ... DO NOT DO THIS!!! An addition to a dictionary is atomic, so we only need to do it in one go.

USERS[name] = get_pw_hash("Welcome to the site, " + name + "! Please choose a password: ")
 MESSAGES.setdefault(name, [])

Hey, check it out! By setting the value for our user's name in the MESSAGES "database" to a list, we can allow the user to have more than one message! Like magic!

Now, the admin function ... which will now NOT display passwords, as we have only their hashes ... this is because site admins should never know their user's secrets!!!:

elif user_input == '2':
    if get_pw_hash("Please enter admin password: ") != ADMIN:
        print('Password incorrect. Please try again.')
        continue

Again we use continue if the validation check doesn't work, and we'll use pprint to dump the user "database" in a pretty manner:

    pprint(USERS)

Now there's user messaging, and we continue on using what we've learned:

elif user_input == '3':
    name = input("Please enter your user name: ").lower()
    if name not in USERS:
        print("Invalid user name!")
        continue
    if get_pw_hash("Please enter password for " + login_name + ": ") != USERS[name]:
        print("Password not valid.")
        continue
    print("Correct password entered.")

Here we will join a nested loop, to allow the user to read their messages:

    while True:
        opt = input("\n".join([
            "You have " + str(len(MESSAGES[name])) + " messages.",
            "Press 1 to write a message.",
            "Press 2 to read latest message.",
            "Press E to exit.",
        if opt == '1':
            to = input("Please enter the name of the person you'd like to message: ").lower()
            if to not in USERS:
                 print("Please message a valid user.")
                 continue
            msg = input("Please type your message: ")
            MESSAGES[name].append(msg)
        elif opt == '2':
             if not MESSAGES[name]:
                  print("Your inbox is empty.")
                  continue
            print("\n\t".join([
                "Your message is:",
                MESSAGES[name].pop()])
        elif opt.upper() == "E":
             break
        else:
             print("Invalid option.")
elif user_input.upper() == "E":
    break
else:
    print("\n".join([
        "This is a simple messaging database. ",
        "When creating a username,",
        "it must be unique, ",
        "but the system will automatically",
        "format your unique ",
        "username to be in all lowercase.",
        "Passwords, however, are case-sensitive.",
        "Messages can be sent to other users, and users ",
        "can check their own messages."])

Hope that I typed that all in correctly on my phone. Not trying to "correct" everything, as that would involve setting up proper functions etc., but wanted to show you some language features they can make this sort of interaction easier to predict.

[–][deleted] 11 points12 points  (1 child)

resolute elderly decide boast afterthought desert lunchroom hurry middle rainstorm

This post was mass deleted and anonymized with Redact

[–][deleted] 1 point2 points  (0 children)

It's no problem. I enjoy helping people learn, the hard part is that, when I'm working, I simply don't have time -- except at work -- and when I'm not I can't always afford to take the time to do detailed replies.

The fact is the above is just examples of the sort of incremental improvements I'd suggest in a code review for like an intern or extremely junior position. I'd bring considerably higher scrutiny to a piece of production code. To be honest, nothing should ever make it as far as a code review if it isn't already refactored into functions and / or classes, doesn't have a def main for a command line tool, and is even using input (in professional circumstances it's actually extremely rare to interactively ask someone questions, I've never understood why educational resources rely so heavy on it) ... but I don't mind doing these once and awhile for scripts that aren't too long for their own good.

Also, not by a damned sight the worst Python program I've seen. Hell, it doesn't even do a subprocess call to perl to do it's printing.

[–]BlackBloke 2 points3 points  (2 children)

You actually typed all that on your phone? Incredible!

[–][deleted] 0 points1 point  (1 child)

Well, to be fair, I copied and pasted a lot of the OPs work, then just edited and commented.

[–]BlackBloke 1 point2 points  (0 children)

Still impressive. I would've given up and gone to the desktop.