9
\$\begingroup\$

This is a finished project where 2 players each roll 2 dice. If the sum of the number that a player has rolled is even, then 10 is added to their score; if the sum of their 2 rolls isn't even then 5 is subtracted from the player's score. If the player's first role = the second roll, then another roll is done and added to their score. And the end of 5 rounds, whoever has the highest score wins - if necessary, tiebreaker rounds will be conducted in order to determine the winner.

After that, the winner's score must be saved in an external file and then the top 5 highest scores are fetched from the file and ordered to form a leaderboard-style message.

The code has a substantially high amount of comments because this was my GCSE computer science coursework (NEA) for 2020.

As far as I know, this was the last year of the Programming Project (at GCSE-level, that is) which is sad as I believe GCSE students should have some sort of experience with designing and altering a python programme, based on a specification.

The code shouldn't be hard to understand due to the amount of comments, lol.

My teachers graded this coursework a 9 (A*, on the old UK grading system).

from random import randint # import the randint function from the random package; can be accessed via randint()
from typing import List # support for type annotations (prevents  my linter from throwing errors)
from time import sleep # import sleep from the time module
from operator import itemgetter as ItemGetter # Import the itemgetter module; this makes it easier to manipulate 2 dimensional lists. represents a callable object that fetches the given item(s) from its operand. 

usernames = ['user', 'admin'] # list of valid usernames;
pins = ['2369', '1642'] # list of valid PINS (PIN for usernames[0] is pins[0]); 
active_players = [] # List of active players, prevents a user from logging in twice under the same username
rounds = 0 # int | Number of rounds that have passed, used to ensure only 5 rounds have gone before a winner is determined
# Initiate score trackers for both players. 
player1_score = 0
player2_score = 0
carry_on = True # Whether or not rounds should be ran
# Initiate tracker for the winner's name and score
winning_player = None
winning_score = None
tie_breaker = False # Whether tie breaker (tie_breaker) mode is enabled 
sep = '------------------------------------------' # used as a separator for each round, purely to be aesthetically pleasing

class InputMessages:
    username: str = f"Player $playernum: please enter a valid username: "
    pin: str = f"Player $playernum: please enter the PIN for $player: "
    welcome: str = f"\nWelcome $player, you've successfully logged in!"

print("Welcome!\nYou need to be logged in to access this game.")
while len(active_players) < 2: # while the number of usernames in the active_players list is less than 2:
    # input and validate username:
    _id = f"(Player {len(active_players) + 1}): "
    username = str(input(f"{_id}Please enter a valid username: ")).lower() 
    while username in active_players: # if the username is in the active_users list, completely prevent the user from logging in as that user by creating a loop that can only be broken if the user enters a username that is not in this list;
        print(f'Someone is already logged in by that username!')
        username = str(input(f"{_id}Please enter a valid username: ")).lower() # re-assign the new inputted value to the username var, allowing the loop to break if the user has entered a valid username (that is not already taken;
    while not username in usernames: # If a username is not in the usernames list, create a loop which will ask the user to re-input a valid username

        print(f'A user by that username was not found!')
        username = str(input(f"{_id}Please enter a valid username: ")).lower() # re-assign to the new inputted value to "username", allowing the loop to break if the user has entered a username that is not in the active_players list.
    username_index = usernames.index(username) # Find the "username" that the user has enterd's position in the pins list. This will allow me to check if the pin provided is the CORRECT pin for the user identified; 
    # check pin (relative to the username inputted above)
    pin = str(input(f"{_id}Please enter the PIN for {username.capitalize()}: ")) # prompt user to input their "pin" (Type: str)
    while not pins[username_index] == pin: # If the pin for the identified user is incorrect, instantiate a loop asking user to input the correct PIN
        print(f"Invalid PIN! Please try again")
        pin = str(input(f"{_id}Please enter the PIN for {username.capitalize()}: ")) # re-assign the new inputted value so the loop takes the new value into account
    print(f"{_id}Welcome {username.capitalize()}! You're now logged in.")
    active_players.append(username) # Add the username to the end of the active_users list 

player1 = active_players[0].capitalize() # Assigns the active_players[0] username to the player1 variable, purely for convenience 
player2 = active_players[1].capitalize() # Assigns the active_players[1] username to the player2 variable, purely for convenience 

'''
This function will roll a dice and perform the required actions upon the returned value (if even, +10, if odd, -5 etc)
param {bool} Tie_breaker: Whether or not tie breaker mode is active
'''
def roll(Tie_breaker: bool = False): 
    if Tie_breaker == True: # If the Tie_breaker boolean param is True, only return 1 random int between 1 and 6 (tie breaker mode rules)
        return randint(1, 6) 
    score = 0 # The final number of points that the <user> has scored.
    # Generate a random number between 1 and 6
    roll1 = randint(1, 6)
    roll2 = randint(1, 6)
    roll3 = randint(1, 6)
    score = roll1 + roll2 # Add together all of the rolls and store them under one variable, making it easier to manipulate;
    if score % 2 == 0: # Check if the score is an even number; % just divides the number and returns the remainder of the division;
        # Even number, +10 to the user's score.
        score += 10
    else:
        score -= 5 # User rolled an odd number; -5 from their score
    if score < 0: # Checks if the int score is less than 0; if it is, set it to 0
        score = 0
    if roll1 == roll2: # If roll1 equates to roll2, then add the value of roll3 to the final, total score
        score += roll3
    return score

'''
The following function will return either None, 1 or 2 depending on who out of the 2 players now has the highest score
param: {int} score1 --> Player 1's Score to take into account; 
param: {int} score2 --> Player 2's Score to take into account 
returns: 
    --> 1 IF score1 > score2
    --> 2 IF score2 > scrore1
    --> None IF score1 == score2
'''
def identify_winner(score1: int, score2: int): 
    if score1 == score2: # If score 1 is equal to score2, it is a draw: 
        return None
    # elif acts as a shorthand for else if
    elif score1 > score2: # Player 1 has a higher score than player 2; return 1. 
        return 1
    else: # Player 2's score is greater than player 1's score; return 2.
        return 2

while rounds < 5 or tie_breaker == True and (carry_on == True): # Instantiate a loop: this will continue if the number of rounds (rounds) is less than 5 **OR** if tie breaker (tie_breaker) is set as True **AND** if carry on is as True. Since this part is in parenthesis, this will be checked first 
    rounds += 1 # Increment number of rounds
    input(f"Press Enter to proceed to round {rounds} ")
    print("Rolling Die...") 
    sleep(0.5) # wait for 1/2 seconds [just to add effect];
    print(sep)
    print(f"Round No: {rounds}")
    if tie_breaker != True: # make sure that 2 dice are only rolled if tie breaker mode is not active.
        # Roll die and perform actions based on the number
        player1_scored = roll()
        player2_scored = roll() 
        # Add the amount of points scored to the player who scored them
        player1_score += player1_scored 
        player2_score += player2_scored 
    else: # If the above condition if False, then we know that tie breaker mode is active.
        # Call the roll function with the tie_breaker param as True
        player1_scored = roll(True)
        player2_scored = roll(True)
        player1_score += player1_scored 
        player2_scored += player2_scored
    new_winner = identify_winner(player1_score, player2_score) # Utilise the identify_winner() function and determine a winner by the result of this function;
    
    if new_winner == None: # If None is returned, THEN there is a DRAW;
        winning_player = 'DRAW' # set the winning_player to draw; this will be displayed below;
        winning_score = player1_score
    elif new_winner == 1: 
        winning_player = player1 # set the winning player as player1
        winning_score = player1_score 
    else: 
        winning_player = player2 # set the winning player as player2
        winning_score = player2_score

   #     Utilise the "ternary" (compressded if statements) operator to reduce the hassle of a huge if-elseif-else block. 
    player1_display_gained_points = "did not gain any additional points" if player1_scored == 0 else f"has gained {player1_scored} points"
    player2_display_gained_points = "did not gain any additional points" if player2_scored == 0 else f"has gained {player2_scored} points"
    print(f"{player1} {player1_display_gained_points}")
    print(f"{player2} {player2_display_gained_points}")
    print(sep)
    print(f'''
{player1}'s Score: {player1_score}
{player2}'s Score: {player2_score}
    ||    Currently Winning Player: {winning_player} (score {winning_score})
{sep}
    ''')

    if rounds >= 5: # IF the number of rounds incurred (rounds) is greater than or equal to 5, attempt to determine a winner or start tie breaker; 
        if new_winner != None: # Only attempt to determine a winner if new_winner != None, ensuring that it is NOT a draw
            print("🎉 WE HAVE A WINNER! 🎉")
            print(sep)
            print(f'''
{winning_player} has won the game with their score of {winning_score}!
            ''')
            # Prevent further rounds from being ran => 
            carry_on = False 
        else:
            # Enable tie breaker mode 
            print("It seems to be a draw; tie breaker mode will now be activated.")
            print("This means that each user will now only have 1 roll each and any odd/even bonuses will not be applied henceforth.")
            if tie_breaker != True:
                tie_breaker = True

'''
INTENDED FORMAT FOR EXTERNAL FILE:
PlayerName:PlayerScore
where PlayerName represents a player's name 
where PlayerScore represents a player's score
    - If the external file is manually edited to a format different to this, it will result in the production of error(s) 
'''
to_w = f"{winning_player}:{winning_score}" # What to write to the external file

# Use python with open() to open the file, as target followed by a block of code.
# This reduces how cluttery the code is and makes it easier to manage resources, such as file streams as utilised below: 
# List of all winners, read from ext file and the winner of this game

winners = [] # this will be the new content of the external file 
# This code will prevent the write file to completely wipe it each time; it will create some sort of persistence in regards to data in that file. 
with open('./winners.txt', 'r') as f: 
    for line in f.readlines(): 
        # for each line in the file:
            # add the line into the winners list, ensuring \n (newline character) is removed
        winners.append(line.replace('\n', ''))
    winners.append(to_w) # add the winner of this game
    f.close() # close file

with open('./winners.txt', 'w') as f:
    f.write('\n'.join(winners)) # write to the file;
    f.close() # Close the file after it has been written to.

data = [] # This will be the final, formatted, ready-to-sort list

for w in winners: # for every value in the winner_cache list:
    data.append([str(w.split(':')[0]), int(w.split(':')[1])]) # create a new 2d list with the name and the score of the player
# Now we sort the data based on the int score
# Utilise python slice notation on the returned list --> 
sorted_data = sorted(data, key = ItemGetter(1), reverse = True)[:5];

# map through the list, converting the end result to a list.
msg = list(map(lambda arg: f"{arg[0].capitalize()} ({arg[1]} points)", sorted_data))

print(f"Top 5 scores:\n(loaded from ./winners.txt)")
printed = 0 # Amount of printed scores 

# Loop through the printing of each player name and score 
for x in range(0, len(sorted_data)):
    printed += 1
    print(str(x + 1) + '.', msg[x]) # --> 1. Userz (10 points)
                                    #     2. Admin (5 points)
                                    #   ...
        
# Print that not data is found if no data is found - simply reduces confusion for the End User 
while printed < 5: # count controlled loop; only run while printed less than (<) 5
    printed += 1
    print(f'{printed}. -- No Data Found --')

Here's the link to the GitHub in case I copied this over wrong - GitHub

Hopefully this has also helped somebody else in the process.

Thanks for reading :)

\$\endgroup\$
2
  • \$\begingroup\$ Is the list of valid users public information? If not, you have a security issue. \$\endgroup\$ Oct 13, 2021 at 21:36
  • 1
    \$\begingroup\$ "The code shouldn't be hard to understand due to the amount of comments". This actually makes the code much harder to understand \$\endgroup\$
    – Ivo
    Oct 14, 2021 at 7:46

3 Answers 3

25
\$\begingroup\$

This… … … is a mouthfull. The sheer amount of comments make the code very hard to read as it gets buried under tons upon tons of other information. Most of them being just there to state the obvious of what the line of code is doing. This is not what comments are about. Comments, if any, should state why we went some route or another and explain the rationale of not doing it any other way. They should be here to prevent the reader being caught off-guard by a piece of code that may seem to be overly complicated or otherwise non-obvious.

You don't need to explain that you're importing a function from a module: we just read the import keyword on the line, so we know that. You don't need to say that you’ll sort a list: you're using sorted, we will figure it out; maybe explain why you need the list sorted instead, or use a better variable name than sorted_data to explain it for you, like leaderboard for instance.

So clean up the code from the unnecessary, remove the unused bits too (such as the InputMessages class) and it will be much easier to read.

Use functions

Your code is almost all linear. Even though it works, it makes it hard to think about it in terms of logical units. Defining functions will make it easier to digest and reason about. It will also allows you to define docstrings that act as documentation and will let you get rid of most of the comments as you can explain the whys of these functions into the docstring themselves.

Note that docstrings are defined right after the function declaration, not before.

Using functions will also help get rid of this ugly pile of variable definition at the top. Variables should be defined when you need them, so it's easier to go back and forth between their usage and their initial value, and having functions will help you manage their scope so you only need to care about a few of them at once.

Lastly, breaking your code into functions will make it easier to test and reuse.

Improve your file handling

First off, you don't need to close your files manually, as the with block will do it for you.

Second, to add a line to a file, there is absolutely no need to read it first and write it back entirely. The append mode (mode='a') is there for exactly this purpose: it opens the file and seeks to the end so any write will happen after existing content.

And third, the print function can help you write to a file. Note that print can handle any type and help you avoid type conversions all around in your code as well.

Handling Data

You have two lists to handle your credentials and their order is dependant on each other. That's not the job for two lists; it's more suited to a dictionary.

Same for the users names and their score: you can store that in a dictionary and be set for the day you want to play at 3 or 5 users at once.

A lot of your comparisons are unnecessarily complicated and error-prone: if something == True should be written if something: to remove the unwanted repetition and be able to leverage the comparisons of anything in a boolean context if necessary (for example the fact that empty collections evaluate to False).

Remember that collections are iterable and you can get elements inside of them using a simple for element in collection: rather than using a index and retrieving the elements yourself by hand. Watch the excellent loop like a native talk for more on the subject.

Proposed improvements

from operator import itemgetter
from random import randint
from time import sleep

# Used as a separator for each round, purely to be aesthetically pleasing
SEP = '------------------------------------------'

CREDENTIALS = {
        'user': '2369',
        'admin': '1642',
}


def get_logged_on_player(credentials, exclude_players=set()):
    """Ask a user for their name and their PIN and return the username
    if successfully authenticated.

    This function checks the :credentials: dictionnary for valid usernames
    and ask the players to authenticate using their PIN. Keep asking until
    a valid username and the corresponding PIN are entered by a player.

    If a provided username is found in :exclude_players:, this function
    forbids them for logging again.
    """

    player_id = f"Player {len(exclude_players) + 1}:"
    while True:
        username = input(f"{player_id} Please enter a valid username: ").lower()
        if username in exclude_players:
            print('Someone is already logged in by that username!')
            continue
        if username not in credentials:
            print('A user by that username was not found!')
            continue
        pin = credentials[username]
        pin = input(f"{player_id} Please enter the PIN for {username.capitalize()}: ")
        if pin == credentials[username]:
            print("Welcome ", username.capitalize(), "! You're now logged in.", sep='')
            return username
        print("Invalid PIN! Please try again")


def roll(tie_breaker: bool=False): 
    """Roll a dice and perform the required actions

    Rules define the following actions:
     - add the first two dice to get the initial amount of points
     - first two dice add up to an even number => +10 points
     - first two dice add up to an odd number => -5 points
     - first two dice are equal => add the value of the third die to the points

    If the tie_breaker mode is requested, only return the value of a single die.
    """

    if tie_breaker:
        return randint(1, 6)

    roll1 = randint(1, 6)
    roll2 = randint(1, 6)
    roll3 = randint(1, 6)

    score = roll1 + roll2
    score += 10 if score % 2 == 0 else -5
    if roll1 == roll2:
        score += roll3

    return max(score, 0)


def display_score(username: str, points: int):
    """Pretty prints how much point a user gained in a round"""

    if points > 0:
        print(username, 'has gained', points, 'points')
    else:
        print(username, 'did not gain any additional points')


def play_a_round(players, round_number=None):
    """Simulate a round of the dice roll game

    Display the scores along the way and a summary of the
    scores so far at the end of the round.

    If no :round_number: is provided, simulate a tie-breaking round 
    """

    input(f"Press Enter to proceed to round {round_number or 'tie-breaking'}")
    print("Rolling Die...")
    sleep(0.5)  # Cosmetic pause, add dramatic effect
    print(f"{SEP}\nRound No: {round_number or 'tie-breaking'}")

    for player in players:
        score = roll(bool(round_number))
        display_score(player, score)
        players[player] += score

    # Roll our own `max` implementation to search for draws
    # Use the loop to display scores as well
    print(SEP)
    best_score = 0
    best_player = None
    for player, score in players.items():
        print(player, "'s Score: ", score, sep='')
        if score > best_score:
            best_score = score
            best_player = player
        elif score == best_score:
            best_player = None

    print(f"    ||    Currently Winning Player: {best_player or 'DRAW'} (score {best_score})\n{SEP}")
    return best_player


def play(player_count=2, rounds=5):
    """Run several rounds of the dice roll game and return the overall winner"""

    print("Welcome!\nYou need to be logged in to access this game.")
    players = {}
    while len(players) < player_count:
        player = get_logged_on_player(CREDENTIALS, players)
        players[player] = 0

    for r in range(rounds):
        winner = play_a_round(players, r + 1)

    while winner is None:
        print("It seems to be a draw; tie breaker mode will now be activated.")
        print("This means that each user will now only have 1 roll each and any odd/even bonuses will not be applied henceforth.")
        winner = play_a_round(players)

    score = players[winner]
    print("🎉 WE HAVE A WINNER! 🎉")
    print(f"{SEP}\n\n{winner.capitalize()} has won the game with their score of {score}!\n")

    return winner, score


def save_winner(winner, score, filename='winners.txt'):
    """Add a new entry to the provided file with a new winner

    Added entry is a single line of the form
    PlayerName:PlayerScore
    """

    with open(filename, 'a') as f:
        print(winner, score, sep=':', file=f)


def leaderboard(filename='winners.txt', limit=5):
    """Read from the provided file and prints the best :limit: players

    INTENDED LINE FORMAT FOR EXTERNAL FILE:
    PlayerName:PlayerScore

    If the external file is manually edited to a format different to
    this, it will result in the production of error(s).
    """

    scores = []
    with open(filename) as f:
        for line in f:
            name, score = line.rsplit(':', 1)
            scores.append([name, int(score)])

    leaderboard = sorted(scores, key=itemgetter(1), reverse=True)[:limit]
    print("Top", limit, "scores (loaded from", filename, "):")

    for i, (name, score) in enumerate(leaderboard, start=1):
        print(i, ". ", name.capitalize(), " (", score, " points)", sep='')

    # Add missing entries to always have a display of :limit: items
    for i in range(len(leaderboard), limit):
        print(i + 1, ". -- No Data Found --", sep='')


if __name__ == '__main__':
    winner, score = play()
    save_winner(winner, score)
    leaderboard()
\$\endgroup\$
1
  • 1
    \$\begingroup\$ For that matter, you don't even need the third roll unless the first 2 are the same. And there should probably be a completely separate function for tie-breakers, rather than having roll do two different things depending on a boolean argument. \$\endgroup\$
    – chepner
    Oct 14, 2021 at 20:52
17
\$\begingroup\$

You got an excellent grade for this assignment, but there is still room for improvement!

Readability

First and foremost, you'll learn that code readability is very important, as code is harder to read than to write but is read more often than written, and should be a prime consideration.

Unfortunately, various factors make your code harder to read than necessary.

Most of them are covered in the official Python style guide known as PEP8. It's good practice to follow it, as it is designed to produce readable code, and it makes most Python code consistent with your own style, and thus easier to read.

Although I'll point to some specific issues regarding your code, I'd recommend reading it.

Use of comments

You said:

The code has a substantially high amount of comments because this was my GCSE computer science coursework (NEA) for 2020.

Even with that context, there are a lot of comments, and a lot of them are redundant or out of place, and it adds a lot of unnecessary clutter.

Take your first line for example:

from random import randint # import the randint function from the random package; can be accessed via randint()

The first part of the comment is barely parroting the actual code, it even uses the same words ("import", "from", "random", "randint"). It adds no information.

The second part of the comment is not a code comment, it's a personal note. It would make more sense to keep an annotated version of this code in your class notes than including the notes in the actual code:

  1. when you'll need to read your notes for future reference, you will have a harder time finding the relevant code and annotation than if you had proper notes structured as class notes
  2. it clutters the code and makes it unnessessarily harder to review, whether by your teacher who graded it, by yourself in the future if you want to use it as a reference, or by a benevolent reviewer on the internet.

I'd argue that at least 90% of the comments can be removed, and most of the remainder can be replaced by a more explicit variable name. For example you can simplify this line:

tie_breaker = False # Whether tie breaker (tie_breaker) mode is enabled 

to:

enable_tie_breaker = False

without losing any information.

Vertical whitespace

Give your code some space to breathe. There are few blank lines, making it harder to visualize the various logic blocks to read.

PEP8 recommends 2 empty lines around top-level function definitions, and 1 empty line between logical sections, which you already mostly do.

Loooooong lines

PEP8 recommends a maximum line length of 80 characters. Some people feel like it's safe to assume no-one will ever read modern code on 1980's terminals and bump this number up a bit, usually to 100 characters.

You, however, have lines extending to almost 300 characters, and if you exclude comments to 140 characters. This is bad, as it forces to scroll horizontally, which is anti-ergonomic.

Naming conventions

Once again, PEP8 has an opinion on that. Variables should be snake_case, which you did, and constants should be ALL_CAPS. This allows the reader to immediately identify constants as such, and know not to worry about the value of the constant. Also, use the most descriptive names you can think of, and avoid abbreviations unless they are clear in every context.

For example print(SEPARATOR) is immediately clear that the printed value is not a result from prior computation, but just a separator. print(sep) is not.

Docstrings

You documented your functions with docstrings, eg. triple-quoted strings describing their usage. This very good. However, they should be placed after the function definition, not before. Furthermore, PEP8 recommends limiting the line length to 72 characters, as when they are displayed through the help function, they are indented a couple more levels. Example for one of your functions:

def identify_winner(score1: int, score2: int): 
    '''
    The following function will return either None, 1 or 2 depending on
    who out of the 2 players now has the highest score
    param: {int} score1 --> Player 1's Score to take into account
    param: {int} score2 --> Player 2's Score to take into account 
    returns: 
        --> 1 IF score1 > score2
        --> 2 IF score2 > scrore1
        --> None IF score1 == score2
    '''
    # Do stuff

Code structure

Global variables

You use global variables to keep track of the game state. Global variables are considered bad practice, the main reason is making it hard to keep track of their values (and where these values are acted upon) when following the code logic. You should keep the scope of variables as small as possible.

Having global constants, however, is acceptable and in fact quite often used, as long as they are clearly identified as constants (see above).

Hardcoded values

The game is played in 5 rounds. This value is hard-coded, and only found in the line while rounds < 5.

Say you (or someone else) want to change that number for some reason (maybe the game is too short/long? Maybe you want to test the file I/O without waiting through 5 rounds?), finding where it is in the code is harder than needs be. Make it a named constant at the top of the script.

Same goes for the 0.5 used as an argument for sleep or the path to the high score text file.

Constant class

The InputMessages class exists only to holds a few constant strings. This is very unusual, a dictionary is usually the solution for this use case.

Although I wouldn't say doing it your way is wrong, a class is overkill as it can do a lot of other things, whereas a dict is specifically designed for holding and accessing key-value pairs, which is what is done here.

Also, since you're using a data structure (either a dict or a special class) to hold constant strings, you really should put all of your strings there, yet many are baked right in the middle of the code.

Separation of concerns / lack of structure

Most of your code unroll at the top level, making little use of functions and interleaving actions with different scopes (game logic, user I/O, file I/O).

I suppose, at first, this makes perfect sense when writing the code, as everything is coded in the order it happens when playing the game: log in, initialize game, say game is about to start, roll, display results...

However, it hurts readability, reusability, and maintenance. Say you want to change something in the way things are displayed to the player, or I want to review specifically how logging in is handled, or you want to reuse your high score saving logic... We now have to read through the whole code to find the relevant section, then do whatever we want.

The way to solve this is to:

  • use more functions, each abstracting how to do a specific thing, preferably one at a time.
  • use classes to group things that belong together

For a simple game like this, I think I wold have a class for the game logic keeping track of the game state and advance it, and a class for the player.

You should have very little code left at the top level.

The code I would write may look something like this:

def ask_for_input(prompt)
    value = input(prompt)
    # Validate input
    return value


class Player
    '''a player for the dice roll game'''
    
    def __init__(self, name)
        self.name = name
        self.score = 0
    
    @staticmethod
    def from_login(username, pin):
        if [username matches pin]:
            return Player(username)
        return None


class DiceGame
    '''A simple dice game'''
    
    high_score_path = './winners.txt'
    
    def __init__(self, round_number, player_1, player_2):
        self.round_number = round_number
    
    def run(self)
        round = 0
        while round < self.round_number and self.identify_winner() is not None:
            # Main game loop
        self.add_to_high_score(identify_winner())
        self.display_high_scores()
    
    def play_round(self)
        # Do stuff
    
    def display_scores(self)
        # Do stuff
    
    def identify_winner(self)
        # Do stuff
    
    def roll(self):
        # Do stuff
    
    def add_to_high_score(self, player):
        # Do stuff
    
    def display_high_scores(self):
        # Do stuff


if __name__ == '__main__':
    ROUND_NUMBER = 5
    
    payers = []
    while len(players) < 2:
        player = Player.from_login(
            ask_for_input('Please enter your username: '),
            ask_for_input('Pease enter your PIN: '))
        if player is not None:
            players.append(player)
    
    game = Game(ROUND_NUMBER, players[0], players[1])
    game.run()

Note how there are only 10 lines of logic left outside of any class. I could now work on each of the aspect of the game one at a time, and come back to any of them later if I'm no longer satisfied with them.

Also note I included all of these lines after a if __name__ == '__main__': statement. This means they will get executed if you run this script, but not if you import this script from another one, an re-use the game logic in a broader project (for example, a project that lets you play this game or another one).

Random things

with open('./winners.txt', 'w') as f:
    f.write('\n'.join(winners))
    f.close() 

You don't need the f.close() here, the file is closed automatically at the end of the with statement.


At one point, you compare a value to None this way: if foo == None.

The right way of comparing to None is if foo is None. Indeed, for some objects, foo == None may return True although foo is None is False, depending on how the == operator is defined for foo.

Further reading on the subject on StackOverflow


sorted_data = sorted(data, key = ItemGetter(1), reverse = True)[:5];

Glossing over the extra ; at the end of the line, this creates a sorted copy of the list. This is wasteful, as you don't need the unsorted list anymore.

Instead, you can sort the data list: sort(data, ...)


I hope you do realize that this is not even remotely close to the proper way to handle secrets like a PIN to log in a player. I won't go into detail on how to do that, as I feel it is a requirement for the assignment, but not related to the game per se.

Going further

If you want, as a further exercise, I can think of a few things to bring this program a little further.

First of all, I found it frustrating to see the points total score gained in each round, wouldn't it be more exciting to see the outcome of each roll individually?

Also, overall, this just amounts to an overengineered coin toss, there are no player decision, which is not much fun to play. Maybe you could add a betting mechanic to it, to make it interesting? It also gives the possibility to have a human or computer player.

Have fun coding!

\$\endgroup\$
3
  • \$\begingroup\$ Hello yes I have read your reply - I understand that the sheer number of comments makes it unreadable and I will work on this in the future. In regards with using f.close() when in the with block, I knew the with block would automatically close however I needed to show that I knew that we had to close a file after opening it, just to show that I know the method exists. In regards to the extra semicolon on the sorted_data = ... line, that's my bad - I'm usually coding in JavaScript and semicolons are pretty essential there so it gets in bad habit. \$\endgroup\$ Oct 15, 2021 at 7:10
  • \$\begingroup\$ I totally agree with you in terms of the comments and everything you've said about the structure and logic about the code. At the time of writing the assignment I was really overwhelmed by other subjects too, such as Chemistry etc, and whilst under a certain time frame so it was quite stressful. Also, I have wayyyy more experience in JavaScript/nodejs, and only really used Python for this coursework. \$\endgroup\$ Oct 15, 2021 at 7:12
  • \$\begingroup\$ I never really knew how to utilise a class system in PY and I didn't really know a dict exiisted LOL, from what you've shown me, a dict looks like a JavaScript object (obj={key:"value",key0:"value0"})`. But I 100% agree with the things you've said, and thank you for providing feedback on this. I'll be sure to take it on further. I was also aware that this is by no means a good way to have someone's PIN stored, it was only really a requirement to have a login system and there was no additional security required. \$\endgroup\$ Oct 15, 2021 at 7:14
1
\$\begingroup\$

Something else that you might consider is using a different random number generator than the one in the standard library when you are working with simulations.

The standard library's implementation uses the Mersenne Twister, which is alright, but it has some drawbacks. Specifically, the statistical properties of the generated output tend to be differentiable from a truly random sequence. This is obviously why it shouldn't be used for cryptographic purposes, but the deviation is significant enough that it can cause problems in things like Monte Carlo simulations.

This is obviously not a Monte Carlo simulation, but I would recommend getting acquainted with the random number generator in the Numpy library anyways, since it not only outperforms the Mersenne Twister (both statistically and in actual speed), but getting used to using Numpy for numerical python programming is a good idea, since it's pretty much the standard. The generator itself is based on the PCG64 family of random number generators (you can read the original paper introducing them here), and instantiating and using the generator isn't too much more involved.

To obtain random deviates sampled from a uniformly distributed sample space consisting of the integers from 1 to 6, inclusive, simply instantiate the random number generator as follows.

from numpy.random import default_rng

# Default generator uses PCG64.
rng = default_rng()

# Simulate a simple dice roll. Note that the endpoint
# parameter must be explicitly set to True, as the
# default behavior is to assume a half-open interval
# which excludes the high point.
dice_roll = rng.integers(low=1, high=6, endpoint=True)
\$\endgroup\$
2
  • \$\begingroup\$ Suggesting learning numpy for a simple dice rolling game is overkill \$\endgroup\$
    – qwr
    Oct 15, 2021 at 8:33
  • \$\begingroup\$ @qwr I only suggested using the rng from the numpy random module, although I respectfully disagree that it's overkill. The goal is not overengineering a simple task, but rather using these learning exercises to gain exposure to the standard implementation methodologies and processes used in professional environments, is it not? Although in fairness I do agree that if you literally just wanted a dice game, learning numpy is overkill, although the rng suggestion I think remains valid \$\endgroup\$ Oct 15, 2021 at 8:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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