Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As the title says, I have built a basic and simplistic javascript application framework. It's being released under the GPLv3 license and I'd really appreciate some feedback on the code quality if anyone can find the time. Would love to hear the community response to this. It's nothing more than sugar really, and doesn't promise a 10th of what Spine.js / Backbone.js deliver, however it's light and barebones and easy to use. A smattering of classical inheritance in there too...

Docs are being worked on, as they are a bit messy. Feel free to ask any questions and submit pull requests:

https://github.com/skippychalmers/xa.js/blob/master/xa.js

share|improve this question

closed as off-topic by codesparkle Aug 31 '13 at 2:04

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must include the code you want reviewed. Code must not appear only in a link to an external source. Doing so makes us dependent on a third party and makes it harder to review your code. If your code is very large, please select only the portions in which you are especially interested in a review. You are encouraged to keep the link to the rest of your code." – codesparkle
If this question can be reworded to fit the rules in the help center, please edit the question.

add comment

3 Answers 3

The first thing I'd do is remove all the useless comments like

// return the Class object which is now ready to be further extended.
return Class;
...
// execute constructor method if present
if (this.construct) this.construct.apply(this, arguments);
...
// store config
config = scope;

and a million more.

The second thing I'd do is stop using tabs for indentation. Use 4-space indentation instead.

In your for-in loops, ALWAYS use a hasOwnProperty call or Douglas Crockford will revoke your JavaScript privileges. For example

for (var i in config)
    this[i] = config[i];

needs to become

for (var i in config) {
    if (config.hasOwnProperty(i))
        this[i] = config[i];
}

or the world will end in 2012, and you really don't want to be responsible for that.

Stuff like

if (complete === true || ...) ...

is redundant.

Here and there there is an inconsistent use of braces, normally you don't use them for one-line statements but occasionally there is stuff like

if (scope instanceof Function) {
    target = scope.prototype[name];
}

Sometimes you make useless checks like

if (jQuery !== undefined && jQuery instanceof Function) ...

undefined isn't an instance of Function, so you can skip the undefined check.

Oh, and instanceof should generally be avoided anyway, since objects created in a different window will return false and IIRC there is an IE memory leak with some COM+ objects, so I'd just replace it (and others like it) with if (typeof jQuery === 'function').

window.xa = window.$xa = window.XA = new xa();

I'd pick just one of these, three with nearly identical names just pollute the namespace.

So yeah. I haven't really looked into what this does a lot, just browsed the code and picked at obvious things, but it looks kinda promising. Good luck. :-)

If for some reason you need support for Safari 1.3 and IE 5, which don't support hasOwnProperty, this will work reasonably well.

if (typeof Object.prototype.hasOwnProperty !== 'function') {
    Object.prototype.hasOwnProperty = function (prop) {
        var type = {}.toString.call(this).slice(8, -1);
        return this[prop] !== undefined && this[prop] !== window[type].prototype[prop];
    };
}
share|improve this answer
    
Really appreciate this alpha123, a fantastic response. I'll be commuting fixes today. Feel free to also clone and send pull requests too yourself. You're a champ and will update here with changes made. –  SkippyChalmers Aug 11 '11 at 8:39
    
Hmm. Might need some sugar for the hasOwnProperty(i) method. I'd like to work in Safari 1.3... Not a big concern though. Any thoughts? The code snippet you mentioned is really just a way to copy in anything at all provided with the config object, so i'd want methods etc copied in, so a typeof === 'function' check would be a bad call. –  SkippyChalmers Aug 11 '11 at 9:16
    
I wouldn't be too worried about Safari 1.3 support, as it's from 2006, quirksmode doesn't test it, and its market share is well under 1%, but if you really are concerned, I edited the answer to include a reasonable workaround. –  alpha123 Aug 11 '11 at 21:15
    
thank you again.. your help is very much appreciated. If you go to github.com/skippychalmers/xa.js you'll see I've made some big changes based on what you've suggested. I have left the comments in, as for now they help me understand the code... they're not a problem once minified. Some of the more obvious and silly comments should still be taken out though, I agree. I removed all use of instanceof and found workarounds where necessary. In the one case where a for in loop is used, I think it's appropriate to avoid the check in the given context? But I like your workaround :) –  SkippyChalmers Aug 11 '11 at 21:51
1  
@SkippyChalmers Kill comments! Hack slash smash die! Rar! I think this is a great note for a framework developer especially, everywhere you see a comment, remove it and see if the functionality it explained is clear without the comment. If not, change it to be so. Usually it's just a matter of more explicitly named terms, more explicitly named function, or factoring a block into another function. The only place comments are justified is computational algorithms. –  Jimmy Hoffa Aug 13 '11 at 20:47
show 3 more comments

When you run tests like

Log.assertion('type', !!(
    xa.u.type(function(){}) === 'function'
    && xa.u.type({}) === 'object'
    && xa.u.type([]) === 'array'
    && xa.u.type('string') === 'string'
    && xa.u.type(null) === 'null'
    && xa.u.type(undefined) === 'undefined'
));

how do you tell what's failing?

If your tests are going to help rather than hinder code maintenance, they should tell you exactly what's going wrong.

With the above, if xa.u.type('string') returns 'spam', all the info about the values being compared, and the line number of the particular && clause that is false is lost before it reaches the code maintainer.

share|improve this answer
    
Thanks Mike, this kind of feedback is invaluable. I guess I was trying to introduce some testing, even if it was at a very high and vague level. I assumed that if a test failed, the maintainer would delve deeper. I absolutely agree however that the tests needs to be more robust and detailed. +1 for your answer. As always, feel free to pull request, also... give the framework a try and let me know what you think of it :) –  SkippyChalmers Aug 12 '11 at 8:43
    
Mike, I've made massive changes to the testing pages thanks to your advice. It still needs to have more detail, but we're on our way. Thank you. –  SkippyChalmers Aug 12 '11 at 16:44
add comment

In the case of a framework you want to be generic I understand, but a few things I personally found too generic, such that I had multiple ideas what they might be and no idea which is right:

  • config
  • context
  • scope

Is your config object derived from an xml file? Is it handed in by every single class? app? only uncommon apps? maybe give it a more explicit name, xaAppSpecificConfig or some such? (or is it class specific? I'm really not certain). Same type of things for the others, I know what scope is in normal programming, but again is it class or app specific? If it's class specific, does that mean if I changed the scope of a class to another app it would no longer be available in the previous app but would be in the new one? Or is scope merely an inheritance definition such that the inherit mechanisms would be different?

Little thing I saw in your utils.extends where the large for loop is, the entire for loop's activity is in a nested if, so reverse the bool check of the if and put a continue in it so the rest of the code can be one tab less and clearer to the fact that the if means "else do nothing" in your current code:

for ( ; i < length; i++ ) {
  // Only deal with non-null/undefined values
  if ( (options = arguments[ i ]) != null ) {

should be:

for ( ; i < length; i++ ) {
  // Don't deal with null/undefined values
  if ( (options = arguments[ i ]) === null ) {
    continue;
  }

Next thing is generally you want to order your class contents as:

  • member value/property instances
  • constructors
  • event handlers (in sequence if they occur as such)
  • methods
  • nested type implementations

And the reason for this, while it is a style thing, it really is more about readability, when you read a method and you see a member you want to know about, I think for a large part of the industry they expect to look up to the top of the class to see it's instance definition, and if they see a method they tend to look down the code for it's implementation if they want to go into it (side effect of how we read, the next thing that happens in a book is on the line below the previous, and as such we order our methods in execution order descending similarly).

You got the order mostly right, though the BaseApp is a member level instance and used as such, though defined long after the other member level instances (apps/libs/utils/etc).

Also at the very bottom of the file you have a bunch of member level stuff below nested type implementations.

share|improve this answer
    
this is invaluable to me. Thank you! +1. I'll work through your suggestions tomorrow, however (you've done enough already though) pull requests are always welcome! You're right, a bit more clarity on the innards of things. Basically the config object wasn't to really be used outside of a few "advanced" edge cases within the actual framework code itself. I should probably move it out of the utils container or maybe prefix the methods with '_' to hint at their intended visibility. –  SkippyChalmers Aug 14 '11 at 0:57
    
(cont.) Scope is not class specific, it's not a permanent change but allows you to access and call methods outside of current scope. Again, fairly edge case use... Once again, I'm absolutely going to take your suggestions to heart and expect to see some updates addressing your suggestions from me in the next few days. Thanks again! –  SkippyChalmers Aug 14 '11 at 0:58
1  
@SkippyChalmers if it's edge case use, try and make that clear, both by scope of accessibility and naming. Last thing you want is someone thinking they need to define it every time and going forth to do that if it's rarely valueable. Framework development is a lot about hints and nudges like this :) –  Jimmy Hoffa Aug 14 '11 at 1:14
    
You're absolutely right. I'll have a little re-think of this. Jimmy, you are a champ :) - Thanks. –  SkippyChalmers Aug 14 '11 at 11:32
    
A few changes made as per your suggestions. Thanks. I've left the jQuery code as it is though for now... I haven't decided on a good name for the scope method. Obviously you'd know that the 'this' keyword wouldn't be updated to reference anything else, so it should be clear that the scope is temporarily changed. I'm thinking of changing the name to 'accessScope', would that make sense to you do you think? Thanks again! –  SkippyChalmers Aug 15 '11 at 10:37
show 2 more comments

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