HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonCritical

Monopoly simulator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simulatormonopolystackoverflow

Problem

I was advised by a Reddit user to get my code reviewed on this site.

The complete code is on GitHub.

```
# Monopoly Simulator
# http://img.thesun.co.uk/aidemitlum/archive/01771/Monopoly2_1771742a.jpg

from random import randint

piece = 0
jail = 0
iteration = 0

brown1 = 0
brown2 = 0

light_blue1 = 0
light_blue2 = 0
light_blue3 = 0

pink1 = 0
pink2 = 0
pink3 = 0

orange1 = 0
orange2 = 0
orange3 = 0

red1 = 0
red2 = 0
red3 = 0

yellow1 = 0
yellow2 = 0
yellow3 = 0

green1 = 0
green2 = 0
green3 = 0

dark_blue1 = 0
dark_blue2 = 0

num = input("How many rolls do you want to simulate? ")

for h in range(0, num):
piece += randint(2, 12)

if piece > 40:
piece -= 40
iteration += 1
#print("Around the board %d times so far" %(iteration)) ### Optional

#print(piece) ### Optional

#Jail
if piece == 30:
piece = 10
jail += 1
#print("JAIL") ### Optional

#Brown
if piece == 1:
brown1 += 1
if piece == 3:
brown2 += 1

#Light Blue
if piece == 6:
light_blue1 += 1
if piece == 8:
light_blue2 += 1
if piece == 9:
light_blue3 += 1

#Pink
if piece == 11:
pink1 += 1
if piece == 13:
pink2 += 1
if piece == 14:
pink3 += 1

#Orange
if piece == 16:
orange1 += 1
if piece == 18:
orange2 += 1
if piece == 19:
orange3 += 1

#Red
if piece == 21:
red1 += 1
if piece == 23:
red2 += 1
if piece == 24:
red3 += 1

#Yellow
if piece == 26:
yellow1 += 1
if piece == 27:
yellow2 += 1
if piece == 29:
yellow3 += 1

#Green
if piece == 31:
green1 += 1
if piece == 32:
green2 += 1
if piece == 34:
green3 += 1

#Dark Blue
if piece == 37:
dark_blue1 += 1
if piece == 39:
dark_blue2 += 1

brown = brown1 + brown2
light_blue = light_blue1 + light_blue2 + light_blue3
pin

Solution

You really need to remove some of your global variables.
As an example I'd change all your browns to use a list instead.

brown = [0, 0]


As you do a lot of logic over brown, cyan, pink, etc, I'd make a dictionary.
Dictionary's are like lists, they have a few more features that I'd use and are basically lists but can have 'any' key.

And so I'd use:

places = {
    'Brown': [0, 0],
    'Cyan': [0, 0, 0],
    'Pink': [0, 0, 0],
    'Orange': [0, 0, 0],
    'Red': [0, 0, 0],
    'Yellow': [0, 0, 0],
    'Green': [0, 0, 0],
    'Blue': [0, 0, 0]
}


A lot of your logic is based around where these places are.
To reduce duplicate information I'd make another dictionary, telling us which piece is there.
And so I'd use:

board = {
    1: ('Brown', 0),
    3: ('Brown', 1),
    6: ('Cyan', 0),
    8: ('Cyan', 1),
    9: ('Cyan', 2),
    11: ('Pink', 0),
    13: ('Pink', 1),
    14: ('Pink', 2),
    16: ('Orange', 0),
    18: ('Orange', 1),
    19: ('Orange', 2),
    21: ('Red', 0),
    23: ('Red', 1),
    24: ('Red', 2),
    26: ('Yellow', 0),
    27: ('Yellow', 1),
    29: ('Yellow', 2),
    31: ('Green', 0),
    32: ('Green', 1),
    34: ('Green', 2),
    37: ('Blue', 0),
    39: ('Blue', 1),
}


As you should see this is easier to use than a list, as we don't have to provide 0.
I would then go on to change almost all your logic to use these.
It will significantly reduce the amount of lines in your code and will make your code easier to read and understand.

First things first I'd change your first loop to use this. I'd not change iteration and jail as they don't fit into the objects we created above.
However I'd change all your other ifs to use dictionary.get.
With a bit of tuple unpacking you can simplify them all to:

house_set, place = board.get(piece, (None, None))
if house_set is not None:
    places[house_set][place] += 1


I'd then go on to change how you calculate and display the totals.
Using a dictionary comprehension and sum.
You want the sum of the 'house_set' and to know the 'place'.
And to get both you need to go through dict.items().

totals = {place: sum(house_set) for place, house_set in places.items()}


To then display this you can use a for loop over totals.items().
Where you display the place and the amount.

for place, amount in totals.items():
    print('{} = {}'.format(place, amount))


After this you display all the numbers, from brown 1 to blue 2.
I'd use the same as above but using places rather than totals.
However this returns a list that we'll have to loop through.
I'd loop through this and display them as we did above but you need to know it's 'brown 1' rather than 'brown'.
And so I'd use enumerate with the optional argument to simplify the logic to getting these numbers.
I'd then use:

for place, house_set in places.items():
    for i, amount in enumerate(house_set, 1):
        print('{} {} = {}'.format(place, i, amount))


Finally you print the amount of times you've been jailed before you print a nice looking board.
This is roughly the same as before, but as I use str.format rather than % I'd change it to keep consistent.

print("You've been jailed {} times".format(jail))


I'd drastically change how you display the board, I'm not saying this is the best way.
It's just simpler than typing space and blank a lot.
It will also show that the board is actually just a line.

Currently you use a while loop and divide by ten a lot to find the amount of digits the number has.
This is alright but not as clear as changing it to a string and finding it's length.
You also find the largest number to get this length from.
This is by manually writing places instead I'd use a list comprehension.
As we will go through (my) places again but we don't need the name you can use dict.values() to get the values.
And so would result in:

digit = len(str(max(amount for house_set in places.values() for amount in house_set)))


After this you make blank and space this size manually. Instead you can use " " * digit.
This will allow you to duplicate the string that many times. And so simplifies the code:

blank = "-" * digit
space = " " * digit


After this you make a formatted list. I'd instead format places.
However to be able to keep the original I'd use a dictionary comprehension and a list comprehension.
This will be like the for loops we used earlier to display the numbers.
I'd also use str.format the format you are using is simple in this mini-language.
You want the number to be proceeded by zeros, and so you'd use something like {:0>digit}.
However you want to pass the digit. To simplify the logic you can use {{:0>{}}}.format(digit).format(data).
Where data is the data we will be formatting, as you did before.
And so I'd use:

```
place_format = '{{:0>{}}}'.format(digit)

formatted = {
place: [place_format.format(amount) for amount in house_set]
for place, house_set in places.it

Code Snippets

brown = [0, 0]
places = {
    'Brown': [0, 0],
    'Cyan': [0, 0, 0],
    'Pink': [0, 0, 0],
    'Orange': [0, 0, 0],
    'Red': [0, 0, 0],
    'Yellow': [0, 0, 0],
    'Green': [0, 0, 0],
    'Blue': [0, 0, 0]
}
board = {
    1: ('Brown', 0),
    3: ('Brown', 1),
    6: ('Cyan', 0),
    8: ('Cyan', 1),
    9: ('Cyan', 2),
    11: ('Pink', 0),
    13: ('Pink', 1),
    14: ('Pink', 2),
    16: ('Orange', 0),
    18: ('Orange', 1),
    19: ('Orange', 2),
    21: ('Red', 0),
    23: ('Red', 1),
    24: ('Red', 2),
    26: ('Yellow', 0),
    27: ('Yellow', 1),
    29: ('Yellow', 2),
    31: ('Green', 0),
    32: ('Green', 1),
    34: ('Green', 2),
    37: ('Blue', 0),
    39: ('Blue', 1),
}
house_set, place = board.get(piece, (None, None))
if house_set is not None:
    places[house_set][place] += 1
totals = {place: sum(house_set) for place, house_set in places.items()}

Context

StackExchange Code Review Q#129763, answer score: 75

Revisions (0)

No revisions yet.