24
\$\begingroup\$

After studying several ways of doing OOP in JavaScript I think I finally came up with one that seems OK to me. Is it okay? Do you see some problems I can face by using OOP in JavaScript like this?

JSFiddle test

var Operators = (function ($, self) {
    var self = {};

    //private
    var IPT_X = '#x';
    var IPT_Y = '#y';

    //public
    self.x = 0;
    self.y = 0;

    //public
    self.showOperators = function() {
        //use of a private property (IPT_X) and a public property (this.x)
        $(IPT_X).val(self.x);
        $(IPT_Y).val(self.y);
    };

    self.clean = function() {
        self.x = 0;
        self.y = 0;
        // call to a local public method 
        self.showOperators();
    };

    self.updateOperators = function(_x, _y) {
        // use of a public property
        self.x = _x;
        self.y = _y;
    };

    self.clean();

    return self;

//Module dependencies
})(jQuery, {});

var Randomizer = (function(self, math) {

    // private
    function getRandomNumber() {
        return math.round(math.random() * 1000);
    };

    // keep superior method so we can call it after overriding 
    self.oldUpdateOperators = self.updateOperators

    // public
    self.updateOperators = function(_x, _y) {
        // call to superior class's method
        self.oldUpdateOperators(_x, _y);
        // call to method of superior object
        self.showOperators();
    };

    self.populateRandomNumbers = function() {
        // call to public local method (this.updateOperators())
        // and to a local private method (getRandomNumber()))
        self.updateOperators(getRandomNumber(), getRandomNumber());
    };

    // init
    self.populateRandomNumbers();

    return self;
})(Operators, Math)

var Operations = (function(self, $) {

    //private
    var IPT_RES = '#res';
    var BTN_SUM =  '#sum';
    var BTN_SUBTRACT =  '#subt';
    var BTN_MULTIPLY =  '#mult';
    var BTN_DIVISION =  '#div';
    var BTN_CLEAN =  '#clean';
    var BTN_RAND =  '#rand';

    // private
    function calcSum() {
        return self.x + self.y;
    };

    function calcSubtraction() {
        return self.x - self.y;
    };

    function calcMultiplication() {
        return self.x * self.y;
    };

    function calcDivision() {
        return self.x / self.y;
    };

    function showRes(val) {
        $(IPT_RES).val(val);
    };

    //public
    self.sum = function() {
        // call to 2 local private methods
        showRes(calcSum());
    };

    self.subtract = function() {
        showRes(calcSubtraction());
    };

    self.multiply = function() {
        showRes(calcMultiplication());
    };

    self.division = function() {
        showRes(calcDivision());
    }; 

    // init
    $(BTN_SUM).on('click', function() { self.sum() });
    $(BTN_SUBTRACT).on('click', function() { self.subtract() });
    $(BTN_MULTIPLY).on('click', function() { self.multiply() });
    $(BTN_DIVISION).on('click', function() { self.division() });   
    $(BTN_CLEAN).on('click', function() { self.clean() });
    $(BTN_RAND).on('click', function() { self.populateRandomNumbers() });

    return self;

})(Randomizer, jQuery);
<html>
<body>
X: <input id='x'>
<br>
Y: <input id='y'>
<br>
Res: <input id='res'>
<br>
<input id='sum' type='button' value='+'>
<input id='subt' type='button' value='-'>
<input id='mult' type='button' value='*'>
<input id='div' type='button' value='/'>
<input id='clean' type='button' value='C'>
<input id='rand' type='button' value='Rand'>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.8.3/jquery.min.js"></script>

</body>
</html>

Pros:

  • Clean code. Not much boilerplate code
  • No need to use .call or .bind which in my view make code harder to understand
  • Clearly defined inheritance
  • No need to instantiate anything (new). The simple presence of the Class definition is enough for everything to work.
  • Most OOP concepts available
  • Clear separation on view and business layers once I don't need to define anything in html. All actions are bind in JavaScript
  • Don't need to worry about prototypes
  • I'm already using this approach in a production system and it's working okay.

Cons:

  • I can't do a call to a higher method version. I mean, call to a "grandfather" class's method that has already been overridden. Only to the last method version. I rarely do this, so I think it's okay.
  • Too different from what's normally done in JavaScript. Although, I'm really not sure about this one.

The objective of this question isn't a code review of a simple calculator. I'm a senior developer and studying JS for some time now and I develop in several other languages and FWs. You can take a look on other (more verbose) OOP methods I studied before in my blog's post here.

I'm perfectly aware how an MVC works and what should be the function for each layer. I have done a simple calculator as an example to show how I intend to do JS OOP. I have mixed UI handling code and logic on purpose just to simplify the example. In a real system I would have separated it in different classes. That goes to the names also. In actual code I always use VERY verbose names.

I tried to simplify OO in JS and created this technique. As I said it's already been used in a production system.

This question's intention is present this OOP format to other developers to see if it has any problems I'm not seeing. So I'm not worried with this specific task as it's a mere example. In order to focus answers to this point, please stick to ways of doing this in OOP.

If someone thinks it's ok I also would like to hear from you.

Edit

I created a new question with many of the recommended modifications, here: Simple object-oriented calculator - follow-up

\$\endgroup\$
7
\$\begingroup\$

Private Variables

I see the comments that you provided to represent which variables are public or private:

//private
var IPT_X = '#x';
var IPT_Y = '#y';

//public
self.x = 0;
self.y = 0;

But those comments are not too useful. If you want a better way, which is more of a standard, is to represent private variables with an underscore before the variable name. Which would look like:

var _IPT_X = '#x';
var _IPT_Y = '#y';

self.x = 0;
self.y = 0;

self

It appears from looking at the code, you are following a Python like syntax. To stay consistent, I would always place self as the first argument.

var Operators = (function ($, self) {

Change to...

var Operators = (function (self, $) {

Recreating JQuery Objects

As @YiJiang said in the comments in the question, it would be better to store the JQuery object than constantly calling $ on the private variables you have. So, you could do something like:

var IPT_X = '#x';

Change to...

var _$IPT_X = $('#x');

Storing Past

In the following code:

// keep superior method so we can call it after overriding 
self.oldUpdateOperators = self.updateOperators

I feel as though a better way to represent overriding a function would be to maybe place some property on self.updateOperators called override.

function overridable(self, prop, descriptor) {
    Object.defineProperty(self, prop, descriptor);
    // If you cannot write over the property then there cannot be an override.
    if(!descriptor.writable) return self;

    // This is the function that will be used to override the property.
    Object.defineProperty(self[prop], "override", { value: function (descriptor) {
        // Creates a chain of overridden properties that store the old property values.
        var overridden = self[prop];
        overridable(self, prop, descriptor);
        self[prop].overridden = overridden;
    }});
    return self;
}

Here is how to use the function:

var obj = {};
overridable(obj, "add", { value: function(a,b) { return a + b }, writable: true });
obj.add(1,2); // => 3
obj.add.override({ value: function(a,b,c) { return a + b + c }, writable: true });
obj.add(1,2,3);            // => 6
obj.add.overridden(1,2,3); // => 3
obj.add.override({ value: function(a,b,c,d) { return a + b + c + d }, writable: true });
obj.add(1,2,3,4);                       // => 10
obj.add.overridden(1,2,3,4);            // => 6
obj.add.overridden.overridden(1,2,3,4); // => 3

NOTE: This is not bullet proof by no means. So, play with it to make it fit what you need.

For more on Object.defineProperty.


Something To Add

What I have found to be very useful when writing libraries in JavaScript is to place everything in one big function to control the scope. As in:

(function(scope) {
/* Library */
})(this)

This prevents from flooding the global scope and you can control what scope the library is connected to.

Here is an example:

(function(scope, dp) {

var _foo = "bar!";
function _f() { return _foo };

var lib = { bar: _f };

// Making to where it is a non-enumerable, non-configurable, and non-writable property to keep the version stuck there.
dp(lib, "VERSION", { value: "1.0.0" });

dp(scope, "lib", { value: lib });

})(this, Object.defineProperty)

This way I am able to use stuff like Object.defineProperty a lot without feeling bad about the added characters.


Others

I feel like you should look at some of the big name libraries and see how they do a lot of their code. Then, try to bring some of their techniques into your style, because they have been around for a while and there must be a reason for what they do.

Some "big names" I know of:

jquery.js (of course), underscore.js, and backbone.js. Then some I like: Three.js and Two.js.

There are plenty more out there, but those are the ones I can think of at the moment.


Your Style

My personal opinion is that since JavaScript is a scripting language, I feel that you can develop your own style in what suits what you are building. Your style is useful for what you have been coding, but I would never set on one specific way. It is fine to have a couple of things, like how you are mimicking Python syntax with the declarations requiring self as the first param, but you should always mold your style based on what you are coding.


Sorry it took a while to post this, I have been on and off all day. Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ It helped a lot! :D I found Objet.defineProperty specially interesting and will definitively study it deeper. It was what lacked in my techique. A way of reaching higher in the heierarchy chain. Thanks! Just for the record, I didn't intend to use python Syntax. I came from a Pascal/C++/Java/PHP background. I maintain a Python project though. I used self because I couldn't use this in the syntax. So I had to choose something and self seemed the right choice to me. But it's not a closed matter. I can use 'ths' or maybe 'this_'. what you think ? \$\endgroup\$ – Nelson Teixeira Dec 24 '15 at 23:55
  • \$\begingroup\$ What 'big names' libraries would you recommend ? I can think of prototype.js at the moment. Any other ? \$\endgroup\$ – Nelson Teixeira Dec 24 '15 at 23:56
  • \$\begingroup\$ One more question: the thing I most want to know is if there is some critical flaw in this way of doing oop. Based in your answer and considering your advised modifications I presume that you found it for the most part, ok. Am I right ? \$\endgroup\$ – Nelson Teixeira Dec 24 '15 at 23:58
  • \$\begingroup\$ @NelsonTeixeira. self is a great choice, I use it myself. (Or me when I am lazy lol). It just reminded me of Python. There is no "critical flaw", how I see it is that you should change your style to suit what you are trying to accomplish. Since you are making it look more OOP like, this looks fine to me. I would still add in the scoping I was talking about to protect the global scope. What you could do is make your own library that simulates OOP to learn more about the JavaScript. (Write methods like overridable) \$\endgroup\$ – tkellehe Dec 25 '15 at 0:29
  • 1
    \$\begingroup\$ @NelsonTeixeira. Sounds good! If you can, I would like to eventually see what you end up making:) \$\endgroup\$ – tkellehe Dec 25 '15 at 2:27
10
\$\begingroup\$

First I'd say the existence of jQuery is overkill for the task you have. Vanilla DOM handling can be used for this. Besides, you only appear to use jQuery for handling events and setting the value, nothing addEventListener and input.value can't handle.

Next would be to isolate the UI code from the logic, separation of concerns. Here, your operation module appears to be aware of the DOM. Your operators are also aware, but why? Random is also aware of the fact that you are to update operators, but isn't random just generating random numbers for you?

To visualize, it should be something like

    -- events ->                  -- data -->        
DOM              UI handling code              logic <-> utils
    <- results --                 <- results --

UI handling code will deal with DOM events and manipulation. Only here we should see DOM related code. Logic will only deal with the business logic, like app-specific stuff. Utils (it's a terrible name) is usually libraries and snippets of code that aid you in your logic but not necessarily bound to your app (like your random number generator).

Another thing to note are your names. We're in a very high-level language, and it's not like space matters (there's minifiers for that). Name your stuff verbosely. x and y can mean anything, how about value1 and value2. Your "constants" that hold selectors aren't named to say they contain selectors, but they look like they hold actual references to DOM elements (BTN_CLEAN, but it's not really a button).

Here's a super simplified version of it:

// This part can live in another file, and are only about operations.
// No part of this code is aware of the DOM.

var CalculatorOperations = {
  add: function(value1, value2) {
    return value1 + value2;
  },
  subtract: function(value1, value2) {
    return value1 - value2;
  },
  multiply: function(value1, value2) {
    return value1 * value2;
  },
  divide: function(value1, value2) {
    return value1 / value2;
  },
  generateRandomNumber: function() {
    return (Math.random() * 1000) | 0;
  },
};

// This part manages events and *uses* the operations.
// No part of this code actually does the logic.

(function() {

  var input1 = document.getElementById('input-1');
  var input2 = document.getElementById('input-2');
  var result = document.getElementById('result');
  var addButton = document.getElementById('add-button');
  var subtractButton = document.getElementById('subtract-button');
  var multiplyButton = document.getElementById('multiply-button');
  var divideButton = document.getElementById('divide-button');
  var clearButton = document.getElementById('clear-button');
  var randomButton = document.getElementById('random-button');

  function setResult(value) {
    result.value = value;
  }

  function setValue1(value) {
    input1.value = value;
  }

  function setValue2(value) {
    input2.value = value;
  }

  addButton.addEventListener('click', function() {
    setResult(CalculatorOperations.add(+input1.value, +input2.value));
  });

  subtractButton.addEventListener('click', function() {
    setResult(CalculatorOperations.subtract(+input1.value, +input2.value));
  });

  multiplyButton.addEventListener('click', function() {
    setResult(CalculatorOperations.multiply(+input1.value, +input2.value));
  });

  divideButton.addEventListener('click', function() {
    setResult(CalculatorOperations.divide(+input1.value, +input2.value));
  });

  clearButton.addEventListener('click', function() {
    setValue1('');
    setValue2('');
    setResult('');
  });

  randomButton.addEventListener('click', function() {
    setValue1(CalculatorOperations.generateRandomNumber());
    setValue2(CalculatorOperations.generateRandomNumber());
  });
}());
X:
<input id='input-1'>
<br>Y:
<input id='input-2'>
<br>Res:
<input id='result'>
<br>
<input id='add-button' type='button' value='+'>
<input id='subtract-button' type='button' value='-'>
<input id='multiply-button' type='button' value='*'>
<input id='divide-button' type='button' value='/'>
<input id='clear-button' type='button' value='C'>
<input id='random-button' type='button' value='Rand'>

OOP isn't "a must". It's just one of those paradigms available to you in JavaScript. What is "a must" though is keeping your code simple and maintainable. In your version, I was running around because some piece of code did this, and this in another part. In the code above, I know where DOM-related code happens, where the logic happens, who does what.

\$\endgroup\$
  • 2
    \$\begingroup\$ Your code is incorrect. It returns 5565 for 55 + 65. You should use +val1 + +val2 or parseInt(val1, 10) + parseInt(val2, 10) \$\endgroup\$ – Ismael Miguel Dec 24 '15 at 21:07
  • \$\begingroup\$ Agree with Joseph, lately, I've started to use +val1 + +val2 to make sure I convert strings typed numbers to numeric. I'm staying away from parseInt unless I want to truncate a decimal as well. Another thing I dislike about parseInt is that it ignores non-numerics after numbers in the string. \$\endgroup\$ – Will Dec 25 '15 at 5:10
  • 1
    \$\begingroup\$ Nice review. I have 2 additions: a) You could save a lot typing writing a shortcut $=function(el){return document.querySelectorAll(el)} b) You could've used a delegate event listener: giving the buttons ids like "add", "substract", you could switch to calculator operations of the according name \$\endgroup\$ – Thomas Junk Dec 25 '15 at 9:03
7
\$\begingroup\$

What you have is closer to a decorator pattern: You're decorating a base object with additional functionality. It's not quite inheritance. It's also not quite "classes", since it's really just an object instance.

Your structure results in one major gotcha: Your 3 "classes" are the exact same object. You just have 3 variables referencing it.

Also, since you're using IIFEs to define each of them, you can't instantiate/create something again. Once evaluated, the decorator functions are lost, leaving only the decorated object.

So it's not like classes - or maybe it's like 1 class with only static members, or a strict singleton. Either way, it's not typical OOP.

It' also not quite the usual decorator pattern. True decorator functions would be generic and applicable to multiple types of objects (see prototype.js for example; it basically decorates different native browser objects with combinations of methods), but here it's more like dividing the code into named "steps". While useful, it could also be done with code comments.

The coupling between the functions also trouble me. You could do some mild refactoring that simply removes the invocation on each, so you can reuse them, e.g.:

var Operators = function ($, self) { ... };
var Randomizer = function (self, math) { ... };
var Operations = function (self, $) { ... };

But you'd still have call each of them in turn to get a reasonably useful object, since each relies on something from the previous one. This is how subclassing works of course, but since this isn't quite subclassing, you have to manage it yourself. I.e. the only really useful invocation is Operations(Randomizer(Operators(x))). That coupling was only satisfied because you used IIFEs, and made sure to call things "the right way", and only once.

I'd change that by making the dependency chain explicit in the functions:

var Operators = function ($, self) {
  self = self || {};
  // ...
  return self;
};

var Randomizer = function (self, math) {
  self = Operators(self);
  // ...
  return self;
};

var Operations = function (self, $) {
  self = Randomizer(self);
  // ...
  return self;
};

Now it's more like typical "classes":

  • You can call any "link in the chain" and it'll make sure its "ancestors" are called.
  • You can get an "instance" from any of the steps; they're not once-only IIFEs.

And you can use them as regular decorators, if you have a compatible object you want to extend.

Still, there's enough coupling here (also to the UI elements on the page), that it's not the most straightforward construction. It still smells like dividing chunks of code into separate blocks in a complex way.

In all: Sure, it works, and it's quite readable. But it's also kinda strange.

Other notes

  • Operators should be named Operands. Operators are things like +, - etc.; operands are the values that operators operate on like xand y.

  • Be consistent with your arguments. Operators takes self as its 2nd argument, whereas the other two take self as the 1st argument. Easy to mess up.

  • Don't pass in stuff you don't need to. Math is a global object, so passing it as an argument called math doesn't really do anything useful. It's just lowercase now.
    Passing in jQuery as $ makes more sense, as it means you can use the $ alias even if you're using jQuery.noConflict() in the global scope. However, since you would presumably want that for all your functions, I'd do something like this:

    (function ($) {
        var Operators = function (self) { ... };
        var Randomizer = function (self) { ... };
        var Operations = function (self) { ... };
    }(jQuery));
    
  • You can't actually enter your own values in the form. Or: You can, but it doesn't alter the result. Changes in the view aren't reflected in the state of your object; it only goes the other way. And even then, it only does so if you call showOperators (ahem, "showOperands"). So as a calculator it's not terribly useful, I'm afraid.

\$\endgroup\$
  • \$\begingroup\$ Wow... you guys rock. I really loved this site. Only love and no harsh comments. What a difference from our normal day at Stackoverflow. :D That's one good piece criticism and advice. Tomorrow I'll experiment with the modifications you suggested and get back to you. Big thank you :) \$\endgroup\$ – Nelson Teixeira Dec 25 '15 at 4:44
  • \$\begingroup\$ edited the question with many modification I did based on the answers. :) \$\endgroup\$ – Nelson Teixeira Dec 26 '15 at 3:43
  • \$\begingroup\$ @NelsonTeixeira Please don't edit questions once they've been answered. It makes all our answers void and irrelevant. The Q&A format hinges on As matching the Q. If you've got new code, post a new question instead, and get new answers. I'll take the liberty of rolling back your edit (unless someone's beaten me to it while I was writing) \$\endgroup\$ – Flambino Dec 26 '15 at 3:47
  • \$\begingroup\$ ok... I'll make a new question then \$\endgroup\$ – Nelson Teixeira Dec 26 '15 at 3:50
  • 1
    \$\begingroup\$ @NelsonTeixeira Nope - in fact, it's encouraged :) So by all means link the two! You can also edit this question with a link to the new question. The point is just to not alter an existing question's main content so much that answers go out of sync, but linking to follow-up questions is perfectly fine. \$\endgroup\$ – Flambino Dec 26 '15 at 3:57

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.