12
\$\begingroup\$

I'm working on a complex Javascript application which includes its own data structure. The previous version's data structure was rather chaotic, and I'm looking to replace it with something where:

  • Every data item is an object which is (effectively) an instance of a class, which imposes a common structure including default values
  • Where possible, functions which set or get the data in a data item are taken out of the controller logic and attached to the data item class prototype

The problem I'm trying to solve by introducing this structure is, lots of messy and inconsistent 'model' style logic buried deep in 'controller' style functions, which has led to inconsistency, type errors, missing data errors, duplicated logic, etc.

Here's an example which uses the exact same approach I'm taking to defining and accessing instances (but the contents of data structure itself is simplified, since my question is about the way it's defined and accessed). I know that Javascript doesn't exactly have object 'classes' the way other languages do, and I've read about approximating them using prototypes, but I may be doing it wrong. In particular:

  • Do I need an init() function? Seems fine without one.
  • Is this a robust approach to the instance object accessing its own data? Can I rely on this within the attached function always being the instance's own data each time the function is called? Or should I be prepared to use call() or wrap calls in an anonymous function, or set it as a variable like that or self?
  • Is this a good way to give a class of objects default values? Note that this application already uses jQuery for other non-negotiable reasons, hence $.extend, and namespacing is already covered
  • I put the defaults object in the prototype so it's not redefined each time a new instance is created. Am I right in thinking this is a better practice than defining it within the function?
  • My actual application isn't about currencies, so please don't comment on the business logic. The code for setting the class is taken straight from my actual application draft, but I did swap the actual parameters and functions with simpler examples (if I hadn't, the explanation of the parameters and purpose of the functions would be longer than the rest of this question combined, and I'd need tonnes of non-relevant code to give a working example)

var dataItem = function(values){
  // Initiate new instance
  $.extend(this,this.defaults,values);  
};

dataItem.prototype.defaults = {
  // Structure & defaults for each data item
  value: 0.0,
  currency: 'USD'
};

dataItem.prototype.formatCurrency = function(){
  // Simple example of a 'getter' function
  var formatted = '';
  switch(this.currency) {
    case 'GBP':
      formatted = '£';
    break;
    case 'USD':
    case 'CAD':
    case 'AUD':
      formatted = '$';
    break;
  }
  return formatted+this.value.toFixed(2)+' '+this.currency;
};

// Some simple usage examples

var expenses = new dataItem({
  value: 199.5,
  currency: 'GBP'
});

var canadaIncome = new dataItem({
  currency: 'CAD' 
})

var taxes = new dataItem({
  value: 60 
})

document.write( expenses.formatCurrency() + ' : ' + canadaIncome.formatCurrency() + ' : ' + taxes.formatCurrency() );
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.4/jquery.min.js"></script>

\$\endgroup\$
  • \$\begingroup\$ This is my first question here, so please let me know if I'm doing it wrong. I've read the help intro and tried to give as much relevant info as possible, please let me know if I've missed anything. \$\endgroup\$ – user568458 May 1 '15 at 15:36
  • \$\begingroup\$ (Re. why jQuery 1.4.4.) Believe me, it's not by choice... this is one element of a complex site full of legacy code, at an organisation that only just allowed staff to use browsers other than IE8... Luckily, it's not key to this question. \$\endgroup\$ – user568458 May 1 '15 at 15:44
  • 1
    \$\begingroup\$ Welcome to Code Review. While it would have been best to use some actual code from your app, I think your code is "advanced" to be considered like real code. \$\endgroup\$ – Marc-Andre May 1 '15 at 15:45
  • \$\begingroup\$ Thanks. It is based on real code, copied and pasted it from my application - I just simplified the details of the data items and the getter function to give a self-contained example. \$\endgroup\$ – user568458 May 1 '15 at 16:02
11
+25
\$\begingroup\$

Naming Things

The class name dataItem doesn't communicate what it is, or how it is meant to be used:

There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. - Martin Fowler

What is a dataItem? From the code, it is a number value and a currency symbol. Perhaps CurrencyFormat might be more appropriate because what you are doing is formatting a number as currency.

Furthermore, naming conventions in JavaScript require that constructor functions should begin with a capital letter (DataItem vs dataItem). This naming convention is reflected in the native JavaScript API as well (String, Number, XMLHttpRequest, ...).

Principle of Single Responsibility

Being that you could format multiple numbers as the same currency, you might want to limit the instances that are available and use them to format any number, rather than keeping track of the number as an instance variable. This is the principle of Single Responsibility (do one thing, and do it well).

You can then provide an instance for each supported currency type, and a factory function that returns the proper formatter based on the currency name (e.g. "USD"):

function CurrencyFormat(symbol, name) {
    this.symbol = currencySymbol;
    this.name = currencyName;
}

CurrencyFormat.prototype = {
    name: null,
    symbol: null,

    constructor: CurrencyFormat,

    format: function(n, decimalPlaces) {
        decimalPlaces = typeof decimalPlaces == "number" ? decimalPlaces : 2;

        return this.symbol + n.toFixed(decimalPlaces) + " " + this.name;
    }
}

CurrencyFormat.AUD = new CurrencyFormat("$", "AUD");
CurrencyFormat.CAD = new CurrencyFormat("$", "CAD");
CurrencyFormat.GBP = new CurrencyFormat("£", "GBP");
CurrencyFormat.USD = new CurrencyFormat("$", "USD");

CurrencyFormat.getFormatter = function(currencyName) {
    if (CurrencyFormat[currencyName] && CurrencyFormat[currencyName] instanceof CurrencyFormat) {
        return CurrencyFormat[currencyName];
    }

    throw new Error("Currency format not implemented: " + currencyName);
};

Given the values you posted, this is how you would use the class:

var taxes = 60;
var expenses = 199.5;
var canadaIncome = 0;

console.log(CurrencyFormat.GBP.format(expenses));
console.log(CurrencyFormat.CAD.format(canadaIncome));
console.log(CurrencyFormat.USD.format(taxes));

If you know the currency name, you can use CurrencyFormat.getFormatter(...) to get the proper object:

// Get a formatter object by name:
var formatter = CurrencyFormat.getFormatter("USD");

// Throws an error because the currency name is not implemented:
var formatter = CurrencyFormat.getFormatter("XYZ");

This allows you to inject a currency formatter into other objects:

function ShoppingCartItem(price, currencyFormatter) {
    this.price = price;
    this.currencyFormatter = currencyFormatter;
}

ShoppingCartItem.prototype.formatPrice = function() {
    return this.currencyFormatter.format(this.price);
}

var item1 = new ShoppingCartItem(105.95, CurrencyFormat.getFormatter("USD"));
var item2 = new ShoppingCartItem(105.95, CurrencyFormat.GBP);

console.log(item1.formatPrice());
console.log(item2.formatPrice());

Removing unnecessary dependencies

There is no need to use jQuery.extend for just a couple of properties. This is an unnecessary dependency. For something as simple as a currency symbol and name, just pass those values in as constructor arguments. Passing in an object of key-value pairs is like hitting a thumb tack with a sledge hammer in this case.

Even if you needed to set a dozen properties by passing in an object of key-value pairs, I still wouldn't use jQuery.extend. It's not magic. It's just a for-in loop. You are better off writing half a dozen lines of code in order to remove a dependency on a large JavaScript library when this one class uses a fraction of what jQuery offers.

\$\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.