7
\$\begingroup\$

I am a Python noob and over the last week it has been my goal to learn how to write python code. As my first project I decided to create a dice game called C low. This is a version of that game that my friends and I played in highschool. I want to ensure I follow Python best practices, also I'm sure there are easier more pythonic ways to write this program, and I am interested in those as well. How can I improve my code?

Game Rules:

  • Two players roll 3 dice to decide who won the hand.
  • 456 = best hand
  • 123 = worst hand
  • Triples beat doubles in order.
    • 666 beats 555 etc
  • For doubles only the remaining die matters
    • ex)
    • 661 = 1
    • 446 = 6
  • If your roll doesn't fall into one of these categories you roll again.

#Dice
#C LOW Highschool edition
import random
from collections import Counter as c
import time
#Create dice art

DICE_ART = ["""
 ------- 
|       |
|   N   |
|       |
 -------  ""","""
 ------- 
|       |
|   1   |
|       |
 ------- ""","""
 ------- 
| 2     |
|       |
|     2 |
 ------- ""","""
 ------- 
| 3     |
|   3   |
|     3 |
 ------- ""","""
 ------- 
| 4   4 |
|       |
| 4   4 |
 ------- ""","""
 ------- 
| 5   5 |
|   5   |
| 5   5 |
 ------- ""","""
 ------- 
| 6 6 6 |
|       |
| 6 6 6 |
 ------- """]


dice = [1, 2, 3, 4, 5, 6]


player_1 = []
player_2 = []
roll = [] 

#roll = ["3", "5", "6"]
def roll_dice():
    #This function will simulate rolling the dice.
    for r in range(0, 3): # Pick 3 dice
        r = dice[random.randint(0, 5)]  # At random
        r = int(r) #Capture the integer of the dice you rolled
        print(DICE_ART[r]) #Print the ASCII art of the roll
        roll.append(r) #Add dice to list roll
    print(roll)


def roll_again(player_name):
    if c(roll) != c([4, 5, 6]) and c(roll) != c([1, 2, 3]):
        while roll[0] != roll[1] and roll[0] != roll[2] and roll[1] != roll[2]:
            print("No matches, " + player_name + " is rolling again...")
            time.sleep(3)
            del roll[:]
            roll_dice()  


def winner(roll, player_name):
    # Decide who won by summing dice
    total = 0
    if c(roll) == c([4, 5, 6]):
        total = 456 #always wins
    elif c(roll) == c([1, 2, 3]):
        total = 0 #always loses
    elif roll[0] == roll[1] and roll[1] == roll[2]:
        total = sum(roll)    
        total = total + 100 # 3 of a kind beats 2 of a kind
    else:
        total = sum([item for item in roll if roll.count(item) == 1])    
    return total  
    print(player + " rolled a " + total)


def roll_statement(roll, player_name):
    #Create a statement to print based on the dice rolled
    if c(roll) == c([4, 5, 6]):
        print(player_name + " rolled a " + "4 5 6!")
    elif c(roll) == c([1, 2, 3]):
        print(player_name + " rolled a " + "1 2 3!")
    elif roll[0] == roll[1] and roll[1] == roll[2]:
        print(player_name + " rolled Triple " + str(roll[0]) + "'s" + ".")
    else:
        x = [item for item in roll if roll.count(item) == 1]  
        print(player_name + " rolled a " + str(*x) + ".")


def score_board(player_1, player_2):
    #Create a scoreboard to display at the end
    time.sleep(5)
    print("*********FINAL SCORE*********")
    print()
    print("           Player 1")
    print("           ________")
    lines = [DICE_ART[i].splitlines() for i in player_1]
    for l in zip(*lines):
        print(*l, sep=' ')
    print("")
    print("           Player 2")

    print("           ________")
    lines = [DICE_ART[i].splitlines() for i in player_2]
    for l in zip(*lines):
        print(*l, sep=' ')
    print() 
    print("*****************************")
    print()
    if player_1_score == player_2_score:
        print("        Tie! Play again!")      
    elif player_1_score > player_2_score:
        print("        Player 1 wins!")
    else:
        print("        Player 2 wins!")
    print()        
    print("*****************************")


play = True
while play:
    print("Player 1 is rolling...")
    time.sleep(3)
    roll_dice()
    time.sleep(2)   
    roll_again("Player 1")
    player_1 = roll[:]
    player_1_score = winner(player_1, "player 1")
    roll_statement(player_1, "Player 1")
    time.sleep(3)
    roll = []

    print("Player 2 is rolling...")
    time.sleep(3)
    roll_dice()
    time.sleep(2)
    roll_again("player 2")
    player_2 = roll[:]
    player_2_score = winner(player_2, "Player 2")
    #print("Player 2 rolled a: " + str(player_2) + "\n")
    roll_statement(player_2, "Player 2")   
    score_board(player_1, player_2)
    print("Would you like to play again?")
    play_again = input().lower().startswith('y')
    roll = []
    if not play_again:
        break
\$\endgroup\$
5
\$\begingroup\$

This is looking very good for just one week of learning Python, good job!

Comments

When writing comments, you should aim to document why you're doing something, not what you're doing.

Through using clear and meaningful variable and function/method names, it should be obvious what you're doing. The ideal situation is that you don't need any comments at all because your code is self documenting. This isn't always the case however.

for example this line

roll.append(r) #Add dice to list roll

This comment is redundant, the append method on a list appends an element to that list, you don't also need to say that it adds to the list.

Here is an example of a comment that's useful

total = total + 100 # 3 of a kind beats 2 of a kind

it isn't immediately clear what the significance is of adding 100 to the total. (you're saying why, not what)

As a side note, instead of writing total = total + 100, you can simply write total += 100

As for docstrings (comments which describe a method/function) they are generally triple quoted strings

"""this is a docstring"""

# this is a regular comment

For any comments describing a function, use a docstring instead.

#This function will simulate rolling the dice.

should become

"""This function will simulate rolling the dice."""

Readability

from collections import Counter as c

I would advise against renaming an import like this just to be able to write c instead of Counter. The code becomes far more readable if I'm looking at

Counter(roll)

instead of

c(roll)

Consider using named constants, instead of magic numbers. A magic number is a number that has no clear meaning on its own.

total = 456 #always wins

what if this looked like

total = ALWAYS_WINS

Now, we don't need the comment anymore, and as a reader, we don't even need to know what the number ALWAYS_WINS is. We can do a similar thing with 0.

Naming

This maybe also belongs in the readability section, but thought it deserves its own because naming is such an important aspect of writing readable and understandable code.

play = True

I would much prefer

playing = True

while playing:

is a lot more clear to me than

while play:

In your roll_dice method.

r = dice[random.randint(0, 5)]  # At random

here, you're using the random module to generate a random index and access it. There's a much easier and more readable way to access a random element from a list.

r = random.choice(dice)

And again, no need for a comment.

Following this, I would say that "r" is not a good name.

I would prefer roll as a name instead of "r" and maybe "rolls" as the name of your list instead of roll. If a variable is called "roll" I would expect a single value, not a collection like a list.

Misc. Comments

return total  
print(player + " rolled a " + total)

this print statement is unreachable code. If this is the actual indentation of your code and not a mistake in formatting when writing your question.

A return statement exits the function, code after a return statement will never be executed.

If you want to print it, simply change the order of the statements

print(player + " rolled a " + total)
return total  

while the following code will work perfectly fine

  if not play_again:
        break

I would prefer to see

  if not play_again:
        playing = False

This better documents intent in your code, again making it more readable and easier to follow.

in your roll_statement function, in 2 of the conditions you check

c(roll)

I would save this as a local variable and use it in both conditions instead

I would maybe consider moving the ASCII art to a file, and read in your list of art from there. It takes up a lot of space in your program.

And do you even need to use the Counter class in your program?

The Counter class is intended for when you want to know the number of occurrences.

If all you are doing is comparing a list of 3 values, your roll_dice could return a sorted list and you could compare them directly.

>>> roll1 = [1,3,2]
>>> roll2 = [2,3,1]
>>> roll1.sort()
>>> roll2.sort()
>>> roll1 == roll2
True

Edit

or as Mathias Ettinger pointed out, you can use the built in sorted.

>>> roll1 = [1,4,3]
>>> roll2 = [3,4,1]
>>> sorted(roll1) == sorted(roll2)
True

An important distinction between these two methods, is that the .sort method is a list method that sorts the list in place while sorted returns a new list and leaves the original unaltered.

Hopefully this review was useful!

\$\endgroup\$
  • \$\begingroup\$ Thank you for your well thought out reply. I will be trying to implement your suggestions in the future. \$\endgroup\$ – Ozixic Apr 26 '17 at 14:19

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.