Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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

Here Here is some more on this.

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

Here is some more on this.

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

Here is some more on this.

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

You can still do the scoping that was discussed before 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.

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.

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.

Source Link
tkellehe
  • 526
  • 2
  • 5

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!

default

AltStyle によって変換されたページ (->オリジナル) /