Quick email program using Python classes

Question

I've just learned about using classes and I have this constant feeling that I'm just writing things inefficiently or wrong.

Could I get some advise on this quick "email" program? How would you have set this program up, more classes? methods outside of User class? Should I be using Global differently? Is my commenting structured well? I'm entirely self-taught so I want to know if I've developed any bad habits or something like that.

To get it to run, all you need is to replace the inboxLocation variable with a location on your station.

import os


class User:


    # Domain name for emails and location for inboxes
    global emailDomain
    emailDomain   = "@test.local"
    global inboxLocation
    inboxLocation = "E:/Users/Dylan/Desktop/Inboxes/"


    # User Creation
    def __init__(self, firstName, lastName, password):
        self.firstName = firstName
        self.lastName  = lastName
        self.userName  = firstName[0]  + lastName
        self.address   = self.userName + emailDomain
        self.password  = password
        self.inbox     = inboxLocation + self.address

        # Create user inbox if it doesn't exist
        if not os.path.exists(self.inbox):
            os.mkdir(self.inbox)


    # Creates a header at the top of every page with the menu title and username
    def getHeader(self, currentMenu):
        header = (currentMenu + " | " + self.userName + "\n")
        print(header)


    # Display information about a user
    def getInfo(self):
        print("User Information for %s\n" % self.address)
        print("First Name: %s"            % self.firstName)
        print("Last Name: %s"             % self.lastName)
        print("Email Address: %s"         % self.address)
        print("Inbox Location: %s\n"      % self.inbox)

        input("\n[Enter to Continue...]\n")
        self.mainMenu()


    # Create and send an email to a user's inbox
    def sendEmail(self):
        self.getHeader("New Email")

        sender   = self.address
        receiver = input("Sender Address: ")
        subject  = input("Subject: ")
        body     = input("Body:\n")

        # Create email file and store it in users inbox
        fileName     = (inboxLocation + receiver + "/" + self.userName + " - " + subject + ".txt")
        emailFile    = open(fileName, "w")
        emailContent = ("from: " + sender + "\n" +
                        "to: " + receiver + "\n" +
                        "subject: " + subject + "\n\n" +
                        body)
        emailFile.write(emailContent)
        emailFile.close()

        # Confirm that email was sent
        if os.path.isfile(fileName):
            print("Email Sent! Returning to main menu.\n\n")
            self.mainMenu()
        else:
            print("Email Transmission failure, please try again.")
            self.sendEmail()


    # Look through a list of emails in inbox, delete function to be added
    def readEmails(self):
        self.getHeader("Inbox")

        # Iterate through inbox and assign numbers to each email for user selection
        print("Select an Email to read\n\nM) Main Menu")
        emailList = (os.listdir(self.inbox))
        num       = 0
        for email in emailList:
            print(str(num) + ". " + email)
            num += 1

        # Ensure user input is a both a number and within range
        readChoice = input("\n\n>>> ")
        try:

            # Exit to email if selected
            if readChoice == "M" or readChoice == "m":
                self.mainMenu()

            # Open selected email
            else:
                readChoice = int(readChoice)
                readEmail  = open(self.inbox + "/" + emailList[readChoice], "r")
                print(readEmail.read())
                readEmail.close()

                input("\n\n[Enter to Continue...]\n")
                self.readEmails()

        # Catch bad inputs
        except TypeError or IndexError:
            print("Please enter a valid number")
            self.readEmail()


    # Main menu method for moving in and out of menus
    def mainMenu(self):
        self.getHeader("Main Menu")
        choice = input("A) Send Email\nB) Inbox\nC) My Profile\nD) Logout\n\n>>> ")
        if choice   == "a" or choice == 'A':
            self.sendEmail()
        elif choice == "b" or choice == 'B':
            self.readEmails()
        elif choice == "c" or choice == 'C':
            self.getInfo()
        elif choice == "d" or choice == 'D':
            self.logout()
        else:
            print("Invalid option, try again")
            self.mainMenu()


    # Logout method for changing users
    def logout(self):
        self.getHeader("logout")
        logoutChoice = input("are you sure you would like to log out?\n\n>>> ")
        if logoutChoice == "Yes" or logoutChoice == "yes" or logoutChoice == "Y" or logoutChoice == "y":
            login()


# Login method called immediately after accounts are created
def login():
    print("Please enter your credentials\n")
    try:
        userEmail = input("Username: ")
        password  = input("Password: ")

        # Raises a NameError if user is not found
        userEmail = eval(userEmail)

        if password == userEmail.password:
            print("Login successful, Welcome to your inbox", userEmail.address)
            userEmail.mainMenu()

        # Raise NameError if password is incorrect
        else:
            raise NameError

    # Handle NameError, inform user of mistake, restart login method
    except NameError:
        print("Username or password is incorrect\n\n")
        login()


# Creates users on start of program, method to create users will be added.
# Users will be stored in file with hashed passwords.
def userCreation():
    global jblow
    global bblow
    jblow = User("joe", "blow", "pass123")
    strudel = User("sylvester", "trudel", "P@ssw0rd")


# Main method, initializes the program
def main():
    userCreation()
    login()


# Run Main
main()

Answer 1

Your program is hard to understand, and use because User is trying to do too much. It's acting as a client, a server, and a user.

And so I'd highly recommend splitting these aspects up:

• Anything to do with mutating data in E:/Users/Dylan/Desktop/Inboxes/ should be in a Server class.

  Each of the public methods on this class should return a status and any data. This makes things easier in the long run, as you should preferably change Server to send requests to an actual server. Which will lead to cases where there could be no connection between the client and the server.

  Preferably, you should later make this code it's own function that runs as a server. This can allow you to set the folder permissions to E:/Users/Dylan/Desktop/Inboxes/ so that only one user can access the files within. This prevents users reading other users emails. And the server should be run under that user, so it can access everything.

• Anything to do with interacting with the server class should be in the Client class.

  Later you may want to split this code up more, however as it is, it's abstracted enough that I think new users can easily understand the code. I don't like the idea of printing inside the class, however, to remove it would be more effort than is worth.

  Ideally this should be implemented in a way that you can easily change your UI, whether it be a GUI or a CLI. This is as each class should add a level of abstraction that makes reasoning with things simpler.

• Anything to do with a single user, should be in the User class.

  This should make sense, as a user isn't part of the client. Any user can use it.

  The biggest changes I made however were:

  • You have to specify everything when creating the user.
  • There is no user.inbox. This is as only the server should know about that, and needs to know it.

• Everything else should be in a separate function or class.

  I personally put the mainMenu code into a new main function, as I don't think Client should control the main loop. As I outlined above.

All these things together I'd change your code to something like: (untested)

from collections import namedtuple
from textwrap import dedent
import os

class Server:
    def __init__(self, inbox_location):
        self.inbox_location = inbox_location
        users = [
            jblow = User('joe', 'blow', 'jblow', '[email protected]', "pass123")
            strudel = User('sylvester', 'trudel', 'strudel', '[email protected]', "P@ssw0rd")
        ]
        self.users = {
            user.user_name: user
            for user in users
        }

    def _email(self, address, email):
        return os.path.join(self.inbox_location, address, email)

    def send_email(self, sender, receiver, subject, body):
        file_name = self._email(receiver, sender.user_name + ' - ' + subject + '.txt')
        file_content = dedent('''\
            from: {}
            to: {}
            subject: {}

            {}
            ''').format(sender.address, receiver, subject, body)
        try:
            with open(file_name, 'w') as email_file:
                email_file.write(file_content)
        except Exception:
            return False
        return os.path.isfile(file_name)

    def list_emails(self, user):
        return True, os.listdir(user.inbox)

    def read_email(self, user, email_id):
        try:
            file_name = self._email(user.address, self.list_emails()[email_id])
            with open(file_name, 'r') as f:
                return True, f.read()
        except Exception:
            return False, None

    def logon(self, user_name, password):
        user = self.users.get(user_name)
        if user is None or user.password != password:
            return False, None
        return True, user


def header(current_menu, user):
    return '{} | {}\n'.format(current_menu, user.user_name)


class Client:
    def __init__(self, server):
        self.server = server

    def send_email(self, sender):
        while True:
            print(header("New Email"))
            receiver = input("Sender Address: ")
            subject  = input("Subject: ")
            body     = input("Body:\n")

            status = self.server.send_email(sender, receiver, subject, body)
            if status:
                print("Email Sent! Returning to main menu.\n\n")
                break

            print("Email Transmission failure, please try again.")

    def read_emails(self, user):
        status, emails = self.server.list_emails(user)
        if not status:
            return
        while True:
            print(header("Inbox"))
            print("Select an Email to read\n\nM) Main Menu")
            for num, email in enumerate(emails):
                print('{}. {}'.format(num, email))

            while True:
                choice = input('\n\n>>> ').lower()
                if choice == 'm':
                    break

                try:
                    choice = int(choice)
                    break
                except ValueError:
                    pass

            if choice == 'm':
                break

            status, email = self.server.read_email(user, choice)
            if status:
                print(email)
                break

    def logon(self, user_name, password):
        while True:
            print('Please enter your credentials\n')
            user_name = input('Username: ')
            password  = input('Password: ')
            satus, user = self.server.logon(user_name, password)
            if status:
                print('Login successful, Welcome to your inbox', user.address)
                return user
            print('Username or password is incorrect\n\n')


UserBase = namedtuple('UserBase', 'first_name last_name user_name address password')


class User(UserBase):
    def info(self):
        print(dedent('''\
            User Information for {0.address}

            First Name: {0.first_name}
            Last Name: {0.last_name}
            User Name: {0.user_name}
            Email Address: {0.address}
        ''').format(self))
        input("\n[Enter to Continue...]\n")


def main():
    client = Client(Server('E:/Users/Dylan/Desktop/Inboxes/'))
    while True:
        user = client.login()
        while True:
            print(header('Main Menu'))
            choice = input('A) Send Email\nB) Inbox\nC) My Profile\nD) Logout\n\n>>> ').lower()
            if choice   == 'a':
                client.send_email(user)
            elif choice == 'b':
                client.read_emails(user)
            elif choice == 'c':
                user.info()
            elif choice == 'd':
                header('logout')
                logout_choice = input('are you sure you would like to log out?\n\n>>> ').lower()
                if logout_choice in ('yes', 'y'):
                    break
            else:
                print('Invalid option, try again')


if __name__ == '__main__':
    main()

Answer 2

For one thing, the domain being global is not very good: what if two users have email addresses on different domains? Are you going to set it to different values in between user creation? Just let the user have its own email address. Globals in general are frowned upon for many reasons, so when you catch yourself thinking "yes, I'll make this a global", please count to ten and then don't do it.

To answer your question in the comments, where you assume you have an internal mail server only: a good rule of thumb is to keep outside of your program everything that can conveniently kept out. In this case, the domain is configuration, and as such it should be kept in a configuration file. Your program does not and should not know what the specific domain is. It just needs to know how to read it from an external configuration file/database/what-have-you.

Modifying software is not a lot of fun, so if you can make a program whose behavior you can modify without even touching the source, it's way better.

What if your domain changes? As it stands, you have to change your program, and possibly even test it again. What if the domain sits in a config file? You change it in there and you're golden.

If you think hard enough, you will find that a lot of configuration can be kept outside of the program, even down to the choice of different algorithms to accomplish the same task. Personally, I try to make everything I make as configurable as possible, because recompiling and testing and deploying is boring.

Finally, globals are bad because they're hard to keep track of, and it's difficult to know what parts of the code read from and write to them, which makes the whole thing horribly coupled. Do try to avoid them as much as possible.