Modeling rainwater collection and cost

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 * (

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.

StackExchange Code Review Q#102438, answer score: 4