4
\$\begingroup\$

I posted some time ago an earlier version of this code and was told about some improvements that should be made. They were mostly about the sql-queries I was using and the DRY-principle.

Even though the program itself is quite simple I can't help but notice that the code isn't. I've been working on the code for some time and know how it obviously works but when someone that's new to the code looks at it, it might look confusing. I've been trying to use classes as a way to structure the code into smaller pieces to make it more readable.

What I would like is to get some input of the readability of the code and some general feedback if possible.

#!/usr/bin/python3
# -*- coding: utf-8 -*-

import sys
import random
import re
import sqlite3

while True:

    class signIn:
        def new_user(self):
            new_user_username_input = input("Hi, please give me your new username. \n\n")
            if not re.match("^[a-zA-Z0-9 _]*$", new_user_username_input):
                print("Please enter a valid username. (Only letters and numbers allowed)")
                self.new_user()
            sql = ("select exists(SELECT * from USERS where USERNAME = ?)")
            args = (new_user_username_input)
            cursor = self.database_connection.execute(sql, (args,))
            for username in cursor:
                pass
            if username[0] == 1:
                print("This username is already taken. Please provide a different one.")
                self.new_user()
            elif username[0] == 0:
                pass
            else:
                print("Something unexpected happened. Please try again")
                sys.exit()    
            new_user_password_input = input("Please give me your new password. \n\n")
            if not re.match("^[a-zA-Z0-9 _]*$", new_user_password_input):
                print("Please enter a valid username and password combination. (Only letters and numbers allowed)")
                self.new_user()
            print ("Creating new user... \n")
            sql = ("INSERT INTO USERS (USERNAME,PASSWORD,WINS,LOSES,GAMESPLAYED,WINPERCENT) VALUES (?,?,0,0,0,0)")
            args = (new_user_username_input,new_user_password_input)
            self.database_connection.execute(sql, args)
            self.database_connection.commit()
            print ("New user created successfully!\n")
            self.menu()

    class Stats:   
        def statistics(self):
            print("Here are the top 10 players in the order, who has the highest winning percentage\n")
            order = self.database_connection.execute("SELECT USERNAME, WINS, LOSES, GAMESPLAYED, WINPERCENT FROM USERS ORDER BY WINPERCENT DESC LIMIT 10")
            self.database_connection.commit()
            for row in order:
                print ("Username = ", row[0])
                print ("Wins = ", int(row[1]))
                print ("Loses = ", int(row[2]))
                print ("Games played = ", int(row[3]))
                print ("Winning percent = ", round(row[4], 1), "\n");
            self.menu()


    class Users:
        def allUsers(self):
            print("Below are listed all the available users.\nYou can create a new user from the MENU.\n")
            order = self.database_connection.execute("SELECT USERNAME FROM USERS ORDER BY USERNAME ASC")
            self.database_connection.commit()
            for row in order:
                print (row[0]);
            print ("\n")
            self.menu()


    class RockPaperScissors(signIn,Stats,Users):
        def __init__(self):
            RockPaperScissors.database_connection = sqlite3.connect('test.db')
            self.database_connection.execute('''CREATE TABLE if not exists USERS
                 (ID INTEGER PRIMARY KEY   AUTOINCREMENT,
                 USERNAME       TEXT    NOT NULL,
                 PASSWORD       TEXT     NOT NULL,
                 WINS           FLOAT   NOT NULL,
                 LOSES         FLOAT    NOT NULL,
                 GAMESPLAYED   FLOAT NOT NULL,
                 WINPERCENT    FLOAT NOT NULL   );''')           

        def welcome(self):
            print("\n\nThis is a Rock, Paper, Scissors game where you can play against a computer. \nThe one who first gets three wins is the champion.\n")

        def menu(self):
            user_input = input("--- MENU --- \nPlay (1) \nUsers (2) \nNew user (3) \nDelete User (4) \nStatistics (5) \nExit game (6)\n\n")
            if user_input == "1":
                self.player1_username()
            elif user_input == "2":
                self.allUsers()
            elif user_input == "3":
                self.new_user()
            elif user_input == "4":
                self.deleteUsers()
            elif user_input == "5":
                self.statistics()  
            elif user_input == "6":
                print ("\nThanks for playing!")
                sys.exit()
            else:
                print ("Please give a valid answer (1),(2),(3),(4),(5) or (6).\n")
                self.menu()

        def player1_username(self):
            player1_username_input = input("Player 1, please give me your username: \n\n")
            RockPaperScissors.player1_username_input = player1_username_input
            if not re.match("^[a-zA-Z0-9 -äöåÄÖÅ _]*$", self.player1_username_input):
                print("Please enter a valid username using only letters, numbers and spaces.")
                self.player1_username()
            sql = ("select exists(SELECT * from USERS where USERNAME = ?)")
            args = (player1_username_input)
            cursor = self.database_connection.execute(sql, (args,))
            for username in cursor:
                pass
            if username[0] == 1:
                self.player1_password()
            elif username[0] == 0:
                print ("\nThe username was not found.\nYou must create a user in MENU if you haven't got one.\n")
                self.menu()
            else:
                print("Something unexpected happened. Please try again")
                sys.exit()

        def player1_password(self):
            player1_password_input = input("Please give me your password: \n\n")
            if not re.match("^[a-zA-Z0-9 -äöåÄÖÅ _]*$", player1_password_input):
                print("You can only enter a password with letters, numbers and spaces.")
                self.player1_password()
            sql = ("select exists(SELECT * from USERS where PASSWORD = ? AND USERNAME = ?)")
            args = (player1_password_input,self.player1_username_input)
            cursor = self.database_connection.execute(sql, args)
            for password in cursor:
                pass
            if password[0] == 1:
                print ("Login successful ")
            elif password[0] == 0:
                print ("The password doesn't match with the given username. Please try again.\n")
                self.menu()
            else:
                print("Something unexpected happened. Please try again")
                sys.exit()

        def deleteUsers(self):
            delete_username_input = input("Which user you would like to delete?\n\n")
            sql = ("select exists(SELECT * from USERS where USERNAME = ?)")
            args = (delete_username_input)
            cursor = self.database_connection.execute(sql, (args,))
            for username in cursor:
                pass
            if username[0] == 1:
                delete_password_input = input("Please give password of the user. \n\n")
                sql = ("select exists(SELECT * from USERS where PASSWORD = ? AND USERNAME = ?)")
                args = (delete_password_input,delete_username_input)
                cursor = self.database_connection.execute(sql, args)
                for password in cursor:
                    pass
                if password[0] == 1:
                    sql = ("DELETE from USERS where USERNAME = ?;")
                    args = (delete_username_input)
                    self.database_connection.execute(sql, (args,))
                    self.database_connection.commit()
                    print ("User {0} successfully deleted!\n".format(delete_username_input))
                    self.menu()
                elif password[0] == 0:
                    print("The given password didn't match the username\n")
                    self.menu()
                else:
                    print("Something unexpected happened. Please try again")
                    sys.exit()  
            elif username[0] == 0:
                print("The given username was not found.\n")
                self.menu()
            else:
                print("Something unexpected happened. Please try again")
                sys.exit()

    class playAgain(RockPaperScissors):
        def new_game(self):
            self.new_game_choice = str.lower(str.strip(input("Do you want to play another round? (yes/no)\n\n")))
            if self.new_game_choice == "yes":                
                print("Great!\n\n")
            elif self.new_game_choice == "no":
                print("Thanks for playing!")
                self.database_connection.close()
                sys.exit()
            else:
                print("Please enter a yes or no as an answer")
                self.new_game()


    class Messages(RockPaperScissors):
        def win_messages(self):
            Messages.player1_win_message = ("Congrats {0}, you won computer in the game!".format(self.player1_username_input))
            Messages.computer_win_message = ("Too bad {0}, computer won you in the game!".format(self.player1_username_input))
            Messages.player1_games_won = 0
            Messages.computer_games_won = 0

        def player1_congrats(self):
            print ("Gongrats {0}, you are the champion!\n".format(self.player1_username_input))
            sql = ("UPDATE USERS set WINS = WINS + 1 where USERNAME = ?")
            args = (self.player1_username_input)
            self.database_connection.execute(sql, (args,))

        def computer_congrats(self):
            print ("Too bad {0}, computer is the champion!\n".format(self.player1_username_input))
            sql = ("UPDATE USERS set LOSES = LOSES + 1 where USERNAME = ?")
            args = (self.player1_username_input)
            self.database_connection.execute(sql, (args,))

        def congrats(self):
            sql = ("UPDATE USERS set GAMESPLAYED = WINS + LOSES where USERNAME = ?")
            args = (self.player1_username_input)
            self.database_connection.execute(sql, (args,))
            sql = ("UPDATE USERS set WINPERCENT = WINS/GAMESPLAYED*100 where USERNAME = ?")
            args = (self.player1_username_input)
            self.database_connection.execute(sql, (args,))
            self.database_connection.commit()


    class Choices(Messages):      
        def player1_choice(self): 
            self.player1_guess = str.lower(str.strip(input("So {0} rock(1), paper(2) or scissors(3)?\n\n".format(self.player1_username_input))))   
            if self.player1_guess == "1":
                self.player1_num = 1
            elif self.player1_guess == "2":
                self.player1_num = 2
            elif self.player1_guess == "3":
                self.player1_num = 3
            else:
                print ("Please only insert number 1,2 or 3 as an answer!")
                self.player1_choice() 

        def computer_choice(self): 
            self.computer_num = random.randrange(1,4)
            if self.computer_num == 1:
                print("The computer chose rock")
            elif self.computer_num == 2:
                print("The computer chose paper")
            elif self.computer_num == 3:
                print("The computer chose scissors")        

        def results(self): 
            difference = self.computer_num - self.player1_num
            if difference == 0:
                print ("It's a tie!")
            elif difference % 3 == 1: 
                Choices.computer_games_won+=1
                print (self.computer_win_message)
            elif difference % 3 == 2: 
                Choices.player1_games_won+=1
                print (self.player1_win_message)
            print ("So far {0} has won {1} times, and computer has won {2} times".format(self.player1_username_input,self.player1_games_won,self.computer_games_won))


    eka = RockPaperScissors()
    eka.welcome()
    eka.menu()
    toka = signIn()
    kolmas = Stats()
    neljas = Messages()
    neljas.win_messages()
    viides = playAgain()
    kuudes = Choices()
    seitsemas = Users()
    while True:
        if kuudes.player1_games_won == 3: 
            neljas.player1_congrats()
            neljas.congrats()
            viides.new_game()
            break
        elif kuudes.computer_games_won == 3: 
            neljas.computer_congrats()
            neljas.congrats()
            viides.new_game()
            break
        else: 
            kuudes.player1_choice() 
            kuudes.computer_choice()
            kuudes.results()
\$\endgroup\$
  • \$\begingroup\$ I'd expect signIn and playAgain to be methods, not classes. I'd expect a Player class to combine the many functions about the player. What made you write your code the way you did? \$\endgroup\$ – Mast Aug 2 '17 at 12:40
  • \$\begingroup\$ @Mast I just tried to break the code down to smaller pieces to make it more readable. I will rethink the classes and methods. Thanks for the feedback! \$\endgroup\$ – B.Turris Aug 2 '17 at 12:47
  • 1
    \$\begingroup\$ Oh, don't worry, somebody will point out in an answer how it should've been done :-) \$\endgroup\$ – Mast Aug 2 '17 at 12:49
1
\$\begingroup\$

Structure

This is a very weird way to define your classes:

while True:
    class A:
        ...

    a = A()
    ...

You would normally want to define your classes first, and only once (instead of re-defining them every loop iteration):

class A:
    ...


while True:
    a = A()
    ...

And then you probably want to isolate that running code. What if you write some other script in the future, where you also need the A class? If you tried to do a from rps import A, your while loop would run!

Instead, put this code under an if __name__ == "__main__": guard, and ideally also into a main function (if it is too long, as it is here):

class A:
    ...


def main():
    while True:
        a = A()
        ...


if __name__ == "__main__":
    main()

Variable naming

When posting code on the internet for people to look at, on an english-speaking site, you should translate all non-english parts, where possible.

We have no idea if kuudes and neljas are good or bad variable names in finnish(?) or not and so can't advise you on your variable naming. But as Google Translate says that these translate to sixth and fourth, respectively, it looks more like the latter.

You should come up with descriptive variable names (one of the two hardest things in software development). Maybe something like:

game = RockPaperScissors()
game.welcome()
game.menu()
login = signIn()
stats = Stats()
messages = Messages()
messages.win_messages()
play_again = playAgain()
choices = Choices()
users = Users()
\$\endgroup\$
  • \$\begingroup\$ How common would you say with a program that long, to break it into different files (Class per file)? \$\endgroup\$ – B.Turris Aug 9 '17 at 8:56
  • \$\begingroup\$ @B.Turris With this many classes, not uncommon. However, I agree with Mast's comment on your question, that most of your classes should actually be functions and then I would keep them all in one file probably, because they logically belong together. \$\endgroup\$ – Graipher Aug 9 '17 at 8:59
2
\$\begingroup\$

It is possible to break the long string literals just putting its parts one after other:

       user_input = input("--- MENU --- \nPlay (1) \nUsers (2) \nNew user (3) \nDelete User (4) \nStatistics (5) \nExit game (6)\n\n")

may become

       user_input = input("--- MENU --- "
                          "\nPlay (1) "
                          "\nUsers (2) "
                          "\nNew user (3) "
                          "\nDelete User (4) "
                          "\nStatistics (5) "
                          "\nExit game (6)\n\n")

(the symbol \ for the continuation at the end of lines is not needed as the all parts are in the parentheses).


Functions / methods are first-class citizens in Python so you may prepare a tuple / list / dictionary of them if you create a menu. For example instead of this part of your code

        if user_input == "1":
            self.player1_username()
        elif user_input == "2":
            self.allUsers()
        elif user_input == "3":
            self.new_user()
        elif user_input == "4":
            self.deleteUsers()
        elif user_input == "5":
            self.statistics()  

you may use

        methods = {
            '1': self.player1_username, 
            '2': self.allUsers, 
            '3': self.new_user, 
            '4': self.deleteUsers, 
            '5': self.statistics
            }
        if user_input in methods:  # better than 'if user_input in "12345"' thank Graipher
            methods[user_input]()

Slightly similarly, in situations like this

        if self.player1_guess == "1":
            self.player1_num = 1
        elif self.player1_guess == "2":
            self.player1_num = 2
        elif self.player1_guess == "3":
            self.player1_num = 3

you may use

        if self.player1_guess in "123":
            self.player1_num = int(self.player1_guess)
\$\endgroup\$
  • \$\begingroup\$ I would use if user_input in methods instead of if user_input in "12345" it is both faster (O(1) instead of O(n)) as well as more maintainable (impossible to forget to add a "6" to the string when adding another menu item). \$\endgroup\$ – Graipher Aug 4 '17 at 9:42
  • \$\begingroup\$ @Graipher - very nice tip, thanks! I'm going to edit my answer using your advice. \$\endgroup\$ – MarianD Aug 4 '17 at 13:37

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.