9
\$\begingroup\$

This is a registration and login program I made in Python that runs through the terminal. I am new to programming so I didn't have any actual use for this, I simply made it for practice. Please give me tips on minimizing code and making things shorter, point out any bad programming practices I used here and how I can change them and look out for them next time.

# Import modules
import time

# All accounts
users = {
    "root": {
        "password": "gucci-mane",
        "group": "admin",
        "mail": []
    }
}

# Form validation
def validate(form):
    if len(form) > 0:
        return False
    return True

# Login authorization
def loginauth(username, password):
    if username in users:
        if password == users[username]["password"]:
            print("Login successful")
            return True
    return False

# Login
def login():
    while True:
        username = input("Username: ")
        if not len(username) > 0:
            print("Username can't be blank")
        else:
            break
    while True:
        password = input("Password: ")
        if not len(password) > 0:
            print("Password can't be blank")
        else:
            break

    if loginauth(username, password):
        return session(username)
    else:
        print("Invalid username or password")

# Register
def register():
    while True:
        username = input("New username: ")
        if not len(username) > 0:
            print("Username can't be blank")
            continue
        else:
            break
    while True:
        password = input("New password: ")
        if not len(password) > 0:
            print("Password can't be blank")
            continue
        else:
            break
    print("Creating account...")
    users[username] = {}
    users[username]["password"] = password
    users[username]["group"] = "user"
    users[username]["mail"] = []
    time.sleep(1)
    print("Account has been created")

# Send mail
def sendmail(username):
    while True:
        recipient = input("Recipient > ")
        if not len(recipient) > 0:
            print("Recipient can't be blank")
            continue
        elif recipient not in users:
            print("There is no account with that username")
            continue
        else:
            break
    while True:
        subject = input("Subject > ")
        if not len(subject) > 0:
            print("Subject can't be blank")
            continue
        else:
            break
    while True:
        context = input("Context > ")
        if not len(context) > 0:
            print("Context can't be blank")
        else:
            break
    print("Sending mail...")
    users[recipient]["mail"].append(["Sender: " + username, "Subject: " + subject, "Context: " + context])
    time.sleep(1)
    print("Mail has been sent to " + recipient)

# User session
def session(username):
    print("Welcome to your account " + username)
    print("Options: view mail | send mail | logout")
    if users[username]["group"] == "admin":
        print("")
    while True:
        option = input(username + " > ")
        if option == "logout":
            print("Logging out...")
            break
        elif option == "view mail":
            print("Current mail:")
            for mail in users[username]["mail"]:
                print(mail)
        elif option == "send mail":
            sendmail(username)
        elif users[username]["group"] == "admin":
            if option == "user mail":
                print("Whos mail would you like to see?")
                userinfo = input("> ")
                if userinfo in users:
                    for mail in users[userinfo]["mail"]:
                        print(mail)
                else:
                    print("There is no account with that username")
            elif option == "delete mail":
                print("Whos mail would you like to delete?")
                userinfo = input("> ")
                if userinfo in users:
                    print("Deleting " + userinfo + "'s mail...")
                    users[userinfo]["mail"] = []
                    time.sleep(1)
                    print(userinfo + "'s mail has been deleted")
                else:
                    print("There is no account with that username")
            elif option == "delete account":
                print("Whos account would you like to delete?")
                userinfo = input("> ")
                if userinfo in users:
                    print("Are you sure you want to delete " + userinfo + "'s account?")
                    print("Options: yes | no")
                    while True:
                        confirm = input("> ")
                        if confirm == "yes":
                            print("Deleting " + userinfo + "'s account...")
                            del users[userinfo]
                            time.sleep(1)
                            print(userinfo + "'s account has been deleted")
                            break
                        elif confirm == "no":
                            print("Canceling account deletion...")
                            time.sleep(1)
                            print("Account deletion canceled")
                            break
                        else:
                            print(confirm + " is not an option")
                else:
                    print("There is no account with that username")
        else:
            print(option + " is not an option")

# On start
print("Welcome to the system. Please register or login.")
print("Options: register | login | exit")
while True:
    option = input("> ")
    if option == "login":
        login()
    elif option == "register":
        register()
    elif option == "exit":
        break
    else:
        print(option + " is not an option")

# On exit
print("Shutting down...")
time.sleep(1)
\$\endgroup\$
7
\$\begingroup\$

In your validate function, instead of doing a conditional, returning True if is passes and False if it does not, just return the conditional that you are checking.

Here is what I mean:

return len(form) > 0

This will return True if the expression evaluates to True, and False if it evaluates to False. No conditional needed.

Or, if you wish to further simplify this function, just return the length of form. Python treats 0 as False so if the length is 0, it will act as False in a conditional.

At that point, it may not even be worth it as a function.


In your login function, you wrote this in your first if statement:

not len(username) > 0

Which looks almost exactly like your validate function, except for the not.

If you have the function validate, you might as well use it when you need it:

not validate(username)

You to be repeating something like this throughout your code:

if not len(username) > 0:
    print("Username can't be blank")
else:
    break

And the only thing that is really changing is this thing that can't be blank.

I recommend making a function so you can aren't repeating yourself as much.

Here is what I came up with

def cant_be_blank(form, name):
    if not len(form) > 0: # or, use `not validate`
        print(name + " can't be blank.")
        return False
    return True

Then, you can use the function like this:

if not cant_be_blank(username, "Username"):
    break

Add some documentation to your functions.

In your documentation, you should include things like what each parameter means (if there are any), what the return means (if there is any), and a short description of what the function does.

I wrote this for your loginauth function:

def loginauth(username, password):
     """
     * Confirms that the username exists and that the password is correct for that username
     * @param(str) username -- the username
     * @param(str) password -- the password
     * @return(bool) True -- if successful login
     * @return(bool) False -- if unsucessful logic (either username does not exist, or password is incorrect)
     """
    if username in users:
        if password == users[username]["password"]:
            print("Login successful")
            return True
    return False

Note the placement of the, as it is called, docstring.


On statements like this:

if not len(username) > 0:
    print("Username can't be blank")
    continue
else:
    break

I'm not sure how much of a difference this makes in efficiency (if it makes a difference in anything at all), but the else part is unnecessary.

If the conditional passes, continue will run and skip over everything else to go back to the top of the loop. If it does not run, it can just call down to break and exit the loop. No else needed.

So the above code snippet will become this:

if not len(username) > 0:
    print("Username can't be blank")
    continue
break # if the above conditional fails, execution will fall through to this

I recommend creating a class for users so information is more easily stored.

It doesn't have to be too complicated; just a simple storage of values:

class User:

    def __init__(self, username, password, group):
        self.username = username
        self.password = password
        self.group = group
        self.mail = []

Then, when inserting new Users into users, just do:

users[username] = User("john", "smith", "user")

In case in the future you make some mail parser, don't make it too complicated by storing the mail separated by strings ("Sender", "Subject", "Context"); store it in an object. Depending on what the user wrote, those strings can make messages really confusing.

Why not store mail in an object?

Here is what I came up with:

class Mail:

    def __init__(self, sender, subject, context):
        self.sender = sender.username # assuming you use the `User` class I wrote
        self.subject = subject
        self.context = context

    def to_string():
        print("Sender: %s\nSubject: %s\nContext: %s" % (self.sender, self.subject, self.context))

I threw in an extra to_string method to format the object's properties in a way that made sense to output.

This is not coding related, but I believe the correct word would be "content" rather than "context".


If you continue to work on this code, you are going to build up quite a big if/elif/else statement in your session function.

As an easier way to solve this problem, create a dictionary containing the name of the command and the function to call for that command.

Here is what I mean:

commands = {
    "logout": logout,
    "view mail": view_mail
    ...
}

Then, your session function can be reduced to:

if option in commands:
    return_code = commands[option]()
    if return_code == False:
        break
else:
    print(option + " is not an option")

Now, a command should return False if the session is supposed to stop after execution of the command.

Yes, before in your string of if/elif statements, you had a whole if block to if the user was in the group "admin".

Since you don't have this anymore, just create a function for it:

def is_user_admin(user):
    return user.group == "admin"
\$\endgroup\$
9
  • \$\begingroup\$ You should be using a docstring for your refactored version of the loginauth function. \$\endgroup\$ Jul 1 '15 at 23:19
  • \$\begingroup\$ @EthanBierlein I think I fixed is. Is it better now? \$\endgroup\$
    – SirPython
    Jul 1 '15 at 23:22
  • \$\begingroup\$ Almost, you just have to place it under the function declaration, and indent it. :) \$\endgroup\$ Jul 1 '15 at 23:23
  • \$\begingroup\$ @EthanBierlein Did I get it this time? \$\endgroup\$
    – SirPython
    Jul 1 '15 at 23:26
  • \$\begingroup\$ Yup! You got it now! :D \$\endgroup\$ Jul 1 '15 at 23:29
3
\$\begingroup\$

your program looks nice, and seems to follow most of PEP8.


There are two rather long lines, and so you may want to put some information onto the next line.

Limit all lines to a maximum of 79 characters.

users[recipient]["mail"].append(["Sender: " + username,
                                 "Subject: " + subject,
                                 "Context: " + context
                                 ])

This way it is easier to read. As I misread it at a glance.


I thought the following was kinda strange, so you may want to change:

users[username] = {}
users[username]["password"] = password
users[username]["group"] = "user"
users[username]["mail"] = []

# Into

users[username] = {
    "password": password,
    "group": "user",
    "mail": []
}

It stuck out, and is easier to understand at a glance.


I personally love the str.format function. Whilst you can't exploit all of its features, it's still nice. It's mostly something to look into.

print("Deleting " + userinfo + "'s mail...")

# Into

print("Deleting {}'s mail...".format(userinfo))

However for simple print('username: ' + username), it is not needed.


Whilst your comparison operators are good, they are not pythonic. Whilst it is correct, it is long-winded.

For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

if not len(username) > 0:
    print("Username can't be blank")
else:
    break

# Into

if username:
    break
print("Username can't be blank")

And the .lower and in functions are helpful when wanting to validate input.

confirm = input("> ")
if confirm == "yes":
    # ... do more

# Into

confirm = input("> ")
if confirm.lower() in ["yes", "y"]:
    # ... do more

This is so that you have both a shorthand, of 'y', but also you can put 'Yes', 'YEs', 'yEs', etc.


You should wrap the information in the global scope in if __name__ == "__main__":

if __name__ == "__main__":
    print("Welcome to the system. Please register or login.")
    # ...

This is so that if someone accidentally imports this file at a later date, it doesn't start an unintended program.


You can change your main loop into a dictionary lookup.

if option == "exit":
    break
try:
    {
        "login": login
        "register": register
    }[option]()
except KeyError:
    print(option + " is not an option")

This is as functions are first-class citizens in Python! This means that if we ask for login, it will tell us it's place in memory. But if we call it with () then it will execute. To see what I am on about, you can try this:

>>> def a():pass
...
>>> a
<function a at 0x7f9a7e7b3bf8>
>>> a()
>>> b = a
>>> b
<function a at 0x7f9a7e7b3bf8>
>>> b is a
True

Using a dictionary for lookup is a nice trick that you can do to make there be a lot of functions, and adding a new function is just adding another entry to the dictionary. But as this is a small program the if else statement is perfectly good.


give me tips on minimizing code and making things shorter

Well I don't want to write a book, so this will be the last thing I will comment on.

If you look at your program, you see that you use this structure a lot.

while True:
    username = input("Username: ")
    if not len(username) > 0:
        print("Username can't be blank")
    else:
        break

This can easily be changed to a function.

def get_nonempty(name):
    while True:
        user_input = input("{}: ".format(name))
        if user_input:
            return user_input
        print("{} can't be blank".format(name))


# Use

password = get_nonempty("Password")

This would cut down a lot of your code. And, if you were to build on your program, then you would use the yes/no question more. And you would want to change it into a function.

As this is a user input program, you need lots of print statements, and that is a lot of your program. And so it would be hard to reduce this more. Unless you remove print statements... Which, if you did, "\r\b" is nice!


The first two quotes are from PEP8, the Python style guide.

If you have any problems, just say. Hope this helps! Sorry for the light novel, I just wrote about the things that stuck out to me.

\$\endgroup\$
2
\$\begingroup\$

Some comments beyond what others have said

  • Your comments are useless.

    # Login
    def login():
    

    tells you nothing. The name of the function is already login.

    You're better off writing comments for blocks of code, particularly those that are not obvious from a first read, or for which a reader might ask, "why does this code do this?". The comments should be in plain English (read: complete sentences), and shouldn't insult the reader's intelligence.

  • Don't use input to read passwords. Use getpass, like

    password = getpass.getpass()

    (and put import getpass at the top of the file). This will read the password without echoing the characters to the terminal, so that if someone else is looking over the shoulder of the person typing they won't be able to see the password on the screen.

  • Add some empty space between logical blocks of code. As a simple example, sendmail is easier to read as

    def sendmail(username):
    while True:
        recipient = input("Recipient > ")
        if not len(recipient) > 0:
            print("Recipient can't be blank")
            continue
        elif recipient not in users:
            print("There is no account with that username")
            continue
        else:
            break
    
    while True:
        subject = input("Subject > ")
        if not len(subject) > 0:
            print("Subject can't be blank")
            continue
        else:
            break
    
    while True:
        context = input("Context > ")
        if not len(context) > 0:
            print("Context can't be blank")
        else:
            break
    
    print("Sending mail...")
    users[recipient]["mail"].append(["Sender: " + username, "Subject: " + subject, "Context: " + context])
    time.sleep(1)
    print("Mail has been sent to " + recipient)
    

    (along with the suggested changes from the other answers here of course).

  • Be consistent with your user prompts. Some use : and some use >.

  • Why are you calling time.sleep? It seems to be unnecessary, and only serves to slow down the program.

\$\endgroup\$
0
\$\begingroup\$

Beside everything that's being said by others already:

I think you have a security problem in your register function. You do check whether the username and password aren't blank, but you don't check whether this user already exists. I think you should also test*:

if username in users:
    print("Username already exists")
    continue

Otherwise someone could easily overwrite/register a new user root with his own credentials.

*I'm not this familiar with Python, so the code example might not be 100% accurate.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.