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

Monopoly: Positions

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

Problem

I want to know if this can be trimmed down further. I am repeating myself with this.title and this.position. Also this.position is the corresponding array index, not sure if that is a good idea, if player throws 3 then perhaps I can just denote that as player is currently at positions[3]. I use this.price instead of this.rent because otherwise I would have 2 different object properties, purchaseprice and rentprice, I felt I could merge the 2 into 1 property and use an array.

Looking forward to suggestions.

Positions

```
positions = [
new Position("Go", 0),
new Cities("Cairo", "brown", [60, 2, 10, 30, 90, 160, 250], 1),
new CardPosition("Chest", "chest", 2),
new Cities("Vienna", "brown", [60, 4, 20, 60, 180, 320, 450], 3),
new Tax("Income Tax", 200, 4),
new Airport("Schiphol", [200, 25, 50, 100, 200], 5),
new Cities("Brussels", "blue", [100, 6, 30, 90, 270, 400, 550], 6),
new CardPosition("Chance", "chance", 7),
new Cities("Stockholm", "blue", [100, 6, 30, 90, 270, 400, 550], 8),
new Cities("Geneva", "blue", [120, 8, 40, 100, 300, 450, 600], 9),
new Position("Jailhouse", 10),
new Cities("Amsterdam", "pink", [140, 10, 50, 150, 450, 625, 750], 11),
new Position("Electric", 12),
new Cities("Bangkok", "pink", [140, 10, 50, 150, 450, 625, 750], 13),
new Cities("Istanbul", "pink", [160, 12, 60, 180, 500, 700, 900], 14),
new Airport("DBX", [200, 25, 50, 100, 200], 15),
new Cities("Hong Kong", "orange", [180, 14, 70, 200, 550, 750, 950], 16),
new CardPosition("Chest", "chest", 17),
new Cities("Madrid", "orange", [180, 14, 70, 200, 550, 750, 950], 18),
new Cities("Sydney", "orange", [200, 14, 70, 200, 550, 750, 950], 19),
new Position("Free Parking", 20),
new Cities("Toronto", "red", [220, 18, 90, 250, 700, 875, 1050], 21),
new CardPosition("Chance", "chance", 22),
new Cities("Mumbai", "red", [220, 18, 90, 250, 700, 875, 1050], 23),
new Cities("Rome", "red",

Solution

Some minor remarks:

  • Your Cities class should be called City



  • If you are willing to use falsey boolean checks, you do not need to initialize forsale to false



-
I would store the prices separately:

var rentPrices =  [
  "brown" : {
    low : [2, 10, 30, 90, 160, 250],
    high : [60, 4, 20, 60, 180, 320, 450]
  },
  "blue" : {
    low : [6, 30, 90, 270, 400, 550],
    high : [8, 40, 100, 300, 450, 600]
  }
//etc. etc.
]


This way you can indicate the rent prices by providing whether the card should charge low or high rent for that color ( Geneva is high, Stockholm and Brussels are low ). Otherwise you repeat the low price in your positions table for all but brown and navy.

Code Snippets

var rentPrices =  [
  "brown" : {
    low : [2, 10, 30, 90, 160, 250],
    high : [60, 4, 20, 60, 180, 320, 450]
  },
  "blue" : {
    low : [6, 30, 90, 270, 400, 550],
    high : [8, 40, 100, 300, 450, 600]
  }
//etc. etc.
]

Context

StackExchange Code Review Q#40862, answer score: 6

Revisions (0)

No revisions yet.