NEA Computing Task 2 Dice Game

Question

I was recently set Task 2 as seen below and I realise someone answer the question on this site here but I wanted a fresh opinion

TASK 2:

1. Allows two players to enter their details, which are then authenticated to ensure that they are authorised players.
2. Allows each player to roll two 6-sided dice.
3. Calculates and outputs the points for each round and each player’s total score.
4. Allows the players to play 5 rounds.
5. If both players have the same score after 5 rounds, allows each player to roll 1 die each until someone wins.
6. Outputs who has won at the end of the 5 rounds.
7. Stores the winner’s score, and their name, in an external file.
8. Displays the score and player name of the top 5 winning scores from the external file.

try: File = open("Users.txt","r")
except FileNotFoundError:
    raise SystemExit("User file not found")
File = open("Users.txt", "r")

def Login(Username, Player):
    File.seek(0)
    for Line in File:
        ValidUsername = Line.split(",")[0]
        ValidPassword = Line.split(",")[1].replace("\n", "")
        if Username == ValidUsername:
            Password = input("Password: ")
            if Password == ValidPassword:
                print("Player",Player,"logged in")
                print("")
                return True
            else: print("Invalid Details")
    return False

try:
    while True:
        print("Player 1 Login")
        Username1 = input("Username: ")
        if Login(Username1, 1): break
        print("")
    while True:
        print("Player 2 Login")
        Username2 = input("Username: ")
        if Username1 == Username2:
            print("Double login detected")
        elif Login(Username2, 2): break
        print("")
except KeyboardInterrupt:
    raise SystemExit("Exiting...")
finally:
    File.close()

import random
Player1Score = 0
Player2Score = 0

def Roll():
    Dice1 = random.randint(1, 6)
    Dice2 = random.randint(1, 6)
    print("You rolled a",Dice1,"and a",Dice2)
    Change = Dice1 + Dice2
    Change += 10 if (Dice1 + Dice2) % 2 == 0 else -5
    if Change < 0: Change = 0
    if Dice1 == Dice2:
        Dice3 = random.randint(1, 6)
        print("Your third roll is a",Dice3)
        Change += Dice3
    print("")
    return Change

for X in range(5):
    print("Play:",X + 1,"starting")
    input("Player 1, press enter to roll: ")
    Player1Score += Roll()
    input("Player 2, press enter to roll: ")
    Player2Score += Roll()
    print("Player 1 now has a score of",Player1Score)
    print("Player 2 now has a score of",Player2Score)
    print("")

if Player1Score > Player2Score: Winner = 1
if Player1Score < Player2Score: Winner = 2
if Player1Score == Player2Score:
    print("You both got the same score")
    def SameScore():
        input("Press enter to roll dice: ")
        print("")
        Dice1 = random.randint(1, 6)
        Dice2 = random.randint(1, 6)
        print("Player 1 rolled:",Dice1)
        print("Player 2 rolled:",Dice2)
        if Dice1 == Dice2: return False
        if Dice1 > Dice2: return 1
        if Dice1 < Dice2: return 2
    Winner = False
    while not Winner:
        Winner = SameScore()

if Winner == 1:
    Winner = Username1 + ": " + str(Player1Score)
    print(Username1,"won with",Player1Score,"points")
    print(Username2,"lost with",Player2Score,"points")
if Winner == 2:
    Winner = Username2 + ": " + str(Player2Score)
    print(Username2,"won with",Player2Score,"points")
    print(Username1,"lost with",Player1Score,"points")
WinnerScore = int(Winner.split(": ")[1])
FileWritten = False
try:
    File = open("Scores.txt", "r")
    Data = File.readlines();File.close()
    for X in range(len(Data)):
        if WinnerScore > int(Data[X].split(": ")[1]):
            Data.insert(X, Winner + "\n")
            if len(Data) > 5: Data.pop(5)
            FileWritten = True; break
    if len(Data) < 5:
        if not FileWritten: Data.append(Winner + "\n")
    File = open("Scores.txt","w")
    for X in Data:
        File.write(X.replace("\n","") + "\n")
except FileNotFoundError:
    File = open("Scores.txt","w")
    File.write(Winner + "\n")
File.close()
print("")

File = open("Scores.txt","r")
print("Highscores:")
for Line in File:
    if Line != "": print(Line.replace("\n", ""))
File.close()

Any thoughts/optimisations would be greatly appreciated

Answer 1

Welcome to Code Review!

PEP-8

In python, it is common (and recommended) to follow the PEP-8 style guide for writing clean, maintainable and consistent code.

Functions and variables should be named in a lower_snake_case, classes as UpperCamelCase, and constants as UPPER_SNAKE_CASE.

f-strings

Newly introduced in python 3 is the f-string; so instead of having print("string", variable, "string") you can do:

print(f"string {variable} string")

for the same effect.

Functions

Split your code into individual smaller functions, doing singular tasks. A few examples would be, fetching user/password from users.txt, validating user details, reading user credentials and so on.

if __name__ block

For scripts, it is a good practice to put your executable feature inside the if __name__ == "__main__" clause.

Control flow

You try to open users file twice (same is true for scores file). This is followed by a function definition, followed by code to get users to login. Then you have your first import statement followed by yet another function definition and later code again.

Try grouping blocks together. In python (and almost all programming languages), imports are the very first thing (after shebang). Then global constants, functions/classes definitions and then the script code itself.

with statements

Instead of you managing opened file descriptors and later controlling the closure of those, python has with statement that wraps this for you:

with open(your_file, mode) as f:
    something = f.read()
    # f.write(something)

this auto closes (and keeps in-scope) the file descriptor.

Associated attributes

You have player element, which has associated properties like username, player_id, score. This can be put into a class, instead of maintaining 10 different variables for each player.

Inbuilt methods

At a lot of places in your code, you are doing a .replace("\n", ""). This is not really needed, as string objects in python have a .strip() method, which cleans up all whitespaces (and newlines).

Similarly, you try to capture a FileNotFoundError only to raise another error for the same reason. Let the error specifically defined for the job do that. No need to capture if you want the program to fail in case of missing file anyway.

---

Rewrite

from operator import itemgetter
import random

USERS_FILE: str = "Users.txt"
SCORES_FILE: str = "Scores.txt"
NUM_PLAYERS: int = 2
NUM_ROUNDS: int = 5


def roll_die() -> int:
    return random.randint(1, 6)


class Player:
    def __init__(self, _id: int, name: str):
        self._id = _id
        self.name = name
        self.score = 0

    def add_score(self, value: int):
        self.score += value
        if self.score < 0:
            self.score = 0

    def turn(self):
        input(f"{self.name}'s turn. Press enter to roll.")
        dice_1 = roll_die()
        dice_2 = roll_die()
        print(f"{self.name} rolled {dice_1} and {dice_2}.")
        change = dice_1 + dice_2
        change += 10 if change % 2 == 0 else -5
        if change < 0:
            change = 0
        if dice_1 == dice_2:
            dice_3 = roll_die()
            print(f"Third roll is {dice_3}")
            change += dice_3
        self.add_score(change)

    def __str__(self) -> str:
        return f"Player({self.name}): {self.score}"


def fetch_users() -> dict:
    users = {}
    with open(USERS_FILE, "r") as f:
        users = dict([line.strip().split(",") for line in f])
    return users


def authenticate(users: dict, name: str, password: str) -> bool:
    return users.get(name) == password


def show_highscores() -> None:
    with open(SCORES_FILE, "r") as f:
        print(f.read())


def fetch_highscores() -> list:
    scores = []
    with open(SCORES_FILE, "r") as f:
        for line in f:
            name, score = line.strip().split(": ")
            score = int(score)
            scores.append((name, score))
    return scores


def write_score(player: Player, limit: int = 5):
    current_highscores = fetch_highscores()
    current_highscores.append((player.name, player.score))
    sorted_scores = sorted(current_highscores, key=itemgetter(1), reverse=True)
    with open(SCORES_FILE, "w") as f:
        for name, score in sorted_scores[:limit]:
            f.write(f"{name}: {score}\n")


def get_player(_id: int, users: dict) -> Player:
    print(f"Players {_id} login")
    while True:
        name = input("username: ")
        password = input("password: ")
        if authenticate(users, name, password):
            return Player(_id, name)
        print("Invalid details. Try again!")


def get_winning_player(players: list) -> Player:
    def filter_winning_players(player_dices: list, value: int):
        filtered = []
        for player, dice in player_dices:
            print(f"{player.name} rolled {dice}.")
            if dice == value:
                filtered.append(player)
        return filtered
    max_score = max([player.score for player in players])
    winners = [player for player in players if player.score == max_score]
    if len(winners) == 1:
        return winners[0]
    print(f"{len(winners)} players have the same score. Trying to determine single winning player.")
    while True:
        input("Press enter to roll dice: ")
        dices = [roll_die() for _ in range(len(winners))]
        max_dice = max(dices)
        winners = filter_winning_players(zip(winners, dices), max_dice)
        if len(winners) == 1:
            return winners[0]


def game():
    users = fetch_users()
    players = [get_player(count + 1, users) for count in range(NUM_PLAYERS)]
    for round in range(1, NUM_ROUNDS + 1):
        for player in players:
            player.turn()
        print(f"Player scores at end of round {round}:")
        for player in players:
            print(str(player))
    winner = get_winning_player(players)
    print(f"Winner is {winner}.")
    write_score(winner)
    print("Highscores: ")
    show_highscores()


if __name__ == "__main__":
    game()

---

NOTE

The rewrite allows for multiple players, along with option to set multiple rounds (NUM_PLAYERS and NUM_ROUNDS).

Answer 2

There's a lot in your code that can be refactored but let's start from the beginning with some Python styleguides (also called PEP8)

Imports

It's recommended to write all the imports at the top of your file.

Naming

In Python, the name of functions and variables should be snake_cased. That is, instead of def Roll() you should have def roll(), instead of Player1Score you should have player1_score and so on. You got the idea. Read more about it here

Say NO to `;` in Python!

Don't use ; in Python. It's not needed, and it makes me remember the hard times I was using C / C++. You don't want me to be sad, do you? :(

General

It's usually good practice to try to avoid inline blocks of code. It's hard to follow and it doesn't have any benefits. This:

if Player1Score > Player2Score: Winner = 1

Should be written as:

if Player1Score > Player2Score: 
    Winner = 1

That said, your code so far, taking the above advice into consideration, would look like this:

import random


try:
    file = open("Users.txt", "r")
except FileNotFoundError:
    raise SystemExit("User file not found")
file = open("Users.txt", "r")


def same_score():
    input("Press enter to roll dice: ")
    print("")
    dice_1 = random.randint(1, 6)
    dice_2 = random.randint(1, 6)
    print("player 1 rolled:", dice_1)
    print("player 2 rolled:", dice_2)
    if dice_1 == dice_2:
        return False
    if dice_1 > dice_2:
        return 1
    if dice_1 < dice_2:
        return 2


def login(username, player):
    file.seek(0)
    for line in file:
        valid_username = line.split(",")[0]
        valid_password = line.split(",")[1].replace("\n", "")
        if username == valid_username:
            password = input("password: ")
            if password == valid_password:
                print("player", player, "logged in")
                print("")
                return True
            else:
                print("Invalid Details")
    return False


def roll():
    dice1 = random.randint(1, 6)
    dice2 = random.randint(1, 6)

    print("You rolled a", dice1, "and a", dice2)

    change = dice1 + dice2
    change += 10 if (dice1 + dice2) % 2 == 0 else -5

    if change < 0:
        change = 0

    if dice1 == dice2:
        dice3 = random.randint(1, 6)
        print("Your third roll is a",dice3)
        change += dice3

    print("")
    return change


try:
    while True:
        print("player 1 login")
        username1 = input("username: ")
        if login(username1, 1):
            break
        print("")

    while True:
        print("player 2 login")
        username2 = input("username: ")
        if username1 == username2:
            print("Double login detected")
        elif login(username2, 2):
            break
        print("")
except KeyboardInterrupt:
    raise SystemExit("Exiting...")
finally:
    file.close()

player1_score = 0
player2_score = 0

for x in range(5):
    print("Play:", x + 1, "starting")
    input("player 1, press enter to roll: ")
    player1_score += roll()

    input("player 2, press enter to roll: ")
    player2_score += roll()

    print("player 1 now has a score of", player1_score)
    print("player 2 now has a score of", player2_score)
    print("")

if player1_score > player2_score:
    winner = 1

if player1_score < player2_score:
    winner = 2

if player1_score == player2_score:
    print("You both got the same score")
    winner = False
    while not winner:
        winner = same_score()

if winner == 1:
    winner = username1 + ": " + str(player1_score)
    print(username1, "won with", player1_score, "points")
    print(username2, "lost with", player2_score, "points")

if winner == 2:
    winner = username2 + ": " + str(player2_score)
    print(username2, "won with", player2_score, "points")
    print(username1, "lost with", player1_score, "points")

winner_score = int(winner.split(": ")[1])
file_written = False
try:
    file = open("Scores.txt", "r")
    data = file.readlines()
    file.close()
    for x in range(len(data)):
        if winner_score > int(data[x].split(": ")[1]):
            data.insert(x, winner + "\n")
            if len(data) > 5:
                data.pop(5)
            file_written = True
            break

    if len(data) < 5:
        if not file_written:
            data.append(winner + "\n")

    file = open("Scores.txt", "w")
    for x in data:
        file.write(x.replace("\n", "") + "\n")
except FileNotFoundError:
    file = open("Scores.txt", "w")
    file.write(winner + "\n")
file.close()
print("")

file = open("Scores.txt", "r")
print("Highscores:")
for line in file:
    if line != "":
        print(line.replace("\n", ""))
file.close()

Now, this is a bit better. Let's discuss the actual implementation now.

Here:

try:
    file = open("Users.txt", "r")
except FileNotFoundError:
    raise SystemExit("User file not found")
file = open("Users.txt", "r")

You're trying to open a file, and if it doesn't exist you raise and exception. There's nothing necessarily wrong with this, but notice how you open the file two times if there's no exception raised. If you want to check if a file exists or not, you could use the os module to confirm if a filepath exists and create a function which does that. Also, make the filepath a constant and put it at the top of your script:

import os


USERS_FILEPATH = '/path/to/Users.txt'


def check_file(filepath):
    """Verify if a filepath exists.
    
    Return True if a filepath exists. Otherwise raise an exception.
    
    Arguments:
        filepath (str): Path to a file
    
    Returns:
        True or raise an exception
    """
    if not os.path.exists(filepath):
        raise OSError('User filepath {} not found.'.format(filepath))
    return True

In order to call the function you can do:

if check_path(USERS_FILEPATH):
    # do things here

There are a couple of new things here. First, notice how I added a docstring to this function and how easy it is to tell what it actually does. Second, notice how easy it is to use this function on any other files that your game might make use of. Third, look at how the string is formatted when I raise an exception. You can read more about Python's strings formatting here.

//have to go for now