6
\$\begingroup\$

This question is an improvement I did based on recommendations from this other question: Simple object-oriented calculator

JSFiddle

var Base = function (self) {
	// base class initialization
    self = self || {};
    
    self.override = function(method, body) {
    	var overriden = self[method];
    	self[method] = body;
    	self[method].superior = overriden;
    }
    
    return self;
};
var ViewBase = function(self) {
	self = Base(self);

	self.parse = function() {
		keys = Array('parse').concat(Object.keys(Base()));
		for(prop in self) {
			if (_.contains(keys, prop)) continue;
			var name = _.camelCase(prop);
			self[name] = $('#' + self[prop]);
		}
	};
	
	self.parse();
	
	return self;
};
// View class mirroring html components
var ViewCalc = function() {
    self = {};
	
    self.IPT_X = 'x';
    self.IPT_Y = 'y';
    self.IPT_RES = 'res';
    self.BTN_SUM =  'sum';
    self.BTN_SUBTRACT =  'subt';
    self.BTN_MULTIPLY =  'mult';
    self.BTN_DIVISION =  'div';
    self.BTN_CLEAN =  'clean';
    self.BTN_RAND =  'rand';
    
    return ViewBase(self);
};
var Operands = function (self) {
    self = Base(self);
    
    // connect view to the base business class
    self.view = ViewCalc(self);

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

    //public
    self.showOperands = function() {
        //use of a private property (IPT_yyX) and a public property (this.x)
        self.view.iptX.val(self.x);
        self.view.iptY.val(self.y);
    };

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

    self.updateOperands = function(_x, _y) {
        // use of a public property
        self.x = _x;
        self.y = _y;
    };
    
    self.clean();
    
    return self;
   
};
var Randomizer = function(self) {
    self = Operands(self);

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

    self.override('updateOperands', function(_x, _y) {
        // call to superior class's method
        self.updateOperands.superior(_x, _y);
        // call to method of superior object
        self.showOperands();
    });

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

    // init
    self.populateRandomNumbers();
    
    return self;
    
};
var Operations = function(self) {
    self = Randomizer(self);
	
    // private
    function doSum() {
        return self.x + self.y;
    };

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

    function doMultiplication() {
        return self.x * self.y;
    };
 
    function doDivision() {
        return self.x / self.y;
    };
    
    function showRes(val) {
        self.view.iptRes.val(val);
    };
    
    //public
    self.sum = function() {
        // call to 2 local private methods
        showRes(doSum());
    };

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

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

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

    // init
    self.view.btnSum.on('click', function() { self.sum() });
    self.view.btnSubtract.on('click', function() { self.subtract() });
    self.view.btnMultiply.on('click', function() { self.multiply() });
    self.view.btnDivision.on('click', function() { self.division() });   
    self.view.btnClean.on('click', function() { self.clean() });
    self.view.btnRand.on('click', function() { self.populateRandomNumbers() });
    
    return self;
};

var o = Operations();
<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://cdnjs.cloudflare.com/ajax/libs/lodash.js/3.10.1/lodash.min.js"></script>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.8.3/jquery.min.js"></script>
<script type="text/javascript" src="base.js"></script>
<script type="text/javascript" src="viewBase.js"></script>
<script type="text/javascript" src="viewCalc.js"></script>
<script type="text/javascript" src="operands.js"></script>
<script type="text/javascript" src="randomizer.js"></script>
<script type="text/javascript" src="operations.js"></script>

What I did:

From @tkellehe's recommendations:

  • Separated the view part in a class of it's own. Now it has its own base class, and I can automatically create the jQuery object's properties. No need to declare them.
  • Create function 'override'. Now I can call higher hierarchy classes' methods.
  • Used one of the 'Big Names' library to create the jQuery object's properties: Lodash.

From @Flambino's recommendations:

  • Converted IIFEs to normal functions and did inheritance as recommended.
  • Changed Operators name to Operands. It's really a more correct name.
  • Removed Math dependency. It wasn't really needed.

From @tkellehe's and @Flambino's recommendations:

  • Self always comes first now.
  • I didn't put all inside a big function because in the actual code I'm developing in my computer each class is in a separate file. I just put everything together here in order to work on the site. And I also intend to use Require.js for code. So I guess this is kind difficult to do with it. Or maybe you can imagine some way of doing it?

From @Quill - HAT MANIAC's recommendations:

  • calcSomething functions where renamed doSomething functions. I wanted to express that the 'operation' function is the function directly connected to the control and the 'doOperation' function is the 'business rules' function.

What do you think of my modifications? Are they better?

I (hopefully) improved the code and created another question here:

Simple object-oriented calculator - part 3

\$\endgroup\$
1
  • \$\begingroup\$ Follow-up question here. (Please links like this in the future so that reviewers don't waste time on an old version of your code.) \$\endgroup\$ Commented Dec 29, 2015 at 21:14

2 Answers 2

5
\$\begingroup\$

Loading Within Context

From @tkellehe and @Flambino recommendations:

  • Self always comes first now. I didn't put all inside a big function because in the actual code I'm developing in my computer each class is in a separate file. I just put everything together here in order to work on the site. And I also intend to use Require.js for code. So I guess this is kind difficult to do with it. Or maybe you can imagine some way of doing it?

You can still do the scoping that was discussed before in my answer. I believe RequireJS does support it, but it has been a while since I have used it. Most of the projects I have been doing now, are small individual components where I use as little 3rd party libraries as possible.

But here is a quick solution that I have been using (of course this is extremely watered down...):

function load_into(filename, scope, callback) {
    var xhr = new XMLHttpRequest();
    xhr.open('GET', encodeURI(filename));
    xhr.onload = function() {
        // The zero is used if you are loading local files because
        // the return status for the request is always zero (for me at least).
        if (xhr.status === 200 || xhr.status === 0)
            callback.call(scope, xhr.responseText)
        else
            console.error("Problem occurred with loading: " + filename + "\n" +
                          "Status: " + xhr.status)
    };
    xhr.send();
}

And here is how to use it:

var lib = {};
load_into("lib.js", lib, function(code) { eval(code) });

load_into will read in "lib.js" and pass that into the callback. Then use the lib object as the scope for the callback which the eval will use as the scope to run/interpret the code.

NOTE: In order to get local files to load you must play with the browser and of course each browser has its own special quirks.

Chomium: This is the one I use. What you do is pretty simple. First duplicate the shortcut and close all Chrome browsers currently open. Right click on the shortcut then go to the properties for the shortcut. There will be an attribute labeled Target under the Shortcut tab paste this at the end:

--allow-file-access-from-files

Then use that shortcut to run your test code (Do not use for normal browsing! Because that can be a security risk!).

FireFox: Have not tried it but here is someone who has.

IE: Have not tried either but all I could find on it was this.

Here is some more on this.


Private Variables

I still have the problem with the comments that tell you whether or not a particular variable is private. Because, If I were to use your library I would not know what variables that were created in the global space that I did not have access to or might overwrite. Also, when reading the code (of course not as noticeable with these short functions) I would have a hard time remembering which variables were private and which ones were not.

This is actually some problems I run into when coding on large projects being in C++. Where, it is difficult to know the accessibility of variables especially when debugging or proof reading someone else's code. So, some of the things we do is place m_ before member variables and prefix _ for private (I have seen some people do suffixes). These little things provide more information than a comment where the variables are created and adds a good bit to the readability without adding comments.

Not quite sure why you have the prefixed _ for the arguments for the function?

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

I do not see what is wrong with just having the parameters being x and y instead of _x or _y.


Inefficient Code

The following code:

var ViewBase = function(self) {
  self = Base(self);

  self.parse = function() {
      keys = Array('parse').concat(Object.keys(Base()));
      for(prop in self) {
          if (_.contains(keys, prop)) continue;
          var name = _.camelCase(prop);
          self[name] = $('#' + self[prop]);
      }
  };

  self.parse();

  return self;
};

The first thing is that you have not declared keys anywhere (which is probably just a typo). Also, every time parse gets called you create a new array which always has the same contents which is bad because Object.keys is pretty expensive. So, you could place that into the scope of the function or attach it to ViewBase as a static like property which would look like:

var ViewBase = function(self) {
    self = Base(self);

    self.parse = function() {
        for(prop in self) {
            if (_.contains(ViewBase.keys, prop)) continue;
            var name = _.camelCase(prop);
            self[name] = $('#' + self[prop]);
        }
    };

    self.parse();

    return self;
};
// Static like property.
ViewBase.keys = Array('parse').concat(Object.keys(Base()));

Also, on my computer at least, using continue in a for loop is not as efficient as just having the if statement.

for(prop in self) if(!_.contains(ViewBase.keys, prop)) {
    var name   = _.camelCase(prop);
    self[name] = $('#' + self[prop]); 
}

Then again you could just make it like:

for(prop in self) 
    if(!_.contains(ViewBase.keys, prop))
        self[_.camelCase(prop)] = $('#' + self[prop]);

But it looks like you left the name variable to add readability.

From testing it on my computer the changes above to just the for loop made the code run ~1.3 times faster than what you had. Which is not much but if you are ever trying to speed up code, the little things like that always are what make the biggest difference.


Object.defineProperty

I personally like using Object.defineProperty to add properties to objects and when I have a function I prefer to use that syntax (with the descriptor containing everything). So, I would recommend some wrapper function around it that still has the look and feel of the OOP you are looking for. You could try something like this:

function def_prop(self, prop, value) {
     return Object.defineProperty(self, prop, 
            { value: value, writable: true, configurable: true, enumerable: true })
}

Use:

def_prop(self, "prop", value);

That function probably is not the best but to me that looks better than

self.prop = value;

But that is just personal taste.

Also, if you do not want some of the properties to be enumerable (where Object.keys and forin loops do not recognize the property) which you were trying to do in the ViewBase function. You can define properties with Object.defineProperty with the enumerable value set to false.

Another thing is that it makes it easier to make cool getters and setters.

function SomeClassThing(self) {
    self = self || {};
    // Member of self.
    var m_prop;
    Object.defineProperty(self, "prop", {
        get: function() { return function(v) {
            if(v === undefined) return m_prop
            m_prop = v;
            return self;
        }},
        set: function(v) { m_prop = v },
        enumerable: true
    }
    return self;
}

var obj = SomeClassThing();
obj.prop = 0;
console.log(obj.prop());          // =>  0
console.log(obj.prop(10).prop()); // => 10

For more on Object.defineProperty.


Other Libraries

I do like it that you are using RequireJS. I completely forgot about that one and it is very useful in making large projects.


Base Class

It might be better to make Base an actual "normal like" JavaScript class (Well at least try using the prototype chain a little bit more...).

function Base(self) {
    // Assumes that not using the new operator.
    if(!(this instanceof Base)) {
        self = self || new Base();
        if(!(self instanceof Base)) {
            for(var i = Base.BaseProperties.length; --i >= 0;) {
                var name = Base.BaseProperties[i];
                self[name] = Base.prototype[name];
            }
        }
        return self;
    }
}

Base.prototype.override = function(method, body) {
    var overriden = this[method];
    this[method] = body;
    this[method].superior = overriden;
}

Base.BaseProperties = Object.getOwnPropertyNames(Base.prototype);
Base.BaseProperties.splice(0); // Removes "constructor"

This will it to where when new objects are created they will spawn from Base and therein extend from your base library. Also, will add all of the properties needed to make any object passed in to "quack" like a Base.

Or if you want to force all objects to have the prototype of Base you can be really savage and just set it.

function Base(self) {
    // Assumes that not using the new operator.
    if(!(this instanceof Base)) {
        self = self || new Base();
        if(!(self instanceof Base)) {
            // The prototype of the other object is now gone.
            self.__proto__ = Base.prototype;
        }
        return self;
    }
}

Base.prototype.override = function(method, body) {
    var overriden = this[method];
    this[method] = body;
    this[method].superior = overriden;
}

Here is a good read on inheritance in JavaScript.


Style

@Flambino did a good job in making the code look the way you wanted it to which at first I liked but now it just looks weird in JavaScript after seeing it here. Especially since I am used to using the new operator that instantiates the objects of the functions. With what you have works and I do not see anything that destroys the code.

There is one concern I have and that is since most JavaScript developers do not follow this style, and this style is pretty unique, you may have troubles with passing the code down to newer developers. Therein, placing a heavy dependency on whoever first writes this code. So, when someone new comes along they may be thrown in for a loop or might try to rewrite it because of the uniqueness.


Hope this helps!

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Again... it helped a lot. Actually I don't have any special feeling about this style. I'm just trying to find the best and most correct way to do object orientation in JS. I have read some books about JS, including "The Principles Of Object-Oriented JavaScript" and "You don't know JS" series and still couldn't find a good way of applying all OO concepts. The techniques are too verbose, I have to put lots of boilerplate code in order for the prototype inheritance to work. \$\endgroup\$ Commented Dec 28, 2015 at 16:30
  • 1
    \$\begingroup\$ An example is this post in my new dev blog where I explain 3 oop techniques: bit.ly/1TmJN1Y. I really find them awful. In every class I have to put "var proto = arguments.callee.prototype; proto.constructor = Randomizer;" and finish class with "Randomizer.prototype = new Operators" or something. I really don't like this aproach. So I'm trying to find a way of having all oop concepts available and a cleaner code. And I'm very greatful for your help. \$\endgroup\$ Commented Dec 28, 2015 at 16:30
  • \$\begingroup\$ One thing that would help a lot was if you could show me a small example like mine that demonstrated all oop concepts and has a more common, natural or javascript like aproach. \$\endgroup\$ Commented Dec 28, 2015 at 16:30
  • 1
    \$\begingroup\$ Alrighty. I will try something like that. Would it be better to just make a new answer? I read through your article (very nice) and I can see how you are running into a conundrum. Also, you don't have to do the argumnents.callee if you know the name of the function. So, you can remove that and replace it with the function name. And besides callee is going out of style. \$\endgroup\$
    – tkellehe
    Commented Dec 28, 2015 at 22:52
  • 1
    \$\begingroup\$ I created a follow up. See how it looks. Link in the bottom of the question. \$\endgroup\$ Commented Dec 29, 2015 at 21:02
4
\$\begingroup\$

Self?

I may be missing something, but what's with all this self? In JavaScript, there is such thing as this which works exactly the same way you are treating this self variable.

It also has some perks. For example, in the ViewCalc "class", its type will be listed as type ViewCalc, where just returning this self object that you manually created will only be listed as Object.

It is also much better practice to be using this.


OOPerations

This Operations "class" thing that you have has some pretty similar logic within:

function doSum() {
    return self.x + self.y;
};

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

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

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

These 4 functions "take in" two integers and give out another integer. For this, we could create a class that holds this logic:

function Operation(do) {
    this.do = do;
}

where do is a function that takes two integers as arguments and returns a single integer. Then, when setting up these operations, you can use the new class:

var addition = new Operation(function(x, y) { return x + y });
var subtraction = new Operation(function(x, y) { return x - y});
...

With this, your code is more object-oriented and thus more flexible.


F(ake)OOP

This just looks wrong:

var o = Operations();

In JavaScript, object-oriented things should look more like this:

var o = new Operations();

where Operations is a class that uses this to set properties and methods and is not just a function that returns an object.

This code seems to be creating a false-OO feeling to it with how it is using this manually-created self object inside functions that don't use new to "instantiate".

\$\endgroup\$

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