Monopoly: Positions

Question

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", [240, 20, 100, 300, 750, 925, 1100], 24),
    new Airport("BCN", [200, 25, 50, 100, 200], 25),
    new Cities("Rio", "yellow", [260, 22, 110, 330, 800, 975, 1150], 26),
    new Cities("Tokyo", "yellow", [260, 22, 110, 330, 800, 975, 1150], 27),
    new Position("Water Works", 28),
    new Cities("Paris", "yellow", [280, 24, 120, 360, 850, 1025, 1200], 29),
    new Position("Go to Jail", 30),
    new Cities("Berlin", "green", [300, 26, 130, 390, 900, 1100, 1275], 31),
    new Cities("Bejing", "green", [300, 26, 130, 390, 900, 1100, 1275], 32),
    new CardPosition("Chest", "chest", 33),
    new Cities("Moscow", "green", [320, 28, 150, 450, 1000, 1200, 1400], 34),
    new Airport("LAX", [200, 25, 50, 100, 200], 35),
    new CardPosition("Chance", "chance", 36),
    new Cities("New York", "navy", [350, 35, 175, 500, 1100, 1300, 1500], 37),
    new Tax("Super Tax", 100, 38),
    new Cities("London", "navy", [400, 50, 200, 600, 1400, 1700, 2000], 39)
];

function Position (title,position){
    this.title = title;
    this.position = position;
    this.forsale = false;
}
function CardPosition (title,type,position){
    this.title = title;
    this.type = type;
    this.position = position;
    this.forsale = false;
}
function Tax (title,tax,position){
    this.title = title;
    this.tax = tax;
    this.position = position;
    this.forsale = false;
    this.type = "tax";
}
function Airport(title,prices,position){
    this.title = title;
    this.rating = 0;
    this.price = prices[this.rating];
    this.position = position;
    this.forsale = true;
    this.type = "airport";
}
function Cities (title,set,prices,position){
    this.title = title;
    this.set = set;
    this.rating = 0;
    this.prices = prices;
    this.price = prices[this.rating];
    this.position = position;
    this.owner = "unowned";
    this.forsale = true;
    this.type = "city";
}

Answer 1

There are a few places that I think you could simplify the code, but I don't think you are too far off. Let me address each concern:

1. I am repeating myself with this.title and this.position.

   You can set all of these classes up in a class hierarchy, so that they can share common attributes. By using the .call() method, you can set up superclasses, and then reference them them as part of your subclass constructor. For example, ignoring any other changes, your first two classes can be reduced to a superclass and a subclass, like this:

   function Position(title, position) {
       this.title = title;
       this.position = position;
       this.forsale = false;
   }
   
   function CardPosition(title, type, position) {
       Position.call(this, title, position);
       this.type = type;
   }
   

2. 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].

   If the positions will always be in the order that they are stored in the array, then, yes, there is no reason to store the position as part of each object . . . the position in the array is the position of the "card", so storing the value in the object would be redundant. I'd remove all of the position attributes from the entire class structure.

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 at your sample data, I would keep these two prices separate. What you have is one purchase price and a series of scaling rent prices. It makes sense to use an array for the rents (because the are multiple values for the same piece of data), but the purchase price is different data and should really remain separate from the rent array.

   However, see my third "additional suggestion" below, for another way to group them . . .

4. I'd also have a few of other comments/suggestions, that might streamline your code:

   • make forsale part of Position and pass the value in as a parameter. All of the subclasses under it have a forsale attribute in common.

   • assuming that "airports" are like "railroads", in Monopoly, don't they also need a owner property?

   • you could consider another superclass called something like "DevelopmentProperties" as a parent of any subclass that has pricing, rent, rating, and owner values, in order to consolidate the data that is common to purchasable, update-able properties.

You could set DevelopmentProperties up like this:

function DevelopmentProperties(price, rents) {
    this.price = price;
    this.rents = rents;
    this.rating = 0;
    this.owner = "unowned";
}

. . . and extend it using this:

    DevelopmentProperties.call(this, price, rents);

So, finally . . .

Putting all of that together, here is how the code would end up:

// first, the common, top-level superclasses
function Position(title, forsale) {
    this.title = title;
    this.forsale = forsale;
}

function DevelopmentProperties(price, rents) {
    this.price = price;
    this.rents = rents;
    this.rating = 0;
    this.owner = "unowned";
}

// next, a mid-level subclass, that also acts as a superclass for the subclasses below
function CardPosition(title, type, forsale) {
    Position.call(this, title, forsale);
    this.type = type;
}

// finally, the low-level, subclasses that extend the superclasses above them
function Tax(title, tax) {
    CardPosition.call(this, title, "tax", false);
    this.tax = tax;
}

function Airport(title, price, rents) {
    CardPosition.call(this, title, "airport", true);
    DevelopmentProperties.call(this, purchase, rents);
}

function Cities(title, set, price, rents) {
    CardPosition.call(this, title, "city", true);
    DevelopmentProperties.call(this, purchase, rents);
    this.set = set;
}

I didn't update the positions array, but, obviously, that would also need to be updated a little bit to match the new structure.

Hope that helps! :)

Answer 2

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.

Answer 3

Your code is actually not too bad, in my opinion.

The title and position parameters are common to all of the positions, so put them consistently as the first two arguments. (It's common practice in many languages to put "optional" parameters last.)

My first instinctual criticism would be that the position parameter is redundant, since it can be inferred from the element's index in the array. However, I should point out that there is a "Jail" and a "Just Visiting" space, both at position 10. Therefore, when you eventually get around to implementing the Jail/Visitor split, you will eventually need to have the position explicitly defined.