# NEA Computing Task 2 Dice Game

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

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:
File = open("Users.txt", "r")

File.seek(0)
for Line in File:
print("Player",Player,"logged in")
print("")
return True
else: print("Invalid Details")
return False

try:
while True:
print("")
while True:
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)
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)
if Winner == 2:
Winner = Username2 + ": " + str(Player2Score)
WinnerScore = int(Winner.split(": "))
FileWritten = False
try:
File = open("Scores.txt", "r")
for X in range(len(Data)):
if WinnerScore > int(Data[X].split(": ")):
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

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:
# 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

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

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:

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

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:
while True:
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
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

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).

• I might be being dumb here but is there any reason to create a one line definition? (The roll_dice() since it would be just as ok to assign the variable a random number instead of creating a def? Oct 21, 2020 at 17:32
• @Evorage so that when you require a change that instead of 6 sided dice, it should be 20-sided, the changes would be at just one place instead of updating 30 different places. Oct 22, 2020 at 0:33

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:
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

file.seek(0)
for line in file:
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)
change += dice3

print("")
return change

try:
while True:
break
print("")

while True:
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)

if winner == 2:
winner = username2 + ": " + str(player2_score)

winner_score = int(winner.split(": "))
file_written = False
try:
file = open("Scores.txt", "r")
file.close()
for x in range(len(data)):
if winner_score > int(data[x].split(": ")):
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:
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):
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

• If the "Users.txt" file is in the same directory (and will always be), is it important to specify the file path? Oct 21, 2020 at 17:09