8
\$\begingroup\$

I've written a 'Mini Twitter' (that's what I've called it) in Python. It includes a text-based login screen and user panel where you can create Tweets, and change your username and password.

Users' names, passwords and whether or not it is an administrator is logged in a text file. It also logs Tweets Users have made in a separate .txt file.

There are 4 different .py files and 2 .txt files named: 'Administrator.py', 'main.py', 'Twitter_functions.py' and 'User.py'

Please assess it and let me know how I can improve it.

User.py:

#User.py

"""
Zak
July-August 2015

User.py

In here is a class named User that has methods such as makeTweet and setPassword.
There are also other functions that the class User uses.
"""

#strftime makes us able to make a note of the current date.
from time import strftime



#Create the class named User
class User:
    '''
    Class that acts as an online user and has methods such as 'make_tweet'.

    It takes 2 arguments, username and password.
    '''

    #Init constructor. This is called as soon as an instance is created.
    def __init__(self, username, password):

        self.__password = password

        self.__username = username
        self.__creationDate = strftime("%d/%m/%Y") #This is the current date.

        #Boolean to show if this class is an Administrator.
        self.__isAdmin = False



    def __repr__(self):
        return "User({}, {})".format(self.__username,self.__creationDate)

    #Getters
    def get_password(self):
        return self.__password
    def get_username(self):
        return self.__username
    def get_creation_date(self):
        return self.__creationDate

    #Setters
    def set_password(self, password):
        self.__password = password
    def set_username(self, username):
        self.__username = username




    def make_tweet(self, text):
        '''Method To make a tweet. It writes text to a file in a specific format.'''

        textLen = len(text)

        if textLen > 140: #140 is the character limit in Twitter.
            raise Exception("Too many characters. Expected 140, got {}.".format(textLen))

        with open("Tweets.txt","a+") as f:
            f.write("{} {}\n\t{}\n".format(self.__username,strftime("%d/%m/%Y"),text))

Administrator.py:

#Administrator.py

"""
Zak
July-August 2015

Administrator.py

In here is a class named Admin that inherits from the User class.
"""

#Import the User class
import User

#Create a class named Admin that inherits from the User class
class Admin(User.User):
    '''
    Class that acts as an online Admin. It inherits from a class named User.

    It takes 2 arguments, username and password.
    '''

    #Init constructor
    def __init__(self, username, password):

        #Call the parent's (User) init method. This saves us from rewriting code.
        super().__init__(username, password)

        #Boolean to show if this class is an Administrator.
        self.__isAdmin = True


    def __repr__(self):
        return "Administrator({}, {})".format(self.__username,self.__creationDate)

Twitter_functions.py:

#Twitter_functions.py

"""
Zak
July-August 2015

Twitter_functions.py

In here are functions that are used throughout my mini Twitter. Functions such as log_user()
and find_users() are here.
"""

import User
import Administrator

import re

#Function to log the user's name, creation date and password.
def log_user(username,password,isAdmin=False):
    '''Writes three variables to a file.'''

    with open("Users.txt","a+") as f:
        f.write("{} {} {}\n".format(username,password,isAdmin))



def find_users():
    '''Return a tuple that contains username,password,isAdmin.'''

    with open("Users.txt","r+") as f:
        lines = f.readlines()

    for line in lines:
        username, password,isAdmin = line.strip().split()
        yield username,password,isAdmin


#This function acts as a log-in screen. It takes 2 arguments that must both be strings.
def login(accUsername,accPassword):
    '''Returns an instance of a class if the 'login' was successful, otherwise False.'''

    for user in find_users():

        #user[0] = a stored username, [1] = a stored password
        if accUsername == user[0] and accPassword == user[1]:

            #The account's username and password is valid:
            if user[2] == 'True':
                #If the account is an admin:

                #cAdmin = currentAdmin
                cAdmin = Administrator.Admin(accUsername,accPassword)
                return cAdmin

            else:
                #If the account is not an admin:
                cUser = User.User(accUsername,accPassword)
                return cUser

    #If no matchings are found:
    return False

#This function acts as a sign-up. It takes 2 arguments that must both be strings.
def signup(accUsername,accPassword):
    '''Returns a string saying whether the signup was successful or not.'''

    for user in find_users():
        if user[0] == accUsername:
            return "Username is already in use."


    cUser = User.User(accUsername,accPassword)

    log_user(accUsername,accPassword)

    return "Signup Successful."

#This function changes a username to specified text in the Users.txt
def change_username(oldUsername, newUsername):
    '''Replaces text (usernames specifically) in a .txt file to something specified.'''

    #Open the file, and search all usernames, If it matches, replace it.
    with open("Users.txt","r") as f:

        if newUsername in f.read():
            raise Exception("Username already exists.")

        f.seek(0)

        filedata = f.readlines()

        newdata = [x.strip().split() for x in filedata]
        #Example newdata: [[username, password, isAdmin]]

        for account in newdata:
            if account[0] == oldUsername:
                account[0] = newUsername

    #Now write the newdata to the file
    with open("Users.txt","w") as f:

        for account in newdata:

            #account[0] = username, [1] = password and [2] is isAdmin
            f.write("{} {} {}\n".format(account[0],account[1],account[2]))



#This function is similar to change_username, and changes a password instead.
def change_password(oldPassword,newPassword, username):
    '''Replaces text (passwords specifically) in a .txt file to something specified.'''

    #Open the file, and search all passwords, If it matches, replace it.
    with open("Users.txt","r") as f:

        #All file contents in a list
        filedata = f.readlines()

        #Get rid of spaces and newlines (\n) in the filedata variable.
        newdata = [x.strip().split() for x in filedata]

        for account in newdata:
            if account[0] == username and account[1] == oldPassword:
                account[1] = newPassword

    #Now write the newdata to the file
    with open("Users.txt","w") as f:

        for account in newdata:

            #account[0] = username, [1] = password and [2] is isAdmin
            f.write("{} {} {}\n".format(account[0],account[1],account[2]))

main.py:

#main.py

"""
Zak
July-August 2015

main.py

Here is the main part of my mini Twitter. It actually prints text, unlike the
other half of my code.
"""

import Twitter_functions as tf


def loggedin(account):

    print("""What do you want to do?
1) Make A Tweet
2) Change Username
3) Change Password
""")

    selection = int(input("Please Enter Your Choice: ") )

    #Make a Tweet
    if selection == 1:

        try:
            tweet = input("Enter Your Tweet Text: ")
        except KeyboardInterrupt:
            exit()
        except:
            loggedin(account)

        try:
            account.make_tweet(tweet)
        except Exception as e:
            print("Error! {}".format(str(e)))
        except:
            print("An error occurred. Restarting.")
            main()
        loggedin(account)

    #Change Username
    elif selection == 2:

        try:
            username = input("Please Enter Your New Username: ")
        except KeyboardInterrupt:
            exit()
        except:
            loggedin(account)

        #Check if the username is valid
        if not username.isalpha():
            print("Username cannot contain symbols.")

        #This function edits Users.txt and replaces the correct usernames. It raises an
        #exception if the username already exists.
        try:
            tf.change_username(account.get_username(),username)
        except Exception as e:

            print("{}".format(str(e)))
            loggedin(account)

        account.set_username(username)

        print("Username set to: {}".format(account.get_username()))
        loggedin(account)

    #Change Password
    elif selection == 3:

        #Get the user to input his/her's current password.
        currentPassword = account.get_password()

        try:
            password = input("Please enter your current Password: ")
        except KeyboardInterrupt:
            exit()
        except:
            main()

        if password != currentPassword:
            print("Incorrect Password.")
            loggedin(account)

        elif password == currentPassword:

            password = input("Enter Your New Password: ")

            if " " in password or len(password) == 0:
                print("Password cannot contain ' ' (space) or nothing.")

            tf.change_password(account.get_password(),password,account.get_username())

            account.set_password(password)

            print("Password set to: {}".format(account.get_password()))
            loggedin(account)



#In Java, this would be a public static void main(){}
def main():
    print("\nWelcome To Mini Twitter!\n")

    #Ask the user if he or she wishes to signup. Then ask for a username and password
    try:
        shouldSignup = input("Do you want to signup? Y/N: ")

    except KeyboardInterrupt: #ctrl+c:
        exit()
    except:
        main()

    if shouldSignup.lower().startswith("y"):
        signingUp = True
    else:
        signingUp = False

    if signingUp:
        print("\n\nSign Up Menu:")
    else:
        print("\n\nLogin Menu:")

    try:
        username = input("Username: ")
    except KeyboardInterrupt:
        exit()
    except:
        main()

    if not username.isalpha():
        print("Username cannot contain symbols or nothing.")
        main()

    try:
        password = input("Password: ")
    except KeyboardInterrupt:
        exit()
    except:
        main()

    if " " in password:
        print("Password cannot contain ' ' (space).")
        main()


    if signingUp:

        try:
            #Signup
            signedup = tf.signup(username,password)
        except:
            print("An error occurred while attempting to signup.. Restarting now.")
            main()

        print(signedup)
        main()

    else:

        #Simple Login
        account = tf.login(username,password)

        if not account:
            print("Incorrect Login Details.")
            main()

        print("Login Successful. Welcome, {}".format(account.get_username()))

        loggedin(account)


#Call main. If it is imported, don't.
if __name__ == "__main__":
    main()

Examples:

Users.txt:

Bobby hello123 False
blaze bobby False
zxyo zxyo1111 False
BlaZe Zak False
boblington tommy False
hihio hi False

Tweets.txt:

tommy 19/07/2015
  Hello world. Just made this account!
zxyo 21/07/2015
  this is cool.
Bob 24/07/2015
  Hi, Im Bobby
BlaZe 24/07/2015
  hi there

One thing I should note is that I don't really have any admin functionality as of now. I don't intend to add this, I just wanted to make it so I could add it in the future.

\$\endgroup\$
3
\$\begingroup\$

The first thing I would say is that you don't need the User and Administrator classes in separate files. You can simply put them together in one module instead. Especially since Administrator actually imports User anyway. You can import them so that you only need the bare class name too and would require only minor changes to your code.

from userclasses import User, Administrator

I also think that your make_tweet function doesn't need to separate these lines:

textLen = len(text)

if textLen > 140: #140 is the character limit in Twitter.

You can just call len(text) in the if statement and save yourself a line of code and a variable declaration. It's also more clear because it's more obvious what the if statement means in that context.

Also, you use open(file, 'a+') multiple times without needing to. a+ allows you to read and append to the file, while a will allow you to just append. In a few places you could ommit the + as unnecessary.

Speaking of unnecessary, you have a lot of comments about pretty self explanatory things.

#Create the class named User
class User:

Python is explicitly designed to be clear enough that something like this shouldn't be necessary. Most people reading Python code will know this or what an init function is, unless your intention is for this to be teaching about how Python works.

On the flipside, this docstring is not explanatory enough:

def log_user(username,password,isAdmin=False):
    '''Writes three variables to a file.'''

It's more useful to explain what the variables are and why they're useful to write to a file. Better would be

def log_user(username,password,isAdmin=False):
    '''Writes a user's username, password and admin status to a file.'''

Even if it's quite clear based on the parameter names, people can sometimes see a docstring without the parameter list and there's no reason to leave out this information in that case.

Also, you don't need to try making private variables. There's no actual way to prevent code from accessing the attributes of a class. At any point code could just directly set someUser.__password = "banana".

In Python the convention is to start a variable name with a single underscore to indicate that other people using your code should treat the variable as private, and if they don't then it's their responsibility, not yours. So, you don't need any of the setters or getters, you can just get and set the password directly with the dot syntax whenever you need to.


I agree with shuttle87 about data structures, but for the current structure I like your yield set up for the user list. However, I think you should be prepared to handle the getting data in the wrong format. If someone edits the file or a write is done incorrectly you would get an error from having too few arguments for the three variables you're trying to set. But that error would be vague and unclear to a user, so you should raise a more clear error:

for line in lines:
    try:
        username, password,isAdmin = line.strip().split()
    except ValueError:
        raise(ValueError, "User data file is in invalid format.")
    yield username,password,isAdmin

You can still catch this error with a try except in cases where you want to continue on regardless.

Also login can just not return anything if you don't get a User class. In the absence of a return statement Python functions will return None, and if None will evaluate as False.

I'd rewrite the change_password and change_username functions to just be one function, with a parameter that specifies what to change. That way it could also be expanded to give admin privileges if you need that later. But even at the moment, you're largely duplicating unneeded code. Also you probably shouldn't let people change other users' usernames.

def change_userdata(username, password, newdata, type):
    '''Replaces text in a .txt file to a new value, returns True if successful.'''

    with open("Users.txt","r") as f:
        filedata = [x.strip().split() for x in f.readlines()]

    for account in filedata:
        if account[0] == username and account[1] == password:
            if type == "username":
                account[0] = newdata
            elif type == "password":
                account[1] = newdata
            elif type == "admine":
                account[2] = newdata
            else:
                raise(ValueError, "Invalid data type {}".format(type))
            break
    else:
        return

    with open("Users.txt","w") as f:
        for account in newdata:
            #account [username, password, isAdmin]
            f.write("{} {} {}\n".format(*account))

    return True

I made some other changes there, but in particular I should explain the for else loop and *account. An else at the end of a Python for loop will run if no iteration of the for loop called break. You can see that I break from the loop when the relevant account has been found, as there's no need to keep going once the correct user has been found. So if the user is never found, break is never run and instead the function will return early, signifying False if you check the result the function returns.

As for *account, it's basically a Python operator to 'unpack' the list, returning all the values as a tuple. That way you don't need to write out each index since you can simultaneously pass all three of them to format.


In main, I think you should actually have a function for each of the user's options. You can actually store a function in a list/dictionary and call it from there so it would be much more organised and easier to expand, like this:

functions = [tweet, change_username, change_password]    
selection = int(input("Please Enter Your Choice: ") )

try:
    functions[selection]()
except IndexError:
    print ("Invalid input, there's no function at index {}".format(selection))

I also think your labouring the input validation and bare excepts are always a bad idea. I'm not even sure what error you're trying to guard with them to be honest so I'd entirely strip them out. Instead, when an error is occurring, you should do your best to either fix it or handle it specifically. Like I've done above with the IndexError and ValueError. The KeyboardInterrupt calling exit is a good example too.

Though to be cleaner, it'd be better to define your own input function and keep using it throughout, since every input location has the same set up essentially:

def my_input(s):
    try:
        return input(s)
    except KeyboardInterrupt:
        exit()

You can then just add more if there are other exceptions to be handled.

You can assign expressions to variables as booleans in Python, so you can shorten this:

if shouldSignup.lower().startswith("y"):
    signingUp = True
else:
    signingUp = False

to this:

signingUp = shouldSignup.lower().startswith("y")

Also I find that validating input works best when using a loop rather than recursion:

while True:
    username = my_input("Username: ")
    if username.isalpha():
        break

    print("Username cannot contain symbols or nothing.")
\$\endgroup\$
4
\$\begingroup\$

datastructures

One thing I would do is change the datastructure that stores the users. You want to be able to look up each user for the purposes of authentication/logins/administration/etc quickly. If you are storing the users as a list that is accessed by yielding tuples you will have O(n) lookup time for each user. This will not scale well if you have a lot of users. You don't want to have to check every existing user every time that you need to do something with a user.

Instead you want to choose a more suitable datastructure. Personally I would store the users information in a dictionary, with the login name as the keys and the values would be the tuple containing the other information associated with the user. This will make all operations involving user data manipulation much much faster.

refactoring

In def loggedin(account): I would refactor this so that each option was in it's own function.

In the following code:

try:
    username = input("Username: ")
except KeyboardInterrupt:
    exit()
except:
    main()

You have a bare except, this means that if anything at all gets thrown then main is called. Say an OSError is raised, do you really just want to go back to main again? This is why bare except clauses are usually a code smell, they hide bugs and create problems in the future.

Place your user input functionality into it's own function and deal with the vaildation there, if anything fails then report the failure and a different level of the code can try to recover if possible.

Private variables and Python

I can see that you are from a Java background so this one might be a bit of a paradigm shift, but I really think you should avoid using the __variable+getter/setter approach. Python is not like Java, there is no compiler to enforce private variable scope so don't concern yourself too much with it. If you want to signal to people that they should treat things as implementation details the convention is to just use a single underscore to state your intention. The philosophy with Python is that if people misuse your classes that is their responsibility, not yours. Overall I think this is good Python code coming from a Java background. I recently had to go from a very Python heavy base of code to a Java code base at work back again and it's a very different paradigm.

\$\endgroup\$
  • \$\begingroup\$ Thank you, but I am still confused with private variables. Should I remove my getters and setters and only have one underscore before a variable? Perhaps you could give an example? It would help. Thank you. \$\endgroup\$ – BlaZe Aug 25 '15 at 8:50
  • 1
    \$\begingroup\$ It's a cultural thing, you are free to do what you have been doing but it's generally frowned upon in the Python world. The reason is that unlike in Java and some other statically types languages you get no access guarantees from Python the language by making the variables "private". One of the strengths of Python simultaneously makes it's fairly easy to circumvent attempts to force people to use your API in a specific way. The main benefit of private variable is to tell other people "hey this is an implementation detail, don't touch this if you don't know what you are doing".... \$\endgroup\$ – shuttle87 Aug 25 '15 at 14:04
  • \$\begingroup\$ The door is left open for people to change the internal state, for doing things such as testing/mocking if they want. The cultural shift is that the responsibility for not changing internal state is placed on the API user instead of the writer. If the concern is that changes will break user code you can use properties to implement getters and setters later on. The point is that in Python, unlike Java, you can make this change later without changing the interface your code exposes to the world. The main downside that getters and setters are designed to avoid is therefore sidestepped in Python. \$\endgroup\$ – shuttle87 Aug 25 '15 at 14:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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