4
\$\begingroup\$

The following is a function that takes an area as an input and outputs some resulting calculations.

I thought an object was useful for creating multiple instances with different custom properties overwriting the defaults to make comparisons easy.

The getter and the setters are there so when one property is updated the update() is run to make sure all related properties are updated too.

I have a few of these relatively complicated models to turn from excel sheets into javascript functions so any advice on using many variable and interconnected properties or just the overall code would really be helpful.

function Cistern (inArea){
    // Private
    var _this = this,
        _type = 'Cistern';

    this.getType = function(){
        return _type;
    }

    // Public properties
    this.precipitationPerEvent = 0;
    this.totalStorageNeeded =  0;
    this.tankCost = 0;
    this.systemBaseCost = 0;

    // Public Variables & Defaults
    var properties = {
        area: inArea,
        maxRainEvent: 2,
        tankType: 'C',
        primaryUse: 'O',
        buildingStories: 3,
        fixtureNumber: 10,
        maintenanceLevel: 'M'
    }

    this.update = function(){
        var tankCostPerGallon = 0,
            pumpSize = 0,
            pumpCost = 0;
        // Storage Volume
        _this.precipitationPerEvent = Math.ceil(_this.area*(_this.maxRainEvent/12) * 7.48);
        _this.totalStorageNeeded = Math.ceil((_this.precipitationPerEvent / 100)) * 100;
        // Capital Costs
        if (_this.tankType === 'C'){
            tankCostPerGallon = 1.66;
        } else if (_this.tankType === 'P'){
            tankCostPerGallon = 1.43;
        } else if (_this.tankType === 'S'){
            tankCostPerGallon = 2.51;
        } else if (_this.tankType === 'F'){
            tankCostPerGallon = 1.33;
        }
        _this.tankCost = _this.totalStorageNeeded * tankCostPerGallon;
        if (_this.primaryUse === 'I'){
            pumpSize = Math.ceil(((62.4 * (_this.buildingStories * 10) * (_this.fixtureNumber * this.buildingStories * 0.00891))/550)/0.5);
        } else {
            pumpSize = 0.5;
        }
        pumpCost = -100.71 * Math.pow(pumpSize, 2) + 1327.7 * pumpSize - 39.38;
        _this.systemBaseCost = Math.ceil(_this.tankCost + (_this.tankCost * 0.6) + pumpCost);
        if (_this.primaryUse === 'I'){ _this.systemBaseCost += 2250};
    }

    // Build Getters & Setters
    Object.keys(properties).forEach(function (prop) {
        Object.defineProperty(this, prop, {
            // Create a new getter for the property
            get: function () {
                return properties[prop];
            },
            // Create a new setter for the property
            set: function (val) {
                if (properties[prop] != val){
                    properties[prop] = val;
                    this.update();
                }
            }
        })
    }, this);

    this.update();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Nice first question! Welcome to Code Review \$\endgroup\$ – Flambino Aug 31 '15 at 22:01
4
\$\begingroup\$

Firstly: There are a lot of magic numbers here. I'd stick those in variables and name them. For instance, I have no idea what 0.00891 is, but it's apparently to do with pump sizes? I guess? It'd also be nice to have them defined in one place. You have things like a general 1.6x cost multiplier, and a 2250 (presumably dollars) extra cost for a certain cistern type. Rather than hunt around in the code to find those if you need to update them (especially if used more than once!), it be nice to have them defined in one place - and labelled. E.g.

var COST_FACTOR = 1.6,
    TYPE_I_EXTRA_COST = 2250,
    // etc.

The names above are not very good, but it's just to illustrate the point.

And like Matt Zeunert said, the single-letter codes are very opaque as well. I'm sure they're some sort of industry code, but still.

For instance, the "primary use" code has quite a dramatic effect on things if it's exactly "I". Don't know why, but since it's a specific case your Cistern objects could perhaps benefit from an is... function (e.g. isForIrigation, or isForIndoorSkatingRink or whatever it is I stands for) that just returns true/false depending on whether primaryUse === "I". That'll make some later calculations much more expressive, because you can say "if the cistern isForIrigation, then...", rather than somewhat cryptically checking if a property happens to be an uppercase i.

Next, your Object.defineProperty usage is overwrought, it seems to me. Rather than recalculate everything whenever something is changed, it'd be easier to just calculate it when requested.

Sure, it mimics Excel quite well; change one cell, and the change ripples out (no water pun intended). But in whatever you're building, you'll be in charge of making those ripples happen. You'll have to ask for the updated values yourself. Sure, they get updated, but you won't see that until you access them. In which case it'd be simpler to just calculate them then.

On another note, the way you use this and _this betrays some confusion. The _this variable isn't necessary; you can use the plain this inside your update function as it refers to the same Cistern instance. In fact you do so by accident in one place already (a line that uses both _this.buildingStories and this.buildingStories in the same expression).

You can also structure your code more as a proper prototype by making update a true prototype method, rather than a property added by the constructor:

function Cistern() {
  // ...
}

Cistern.prototype = {
  update: function () {
    // ...
  }
};

However, as mentioned, I'd go a different route all together. Namely, I'd split update up, and flip the whole thing around: What's now being accessed with getters and setters becomes just plain properties, and what's now plain properties is replaced with getters - and only getters, since they're derived values and thus read-only. That way, your getters are responsible for calculating their value when you call them.

function Cistern (area) {
  this.area = area;
  this.maxRainEvent = 2;
  this.tankType = 'C';
  this.primaryUse = 'O';
  this.buildingStories = 3;
  this.fixtureNumber = 10;
  this.maintenanceLevel = 'M';

  var getters = {
    precipitationPerEvent: function () {
      return Math.ceil(7.48 * this.area * this.maxRainEvent / 12);
    },

    totalStorageNeeded: function () {
      return Math.ceil(this.precipitationPerEvent / 100) * 100;
    },

    tankCostPerGallon: function () {
      return { // this "table" should probably be extracted and put elsewhere
        'C': 1.66,
        'P': 1.43,
        'S': 2.51,
        'F': 1.33
      }[this.tankType];
    },

    tankCost: function () {
      return this.tankCostPerGallon * this.totalStorageNeeded;
    },

    pumpSize: function () {
      if(this.primaryUse !== 'I') {
        return 0.5;
      }

      // I simplified the math here, but the result should be the same
      return Math.ceil(5.5598 * Math.pow(this.buildingStories, 2) * this.fixtureNumber / 225);
    },

    pumpCost: function () {
      return -100.71 * Math.pow(this.pumpSize, 2) + 1327.7 * this.pumpSize - 39.38;
    },

    systemBaseCost: function () {
      var cost = Math.ceil(1.6 * this.tankCost + this.pumpCost);
      return this.primaryUse === 'I' ? cost + 2250 : cost;
    }
  };

  // create getters
  for(var property in getters) {
    Object.defineProperty(this, property, { get: getters[property] });
  }
}

Now you can update the public properties freely, and whenever you need a calculated value like, say, systemBaseCost, it'll be calculated on the fly, doing whatever intermediate calculations it needs along the way.

\$\endgroup\$
  • \$\begingroup\$ Wow, what a great analysis. Thank you for the refactor and awesome explanation! A lot of these magic numbers are even magic to me, as I pulled them from Excel or formulas. \$\endgroup\$ – PFlans Sep 1 '15 at 13:50
2
\$\begingroup\$

You can replace this series of if statements

if (_this.tankType === 'C'){
    tankCostPerGallon = 1.66;
} else if (_this.tankType === 'P'){
    tankCostPerGallon = 1.43;
} else if (_this.tankType === 'S'){
    tankCostPerGallon = 2.51;
} else if (_this.tankType === 'F'){
    tankCostPerGallon = 1.33;
}

With an object lookup:

var perGallonCostByTank = {
    "C": 1.66,
    "P": 1.43,
    "S": 2.51,
    "F": 1.33
};

tankCostPerGallon = perGallonCostByTank[_this.tankType];

And then place the perGallonCostByTank function outside your Cistern class.


Some properties like primaryUse, tankType and maintenanceLevel currently have single-letter values like "M" and "O". Unless these are technical values that everyone familiar with the business domain understands it makes sense to use more descriptive names.

\$\endgroup\$

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.