Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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";
}
share|improve this question

3 Answers 3

up vote 5 down vote accepted

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! :)

share|improve this answer
    
I am working on this might take me a while, but I have one question. I know that forsale is common but it is tedious to add it in every line since its default value will be false. So can the Position superclass set forsale to false for all its subclasses? –  Elton Feb 4 at 20:32
1  
You could, but you do have two instances (Airport and Cities) where the value defaults to true . . . would be nice to be able to pass that in. You could set it up to be an "optional" parameter . . . change this.forsale = forsale to this.forsale = forsale || false; . . . that way, it would default to false, but would still assign the value, if it was provided. It's even at the right position in the parameters already, so it could just be left off for false values . . . the lines would only need to updated if it was ever true and wasn't an Airport or Cities object. –  talemyn Feb 4 at 20:46
    
maybe !!forsale instead? The goal is to convert to boolean, correct? So why not say that instead of saying "convert to boolean and then do a no-op"? –  Tim Seguine Feb 5 at 11:30
    
@TimSeguine - actually, I was assuming that the value (if present) would be Boolean already. Elton didn't want to have to pass false for all of the cards that were not "buyable", when creating their objects. The logic was to make it so that if the parameter was left out (undefined) or set to null, the value of forsale in the object would default to false. –  talemyn Feb 5 at 17:24
    
Okay I recognized then correctly, my argument stands. The thing is: if forsale is a boolean then forsale || false is a no-op (Or'ing with false is the identity). If forsale is not a boolean, then forsale || false converts it to one, then performs a no-op. –  Tim Seguine Feb 5 at 17:43

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.

share|improve this answer

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.

share|improve this answer
3  
that's a good thought on the jail space.however, it might be worth considering that space to be simply just visiting, and maintain a flag to indicate whether the player is in jail or not. To me, in jail, is more a state of the player than it is a position on the board –  heiserman Feb 4 at 22:56
    
@heiserman couldn't have put that any better. The player is either inJail or justVisiting but in either case the player is @ Jailhouse. –  Elton Feb 5 at 3:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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