SproutCore--over 20000 lines of new code!

[David Mark]

After 20000 lines of new code, over 5000 new unit tests, and countless
hours of effort by over 30 contributors, SproutCore 1.0 is ready. ...

Is it?

First suspicious file is browser.js (wonder what that could be?)

//
==========================================================================
// Project: SproutCore - JavaScript Application Framework
// Copyright: ©2006-2009 Sprout Systems, Inc. and contributors.
// Portions ©2008-2009 Apple Inc. All rights reserved.
// License: Licened under MIT license (see license.js)
//
==========================================================================


Hmm. Portions copyright Apple, Inc. Big company == Great JS.


/** Detects the current browser type. Borrowed from jQuery + prototype
*/


Oh no.


SC.browser = (function() {

var userAgent = navigator.userAgent.toLowerCase();
var version = (userAgent.match( /.+(?:rv|it|ra|ie)[\/: ]([\d.]+)/ )
|| [])[1] ;

var browser = /** @scope SC.browser */ {


Please, make it stop. Scope?


/** The current browser version */
version: version,

/** non-zero if webkit-based browser */
safari: (/webkit/).test( userAgent ) ? version : 0,


Non-zero? This must be their own unique interpretation of jQuery's
sniffing. I wonder if they realize that jQuery moved forward one step
to object inferences.


/** non-zero if this is an opera-based browser */
opera: (/opera/).test( userAgent ) ? version : 0,


The one browser in the world that is easy to detect and they blew it.
Of course, there should be no reason to detect Opera in 2010 (or 2009
or 2008...) Can only go downhill from here.


/** non-zero if this is IE */
msie: (/msie/).test( userAgent ) && !(/opera/).test( userAgent ) ?
version : 0,


Sad, especially considering IE has conditional comments. Oh, but that
won't fit into their plans of creating a way-cool browser scripting
framework.


/** non-zero if this is a miozilla based browser */


Miozilla? Not very polished is it?


mozilla: (/mozilla/).test( userAgent ) && !(/(compatible|
webkit)/).test( userAgent ) ? version : 0,

/** non-zero if this is mobile safari */
mobileSafari: (/apple.*mobile.*safari/).test(userAgent) ?
version : 0,

/** non-zero if we are on windows */
windows: !!(/(windows)/).test(userAgent),

/** non-zero if we are on a mac */
mac: !!((/(macintosh)/).test(userAgent) || (/(mac os x)/).test
(userAgent)),

language: (navigator.language || navigator.browserLanguage).split
('-', 1)[0]
};

// Add more SC-like descriptions...


Huh? Comments that don't make sense are worse than no comments at
all.


SC.extend(browser, /** @scope SC.browser */ {

isOpera: !!browser.opera,
isIe: !!browser.msie,
isIE: !!browser.msie,
isSafari: !!browser.safari,
isMobileSafari: !!browser.mobileSafari,
isMozilla: !!browser.mozilla,
isWindows: !!browser.windows,
isMac: !!browser.mac,

/**
The current browser name. This is useful for switch statements.
*/


How so?


current: browser.msie ? 'msie' : browser.mozilla ? 'mozilla' :
browser.safari ? 'safari' : browser.opera ? 'opera' : 'unknown'

}) ;

return browser ;

})();


I don't even want to look in the next file. How about...
core_query.js. Sounds promising.


//
==========================================================================
// Project: SproutCore - JavaScript Application Framework
// Copyright: ©2006-2009 Sprout Systems, Inc. and contributors.
// Portions ©2008-2009 Apple Inc. All rights reserved.
// License: Licened under MIT license (see license.js)
//
==========================================================================
/*globals CQ add*/

require('system/builder') ;

/**
CoreQuery is a simplified DOM manipulation library used internally
by SproutCore to find and edit DOM elements. Outside of SproutCore,
you should generally use a more full-featured DOM library such as
Prototype or jQuery.


In other words, abandon all hope... How is it that all of these
various JS projects turn out with the same sort nonsense inside them.
The logic (and lack thereof), the comments, the documentation, the
forum posts, etc. are always so far distanced from reality; and yet,
the justifications for them always reference some Real World where it
makes sense to be ignorant, incompetent, apathetic and lazy. Oh, and
grandiose.

Why can't these people find another language to screw up?


"CoreQuery itself is a subset of jQuery with some additional plugins.
If you have jQuery already loaded when SproutCore loads, in fact, it
will replace CoreQuery with the full jQuery library and install any
CoreQuery plugins, including support for the SC.Enumerable mixin."


Well, this is going to be a very short review...


"Much of this code is adapted from jQuery 1.2.6, which is available
under an MIT license just like SproutCore."


....and history lesson. :


var styleFloat = SC.browser.msie ? "styleFloat" : "cssFloat";

I remember that one. It's pretty much Game Over as everything else in
the ZIP sits atop this.

// A helper method for determining if an element's values are broken


Again, worthless comments are worse than worthless.


var styleIsBorked = function styleIsBorked( elem ) {


Oh, a named function expression. Somebody let the developers out of
the sound-proof booth. They need to make contact with the rest of the
world.


if ( !SC.browser.safari ) return false;


So, only "values" in Safari (or whatever this thing thinks is Safari)
can be "borked". I really dislike cutesy geek-speak like that. It's
another layer of cheese (on top of a moldy jQuery circa 2007).


// defaultView is cached


Again with the silly comments.


var ret = defaultView.getComputedStyle( elem, null );
return !ret || ret.getPropertyValue("color") === "";
} ;


// implement core methods here from jQuery that we want available all
the
// time. Use this area to implement jQuery-compatible methods ONLY.
// New methods should be added at the bottom of the file, where they
will
// be installed as plugins on CoreQuery or jQuery.
CQ = CoreQuery = SC.Builder.create( /** @scope SC.CoreQuery.fn */ {


Whatever. Skipping to bottom...


// Add some global helper methods.
SC.mixin(SC.$, {

/** @private helper method to determine if an element is visible.
Exposed
for use in testing. */
isVisible: function(elem) {
var CQ = SC.$;
return ("hidden"!=elem.type) && (CQ.css(elem,"display")!="none")
&& (CQ.css(elem,"visibility")!="hidden");
}

}) ;


I've seen that pattern before (and it never fails to make me cringe).
What on earth do hidden inputs have to do with elements that have been
styled hidden (or removed from the layout?) The developers can't see
them? Thanks for bottling that guys.


for(var key in enumerable) {

if (!enumerable.hasOwnProperty(key)) continue ;


Oops, just lost Safari 2. It's not that it's necessary to support all
older browsers, but you can't just cut them off in the middle of a
script with an exception. I assume the developers didn't realize what
they were doing (usually a fair assumption).


for ( name in options ) {
CQ.attr(
(type)?this.style:this,
name, CQ.prop( this, options[ name ], type, i, name ));
}


No filter on that one? The attr method is the usual suspect from
jQuery. Hard to believe anyone would copy it at this point. Of
course, it's hard to believe anyone would still be using it at this
late date. Whatever.


// Check to see if the W3C box model is being used
boxModel: !SC.browser.msie || document.compatMode ===
"CSS1Compat",


That was in jQuery until they "punted" box models (along with quirks
mode in general).

isCoreQuery: YES, // walk like a duck


That's about enough I think. Over 30 people wasted a lot of time
building widgets on top of this rubbish heap. The 5000 unit tests are
obviously worthless. I could have told them all they needed to know
without writing a single one. Executive summary: forget it.

Obviously nobody is ever going to go back in here and rewrite this
jQuery regurgitation (and they sure as hell can't "upgrade" it
either). So they might as well turn out the lights and padlock the
place. The world didn't need a parallel jQuery universe (and sure as
hell not another jQuery UI).

And how do I know the author(s) will find this review "superficial".
In reality, I gave it far more time than it deserved. Guys, find
another hobby. I mean that in the kindest possible sense.

 

[David Mark]

Hi I am the creator of sproutcore. If you had taken the time to look at the rest of sproutcore you might have noticed that corequery is not used very often because our view layer has a more powerful system called the rendercontext.

Very often? Why is an old version of jQuery (!) in there at all? And
the comments indicate it will use an extant jQuery if found. That's
suicide as newer versions are incompatible. So good luck with that!

Also I think nitpicking little bits of code like this is really pointless.. Anyone who has written a big project knows that some parts receive more attention then others based on their actual real world needs.

Oh, the "real world" argument? Here's a thought: the lowest level DOM
code is everything for a project like this. Don't build castles in a
swamp.

Picking one remote part like this and disecting it misses all to useful stuff you might have found if you has just looked.

Remote part? It's deep in the core.

For example spend some time looking at the datastore or observer layers

Is it worth it? What do those layers sit upon?

And Happy New Year!

 

[David Mark]

Hi I am the creator of sproutcore.  If you had taken the time to look at the rest of sproutcore you might have noticed that corequery is not used very often because our view layer has a more powerful system called the rendercontext.  

//
==========================================================================
// Project: SproutCore - JavaScript Application Framework
// Copyright: ©2006-2009 Sprout Systems, Inc. and contributors.
// Portions ©2008-2009 Apple Inc. All rights reserved.
// License: Licened under MIT license (see license.js)
//
==========================================================================

sc_require('system/builder');

/** set update mode on context to replace content (preferred) */
SC.MODE_REPLACE = 'replace';

/** set update mode on context to append content */
SC.MODE_APPEND = 'append';

/** set update mode on context to prepend content */
SC.MODE_PREPEND = 'prepend';

/**
@namespace

A RenderContext is a builder that can be used to generate HTML for
views or
to update an existing element. Rather than making changes to an
element
directly, you use a RenderContext to queue up changes to the
element,
finally applying those changes or rendering the new element when you
are
finished.

You will not usually create a render context yourself but you will
be passed
a render context as the first parameter of your render() method on
custom
views.

Render contexts are essentially arrays of strings. You can add a
string to
the context by calling push(). You can retrieve the entire array as
a
single string using join(). This is basically the way the context
is used
for views. You are passed a render context and expected to add
strings of
HTML to the context like a normal array. Later, the context will be
joined
into a single string and converted into real HTML for display on
screen.

In addition to the core push and join methods, the render context
also
supports some extra methods that make it easy to build tags.

context.begin() <-- begins a new tag context
context.end() <-- ends the tag context...
*/
SC.RenderContext = SC.Builder.create(/** SC.RenderContext.fn */ {

SELF_CLOSING: SC.CoreSet.create().addEach('area base basefront br hr
input img link meta'.w()),


basefont (and it is long deprecated)



/**
When you create a context you should pass either a tag name or an
element
that should be used as the basis for building the context. If you
pass
an element, then the element will be inspected for class names,
styles
and other attributes. You can also call update() or replace() to
modify the element with you context contents.

If you do not pass any parameters, then we assume the tag name is
'div'.

A second parameter, parentContext, is used internally for
chaining. You
should never pass a second argument.

@param {String|DOMElement} tagNameOrElement
@returns {SC.RenderContext} receiver
*/
init: function(tagNameOrElement, prevContext) {
if (tagNameOrElement === undefined) tagNameOrElement = 'div';

// if a prevContext was passed, setup with that first...
if (prevContext) {
this.prevObject = prevContext ;
this.strings = prevContext.strings ;
this.offset = prevContext.length + prevContext.offset ;
}

if (!this.strings) this.strings = [] ;

// if tagName is string, just setup for rendering new tagName
this.needsContent = YES ;
if (SC.typeOf(tagNameOrElement) === SC.T_STRING) {
this._tagName = tagNameOrElement.toLowerCase();
this._needsTag = YES ; // used to determine if end() needs to
wrap tag

// increase length of all contexts to leave space for opening
tag
var c = this;
while(c) { c.length++; c = c.prevObject; }

this.strings.push(null);
this._selfClosing = this.SELF_CLOSING.contains(this._tagName);
} else {
this._elem = tagNameOrElement ;
this._needsTag = NO ;
this.length = 0 ;
this.needsContent = NO ;
}

return this ;
},

// ..........................................................
// PROPERTIES
//

// NOTE: We store this as an actual array of strings so that
browsers that
// support dense arrays will use them.
/**
The current working array of strings.

@property {Array}
*/
strings: null,

/**
this initial offset into the strings array where this context
instance
has its opening tag.

@property {Number}
*/
offset: 0,

/**
the current number of strings owned by the context, including the
opening
tag.

@property {Number}
*/
length: 0,

/**
Specify the method that should be used to update content on the
element.
In almost all cases you want to replace the content. Very
carefully
managed code (such as in CollectionView) can append or prepend
content
instead.

You probably do not want to change this propery unless you know
what you
are doing.

@property {String}
*/
updateMode: SC.MODE_REPLACE,

/**
YES if the context needs its content filled in, not just its
outer
attributes edited. This will be set to YES anytime you push
strings into
the context or if you don't create it with an element to start
with.
*/
needsContent: NO,

// ..........................................................
// CORE STRING API
//

/**
Returns the string at the designated index. If you do not pass
anything
returns the string array. This index is an offset from the start
of the
strings owned by this context.

@param {Number} idx the index
@returns {String|Array}
*/
get: function(idx) {
var strings = this.strings || [];
return (idx === undefined) ? strings.slice(this.offset,
this.length) : strings[idx+this.offset];
},

/**
Adds a string to the render context for later joining. Note that
you can
pass multiple arguments to this method and each item will be
pushed.

@param {String} line the liene to add to the string.


Liene?


@returns {SC.RenderContext} receiver
*/
push: function(line) {
var strings = this.strings, len = arguments.length;
if (!strings) this.strings = strings = []; // create array lazily

if (len > 1) {
strings.push.apply(strings, arguments) ;
} else strings.push(line);

// adjust string length for context and all parents...
var c = this;
while(c) { c.length += len; c = c.prevObject; }

this.needsContent = YES;

return this;
},

/**
Pushes the passed string onto the array, but first escapes the
string
to ensure that no user-entered HTML is processed as HTML.

@param {String} line one or mroe lines of text to add
@returns {SC.RenderContext} receiver
*/
text: function(line) {
var len = arguments.length, idx=0;
for(idx=0;idx<len;idx++) {
this.push(SC.RenderContext.escapeHTML(arguments[idx]));
}
return this ;
},

/**
Joins the strings together, returning the result. But first, this
will
end any open tags.

@param {String} joinChar optional string to use in joins. def
empty string
@returns {String} joined string
*/
join: function(joinChar) {

// generate tag if needed...
if (this._needsTag) this.end();

var strings = this.strings;
return strings ? strings.join(joinChar || '') : '' ;
},

// ..........................................................
// GENERATING
//

/**
Begins a new render context based on the passed tagName or
element.
Generate said context using end().

@returns {SC.RenderContext} new context
*/
begin: function(tagNameOrElement) {
// console.log('%@.begin(%@) called'.fmt(this, tagNameOrElement));


Delete debugging code.


return SC.RenderContext(tagNameOrElement, this);
},

/**
If the current context targets an element, this method returns
the
element. If the context does not target an element, this method
will
render the context into an offscreen element and return it.

@returns {DOMElement} the element
*/
element: function() {
if (this._elem) return this._elem;
// create factory div if needed
var ret, child;
if (!SC.RenderContext.factory) {
SC.RenderContext.factory = document.createElement('div');
}
SC.RenderContext.factory.innerHTML = this.join();

// In IE something weird happens when reusing the same element.


That doesn't inspire confidence.


// After setting innerHTML, the innerHTML of the element in the
previous view
// turns blank. Seems that there is something weird with their
garbage
// collection algorithm.


No. There is something weird going on with your script(s).


// I tried just removing the nodes after keeping a
// reference to the first child, but it didnt work.
// Ended up cloning the first child.


http://www.jibbering.com/faq/faq_notes/clj_posts.html#ps1DontWork


if(SC.RenderContext.factory.innerHTML.length>0){
child = SC.RenderContext.factory.firstChild.cloneNode(true);
SC.RenderContext.factory.innerHTML = '';
} else {
child = null;
}
return child ;
},

/**
Removes an element with the passed id in the currently managed
element.
*/
remove: function(elementId) {
// console.log('remove('+elementId+')');
if (!elementId) return ;


Delete the above two lines. If the calling code is broken, let it
break so you can see the problem(s).


var el, elem = this._elem ;
if (!elem || !elem.removeChild) return ;


Huh? No removeChild method featured, so fail silently?


el = document.getElementById(elementId) ;
if (el) {
el = elem.removeChild(el) ;
el = null;
}
},

/**
If an element was set on this context when it was created, this
method
will actually apply any changes to the element itself. If you
have not
written any inner html into the context, then the innerHTML of
the
element will not be changed, otherwise it will be replaced with
the new
innerHTML.

Also, any attributes, id, classNames or styles you've set will be
updated as well. This also ends the editing context session and
cleans
up.

@returns {SC.RenderContext} previous context or null if top
*/
update: function() {
var elem = this._elem,
mode = this.updateMode,
key, value, styles, factory, cur, next, before;

this._innerHTMLReplaced = NO;

if (!elem) {
// throw "Cannot update context because there is no source
element";
return ;


*Sigh* See above.


}

// console.log('%@#update() called'.fmt(this));
// if (this.length>0) console.log(this.join());
// else console.log('<no length>');
// replace innerHTML
if (this.length>0) {
this._innerHTMLReplaced = YES;
if (mode === SC.MODE_REPLACE) {
elem.innerHTML = this.join();
} else {
factory = elem.cloneNode(false);
factory.innerHTML = this.join() ;
before = (mode === SC.MODE_APPEND) ? null : elem.firstChild;
cur = factory.firstChild ;
while(cur) {
next = cur.nextSibling ;
elem.insertBefore(cur, next);
cur = next ;
}
cur = next = factory = before = null ; // cleanup
}
}

// note: each of the items below only apply if the private
variable has
// been set to something other than null (indicating they were
used at
// some point during the build)

// if we have attrs, apply them
if (this._attrsDidChange && (value = this._attrs)) {
for(key in value) {
if (!value.hasOwnProperty(key)) continue;


You just sank Safari 2. See previous post.


if (value[key] === null) { // remove empty attrs
elem.removeAttribute(key);

LOL. You are making the same mistakes as jQuery, Prototype, YUI, etc.

http://www.cinsoft.net/attributes.html


} else {
SC.$(elem).attr(key, value[key]);

You can't be a little pregnant. Thank you very much for wasting my
time.

 

[Garrett Smith]

After 20000 lines of new code, over 5000 new unit tests, and countless
hours of effort by over 30 contributors, SproutCore 1.0 is ready. ...

Is it?

First suspicious file is browser.js (wonder what that could be?)
[...]
/** Detects the current browser type. Borrowed from jQuery + prototype
*/

Th idea of parsing navigator.userAgent is faulty inference.

Oh no.


SC.browser = (function() {

var userAgent = navigator.userAgent.toLowerCase();
var version = (userAgent.match( /.+(?:rv|it|ra|ie)[\/: ]([\d.]+)/ )
|| [])[1] ;

var browser = /** @scope SC.browser */ {


Please, make it stop. Scope?

@Scope is a JS-doc comment. I did argue with Michael Matthews about that.

JS-Doc toolkit comments are a pain in hte neck to keep up to date and
just add clutter to the actual code. But anyway, that is why that
comment xists.

/** The current browser version */
version: version,

/** non-zero if webkit-based browser */
safari: (/webkit/).test( userAgent ) ? version : 0,

That is a wrongly named identifier. "webkit" is not safari. Webkit is on
Palm pre, in Chrome, in iris.

Non-zero? This must be their own unique interpretation of jQuery's
sniffing. I wonder if they realize that jQuery moved forward one step
to object inferences.


/** non-zero if this is an opera-based browser */
opera: (/opera/).test( userAgent ) ? version : 0,


The one browser in the world that is easy to detect and they blew it.
Of course, there should be no reason to detect Opera in 2010 (or 2009
or 2008...) Can only go downhill from here.


/** non-zero if this is IE */
msie: (/msie/).test( userAgent ) && !(/opera/).test( userAgent ) ?
version : 0,

MSIE is a very commonly faked user-agent. Recognizing that Opera masks
as "msie" should have been a hint that this might not be a good idea.

Sad, especially considering IE has conditional comments. Oh, but that
won't fit into their plans of creating a way-cool browser scripting
framework.

Conditional comments would be much more accurate here. ScriptEngine
would, too, though cc would be best.

mozilla: (/mozilla/).test( userAgent ) && !(/(compatible|
webkit)/).test( userAgent ) ? version : 0,

It is less error-prone to use 'Gecko'. Applications that embed Mozilla
should not be using 'mozilla' in the userAgent, but would be likely to
have 'Gecko' in the user-agent. Regardless, this whole browser detection
inference is faulty.

/** non-zero if this is mobile safari */
mobileSafari: (/apple.*mobile.*safari/).test(userAgent) ?
version : 0,

/** non-zero if we are on windows */
windows: !!(/(windows)/).test(userAgent),

/** non-zero if we are on a mac */
mac: !!((/(macintosh)/).test(userAgent) || (/(mac os x)/).test
(userAgent)),

That should not be something the program would care about. A user
platform is not something that HTTP includes and might very well be
excluded from an embedded system.

language: (navigator.language || navigator.browserLanguage).split
('-', 1)[0]
};

// Add more SC-like descriptions...


Huh? Comments that don't make sense are worse than no comments at
all.

That is a senseless comment.

[...]

current: browser.msie ? 'msie' : browser.mozilla ? 'mozilla' :
browser.safari ? 'safari' : browser.opera ? 'opera' : 'unknown'

It looks like four browsers. There are more than four browsers available
on the market and many of these are going to sadly fall into the
category of "unknown".

}) ;

return browser ;

})();


I don't even want to look in the next file. How about...
core_query.js. Sounds promising.


//
==========================================================================
// Project: SproutCore - JavaScript Application Framework
// Copyright: ©2006-2009 Sprout Systems, Inc. and contributors.
// Portions ©2008-2009 Apple Inc. All rights reserved.
// License: Licened under MIT license (see license.js)
//
==========================================================================
/*globals CQ add*/

require('system/builder') ;

/**
CoreQuery is a simplified DOM manipulation library used internally
by SproutCore to find and edit DOM elements. Outside of SproutCore,
you should generally use a more full-featured DOM library such as
Prototype or jQuery.

That is a useless comment. Useless because it does not state what
features are lacking and what these libraries should be used for.

"CoreQuery itself is a subset of jQuery with some additional plugins.
If you have jQuery already loaded when SproutCore loads, in fact, it
will replace CoreQuery with the full jQuery library and install any
CoreQuery plugins, including support for the SC.Enumerable mixin."



Well, this is going to be a very short review...


"Much of this code is adapted from jQuery 1.2.6, which is available
under an MIT license just like SproutCore."

jQuery 1.2 has a lot of obvious bugs that were fixed. It should not be
used at this time.

...and history lesson. :


var styleFloat = SC.browser.msie ? "styleFloat" : "cssFloat";

I remember that one. It's pretty much Game Over as everything else in
the ZIP sits atop this.

The problem with this inference is that it ties browser that has 'msie'
in userAgent to using 'styleFloat' and browsers that do not use 'cssFloat'.

Where available, the standard cssFloat property should be used. It is
very easy to detect:-

var FLOAT_PROP = "cssFloat";
if(document.documentElement.style[FLOAT_PROP] === "undefined") {
FLOAT_PROP = "styleFloat";
}

This assumes that cssFloat is supported, and if it isn't, then it uses
styleFloat.

// A helper method for determining if an element's values are broken


Again, worthless comments are worse than worthless.


var styleIsBorked = function styleIsBorked( elem ) {

Best to not repeat feature tests; perform the feature test only once.

Oh, a named function expression. Somebody let the developers out of
the sound-proof booth. They need to make contact with the rest of the
world.

There is no good reason to not use a FunctionDeclaration there.

if ( !SC.browser.safari ) return false;


So, only "values" in Safari (or whatever this thing thinks is Safari)
can be "borked". I really dislike cutesy geek-speak like that. It's
another layer of cheese (on top of a moldy jQuery circa 2007).

It isn't just Safari, remember, but *any* webkit here. Only a browser
that identifies as webkit may be borked.

// defaultView is cached


Again with the silly comments.


var ret = defaultView.getComputedStyle( elem, null );
return !ret || ret.getPropertyValue("color") === "";
} ;

The method call to `getPropertyValue("color") can be replaced with
ret.color.

// implement core methods here from jQuery that we want available all
the
// time. Use this area to implement jQuery-compatible methods ONLY.
// New methods should be added at the bottom of the file, where they
will
// be installed as plugins on CoreQuery or jQuery.
CQ = CoreQuery = SC.Builder.create( /** @scope SC.CoreQuery.fn */ {

That looks like an assignment to an undeclared identifier.

The identifiers should have been declared using `var`.

// Add some global helper methods.
SC.mixin(SC.$, {

The identifier `$` is not descriptive. It should be renamed to describe
what it means.

/** @private helper method to determine if an element is visible.
Exposed

yes, the method is exposed.

for use in testing. */
isVisible: function(elem) {
var CQ = SC.$;
return ("hidden"!=elem.type) && (CQ.css(elem,"display")!="none")
&& (CQ.css(elem,"visibility")!="hidden");
}

}) ;

It might be worth considering the case for visibility: collapse.

I've seen that pattern before (and it never fails to make me cringe).
What on earth do hidden inputs have to do with elements that have been
styled hidden (or removed from the layout?) The developers can't see
them? Thanks for bottling that guys.


for(var key in enumerable) {

if (!enumerable.hasOwnProperty(key)) continue ;


Oops, just lost Safari 2. It's not that it's necessary to support all
older browsers, but you can't just cut them off in the middle of a
script with an exception. I assume the developers didn't realize what
they were doing (usually a fair assumption).


for ( name in options ) {
CQ.attr(
(type)?this.style:this,
name, CQ.prop( this, options[ name ], type, i, name ));
}


No filter on that one? The attr method is the usual suspect from
jQuery. Hard to believe anyone would copy it at this point. Of
course, it's hard to believe anyone would still be using it at this
late date. Whatever.

I have no idea what that method is intended to perform, but if it is
using jQuery.attr, it is obviously in very poor judgement, as is
everything else here.

// Check to see if the W3C box model is being used
boxModel: !SC.browser.msie || document.compatMode ===
"CSS1Compat",


That was in jQuery until they "punted" box models (along with quirks
mode in general).

isCoreQuery: YES, // walk like a duck


That's about enough I think. Over 30 people wasted a lot of time
building widgets on top of this rubbish heap.

Sounds like Dojo. This library is not going to die.

 

[David Mark]

[...]

Sounds like Dojo. This library is not going to die.

Is or isn't? It is a shame that so many people have worked for so
long to produce something like this. I don't wish failure on anybody,
but some people refuse to be helped (see the predicted Real World
comment from the "creator" of this thing).

 

[Garrett Smith]

Hi I am the creator of sproutcore. If you had taken the time to look at the rest of sproutcore you might have noticed that corequery is not used very often because our view layer has a more powerful system called the rendercontext.

This message is not wrapping so well. What newsreader are you using?

Also I think nitpicking little bits of code like this is really pointless. Anyone who has written a big project knows that some parts receive more attention then others based on their actual real world needs. Picking one remote part like this and disecting it misses all to useful stuff you might have found if you has just looked.

That would seem like nitpicking if the point was not quality code.

The code in Sproutcore that was just displayed has problems. Browser
detection is known to be the source of forwards compibility issues.
Basing a library on faulty inferences at the base level in the
dependency chain is a disastrous decision. Such inferences and thinking
for detecting styleFloat show another poor inference.

None of that would pass code review. You probably don't want to hear
that and have probably spent a good amount of time on it, but it is the
truth.

For example spend some time looking at the datastore or observer layers

If you would like to prepare for a code review, then this is a fine place.

I recommend formatting the code to 72 chars. you should prepare to
explain briefly the intent of the code (use diagrams if you like).
Posters of this newsgroup will find the errors and mistakes in it for you.

I can also meet you in person, as you're local.
Ah, using a web forum. Try downloading Thunderbird an getting an account
with a newsgroup provider. I use eternal-september.org. THe newsgroup is
comp.lang.javascript.

 

[RobG]

After 20000 lines of new code, over 5000 new unit tests, and countless
hours of effort by over 30 contributors, SproutCore 1.0 is ready. ...

Is it?

"Ready" for some unspecified purpose.

Unfortunately this crap is at the heart of Apple's MobileMe site.
Despite the "mobile" claim, it doesn't work on iPhone. They sniff for
mobile safari, block entry and tell you to use iPhone apps instead. It
also sniffs for, and refuses entry to, IE 6.

It is slow and fat - even simple pages are 1.5mb. If this is the
future of web applications, count me out.

 

[Thomas 'PointedEars' Lahn]

Hi I am the creator of sproutcore. If you had taken the time to look at
the rest of sproutcore you might have noticed that corequery is not used
very often because our view layer has a more powerful system called the
rendercontext.

[snipped 400+ lines of garbage]

You have been told before that your code reviews are next to unreadable,
thus next to useless. You have also been asked before to take more care
when posting them. Suggestions have been made how this could be
accomplished. Please do everyone a favor and, as your New Year's
resolution, do not waste more bandwidth like this again. Thanks in advance.


PointedEars

 

[David Mark]

[snipped 400+ lines of garbage]

You have been told before that your code reviews are next to unreadable,
thus next to useless.

Wonder why they get so much attention then. And this addendum was
not so much a review as a demonstration of some very bad code (which
had been described as "more powerful" by the creator). It really
needed no introduction.

You have also been asked before to take more care
when posting them.
Yes.

Suggestions have been made how this could be
accomplished.
Yes.

Please do everyone a favor and, as your New Year's
resolution, do not waste more bandwidth like this again.

No, I'm sorry, To hell with the bandwidth.

 

[Thomas 'PointedEars' Lahn]

Charles Jolley wrote:
Hi I am the creator of sproutcore. If you had taken the time to look
at the rest of sproutcore you might have noticed that corequery is
not used very often because our view layer has a more powerful system
called the rendercontext.

[snipped 400+ lines of garbage]

You have been told before that your code reviews are next to unreadable,
thus next to useless.

Wonder why they get so much attention then.

Interest in a thing does not imply acceptance of that thing. Besides, I
would not call four followups, out of a group of regulars of at least 10
people, out of a newsgroup used by at least 2313 people, out of a medium
used by millions, "much attention".

And this addendum was not so much a review as a demonstration of some
very bad code (which had been described as "more powerful" by the
creator). It really needed no introduction.

I am sure nobody expected an introduction. This is about the difficulty a
reader must be having when trying to determine which part of your posting
belongs to the code and which part to your comments about it. Such
postings are _not_ useful, and they certainly do not help anyone (to see
your point).

No, I'm sorry, To hell with the bandwidth.

And to hell with your readers? So you are just a poser after all?

If you do not want to post so that it can be easily read, you are much
better off with writing a diary.


PointedEars

 

[David Mark]

David Mark wrote:
Charles Jolley wrote:
Hi I am the creator of sproutcore.  If you had taken the time to look
at the rest of sproutcore you might have noticed that corequery is
not used very often because our view layer has a more powerful system
called the rendercontext.
[snipped 400+ lines of garbage]
You have been told before that your code reviews are next to unreadable,
thus next to useless.

Wonder why they get so much attention then.  

Interest in a thing does not imply acceptance of that thing.  Besides, I
would not call four followups, out of a group of regulars of at least 10
people, out of a newsgroup used by at least 2313 people, out of a medium
used by millions, "much attention".

And this addendum was not so much a review as a demonstration of some
very bad code (which had been described as "more powerful" by the
creator).  It really needed no introduction.

I am sure nobody expected an introduction.  This is about the difficulty a
reader must be having when trying to determine which part of your posting
belongs to the code and which part to your comments about it.  Such
postings are _not_ useful, and they certainly do not help anyone (to see
your point).

No, I'm sorry,  To hell with the bandwidth.

And to hell with your readers?  So you are just a poser after all?

As I said, that was pretty much a code dump, not a review. And don't
you worry about my readers as I am about to launch a site that
summarizes all of this stuff in one easy to find place (and that place
will not resemble GG).

If you do not want to post so that it can be easily read, you are much
better off with writing a diary.

So you feel my voice isn't being heard?

 

[Thomas 'PointedEars' Lahn]

As I said, that was pretty much a code dump, not a review. And don't
you worry about my readers as I am about to launch a site that
summarizes all of this stuff in one easy to find place (and that place

We (I think I can speak for all subscribers regarding this) do not need
*such* "code dumps" here, thank you very much.

So you feel my voice isn't being heard?

That is beside the point. Your voice would certainly be heard better and
more clear if you would come to your senses and post something better
readable than this.


EOD

PointedEars

 

[David Mark]

We (I think I can speak for all subscribers regarding this) do not need
*such* "code dumps" here, thank you very much.

Ah, the royal "we". I don't think you speak for all "subscribers" at
all. I'd just as soon I speak for them. Thank you very much.

That is beside the point.

Then I must have missed your point.

Your voice would certainly be heard better and
more clear if you would come to your senses and post something better
readable than this.
Yes.


EOD

Mad as a mongoose named Maddie on a particularly mad day.

 

[Garrett Smith]

We (I think I can speak for all subscribers regarding this) do not need
*such* "code dumps" here, thank you very much.


That is beside the point. Your voice would certainly be heard better and
more clear if you would come to your senses and post something better
readable than this.

Good reviews take time. I can try and take a partial review:

Looking at the datastore:

http://github.com/sproutit/sproutcore/blob/master/frameworks/datastore/models/record.js

That depends on SC.Object.extend, defined here:-

http://github.com/sproutit/sproutcore/blob/master/frameworks/runtime/system/object.js

It is well-commented, I'll give it that.

I'm not accustomed to where everything is defined and don't find it all
right away (the packaging is not intuitive). It is hard to follow method
calls across several files and it is unclear which file the method is
defined in. Things are added to Function.prototype in various places,
but it is uncertain where.

I see global identifiers, such as `YES and `NO` (silly), global
identifiers in IE, via NFE. I see cases not considering older browsers
(hasOwnProperty).

The design of many functions/methods leaks implementation details out
using fake private. This is unnecessary and usually not very hard to avoid.

I also see some benchmarking interspersed with the code. What does
benchmarking have to do with extending objects?

http://github.com/sproutit/sproutcore/blob/master/frameworks/runtime/system/object.js

My inline comments as "// (GS) ... ".


extend: function(props) {

// (GS) What does benchmarking have to do with extending objects?
// (GS) I suggest removing the benchmarking. You might want to put it in
// (GS) completely separate AOP layer. Or just get rid of it (I would).
var bench = SC.BENCHMARK_OBJECTS ;
if (bench) SC.Benchmark.start('SC.Object.extend') ;

// build a new constructor and copy class methods. Do this before
// adding any other properties so they are not overwritten by the
// copy.

// (GS) This looks like you're building up a big global SC.Object with
// (GS) more properties coming from anyone who should call
// (GS) `SC.Object.extend`. What is SC.Object defining?
//
// (GS) Why the underscore here? This is a globally-accessible
// (GS) SC.Object.prototype._object_init; it is not really private.
var prop, ret = function(props) { return this._object_init(props);};
for(prop in this) {
if (!this.hasOwnProperty(prop)) continue ;

// (GS) Avoid referencing `this` in static context.
ret[prop] = this[prop];
}

// (GS) `hasOwnProperty` won't work in some implementations.
// (GS) Also, `toString` is not the only problematic one; Did you
// (GS) consider `valueOf`?
// manually copy toString() because some JS engines do not enumerate it

if (this.hasOwnProperty('toString')) ret.toString = this.toString;

// now setup superclass, guid
ret.superclass = this ;
SC.generateGuid(ret); // setup guid

ret.subclasses = SC.Set.create();
this.subclasses.add(ret); // now we can walk a class hierarchy

// setup new prototype and add properties to it
var base = (ret.prototype = SC.beget(this.prototype));
var idx, len = arguments.length;
for(idx=0;idx<len;idx++) SC._object_extend(base, arguments[idx]) ;
base.constructor = ret; // save constructor

// (GS) Again with the benchmarking.
if (bench) SC.Benchmark.end('SC.Object.extend') ;
return ret ;
},

I see that function calls various other functions. I see a call to
SC._object_extend. I've modified the code to wrap at 72 chars.


// (GS) Named FunctionExpression will add a global `_object_extend`
// (GS) property in JScript <= 5.8. I suggest not doing that.
SC._object_extend = function _object_extend(base, ext) {

// (GS) Suggest throwing an object. That helps some Debuggers get a
// (GS) stack trace (error.stack).
if (!ext) throw "SC.Object.extend expects a non-null value. "
+ "Did you forget to 'sc_require' something? Or were you passing a "
+ " Protocol to extend() as if it were a mixin?";

// (GS) There is not need to leak implementation details to the base
// (GS) object. I suggest avoiding doing that, so that these
// (GS) implementation details remain hidden, and can never be relied
// (GS) on.
// set _kvo_cloned for later use
base._kvo_cloned = null;


I've given enough suggestions. There is enough complexity to confuse me.
I managed to find SC.Set.

http://github.com/sproutit/sproutcore/blob/master/frameworks/runtime/system/set.js
End.

How's that?

 

[Jorge]

(...)

It's so funny and ridiculous and seems sooo stupid that the same guys
that advocate turning off JS in the browser (many cljs regulars), feel
themselves as an authority so as to judge somebody else's work whose
main goal/underlying idea don't even understand (e.g., you don't know
a word of cocoa), just by looking at a few lines in their JS source.

 

[Jorge]

(e.g., you don't know
a word of cocoa),

Utterly irrelevant. Apple's Cocoa is a "collection of frameworks, APIs,
and accompanying runtimes"[1] specifically for Mac OS X developers. That
is unrelated to a script library that is supposed to be suitable for web
applications and presumably platforms other than Safari on Mac OS.

In case you didn't know, SproutCore is Cocoa for the Web.

 

[Jorge]

(...)
Cljs is *the* place to discuss technical issues related to it.
(...)

Sadly, in higher profile forums, cljs is seen more and more as a bunch
of clueless idiots. Garret is helping a lot with that, for in every
other forum he dares to put a feet on -where he invariably tries to
impersonate as the author on the cljs faq (*)- it's just to annoy them
with pointless, arrogant, ridiculous posts of little or no value at
all, except for his great success in pissing off the regulars.

(*)See his signature, and the 1st line in the cljs faq states "by"
him. Given the negative image he projects wherever he goes, I wonder
whether -for the good of cljs- his name ought to be there at all to
begin with.

 

[Garrett Smith]

It's so funny and ridiculous and seems sooo stupid that the same guys
that advocate turning off JS in the browser (many cljs regulars),

I don't agree that "many" cljs regulars recommend turning off javascript
on a regular basis. It is certainly worth doing on sites that throw
script errors or are otherwise unusable.

Unfortunately many sites are needlessly dependent on script so turning
script support off is not always a useful an option. However, plugins
like NoScript allow users to selectively run scripts so that they can
accomplish what they want without running all scripts that a site has.

feel
themselves as an authority so as to judge somebody else's work whose
main goal/underlying idea don't even understand

The subject of this thread is from Sproutcore's home page, which also
states:

"Create fast, native-style applications in any modern web browser
without plugins."

It also claims to be a "JavaScript HTML5 Application Framework"

<URL: http://www.sproutcore.com/home/ >

Cljs is *the* place to discuss technical issues related to it.

(e.g., you don't know
a word of cocoa),

Utterly irrelevant. Apple's Cocoa is a "collection of frameworks, APIs,
and accompanying runtimes"[1] specifically for Mac OS X developers. That
is unrelated to a script library that is supposed to be suitable for web
applications and presumably platforms other than Safari on Mac OS.

just by looking at a few lines in their JS source.

Those who have commented here have looked at more than "a few lines" -
SproutCore is tens of thousand of lines of code, it is incredibly
verbose. No one should post a review of the entire library here.

For something 20k long, I would consider a few things in assessing it:
1) What is the overall goal of the framework
2) How is is structured?

After that, I would want to look at the lowest level core and see the
actual code.

I want to know that the library is using abstractions that are
well-written. The core file has browser detection, global identifiers,
recommendations to use either PrototypeJS or jQuery.

I see some higher level parts of the framework relying on modifications
to Function.prototype, for example. I saw some place that defined
certain modifications to Function.prototype, but not the modifications
that were used in the datastore's renderer.

I have also noticed recent commits in GitHub with things like:-

Commit comment:
"Make it work a bit better in IE"

Where "a bit better" is a euphemism for BUGFIX.

The previous code was:-
| if (SC.none(newLayout[key])) delete style[key];

And the new code is:-
| if (SC.none(newLayout[key])) style[key] = ""; // because IE is stupid
|// and can't handle delete or debug.

What is debug?

Calling `delete` on a host object results in Errors in IE.

It is good that the author did recognize and fix the bug. But it would
be hard to notice the bug without at least trying once in IE. Also, the
comment should not be calling IE "stupid" for what is perfectly legal
behavior. IE throwing on delete for host object is legal in ES3,
through vague wording, and legal in ES5 through explicit wording via
internal [[Delete]] (P, Throw).

Setting a style property to `null` IE, as another comment indicated was
attempted, might also throw "Invalid Argument" errors. The program
should only supply that are legal CSS values. Literal `null` might
convert to domstring "null", which would either illegal CSS value (or
useless things like "font family", where it would be legal).

I also see things in random places such as:-
http://github.com/sproutit/sproutcore/blob/master/frameworks/designer/css/css_style_sheet.js

My inline comments follow:

| init: function() {
| // (GS) See note below on Transcoded Function call.
| sc_super() ;
|
| var ss = this.styleSheet ;
|
| if (!ss) {
| // (GS) Extract this branch to a hidden-in-scope function.
| // create the stylesheet object the hard way (works everywhere)
| ss = this.styleSheet = document.createElement('style') ;
| ss.type = 'text/css' ;
| var head = document.getElementsByTagName('head')[0] ;
|
| // (GS) What does this fix for Opera?
| if (!head) head = document.documentElement ; // fix for Opera
| head.appendChild(ss) ;
| }
|
| // (GS) Move this "get or create" Factory logic elsewhere. It does not
| // (GS) belong in init().
| // cache this object for later
| var ssObjects = this.constructor.styleSheets ;
| if (!ssObjects) ssObjects = this.constructor.styleSheets = {} ;
| ssObjects[SC.guidFor(ss)] ;
|
| // (GS) This is either undefined or an IE `rules` collection.
| // create rules array
|
| var rules = ss.rules || SC.EMPTY_ARRAY ;
| var array = SC.SparseArray.create(rules.length) ;
|
| // (GS) Avoid leaking implementation details to the system.
| array.delegate = this ;
| this.rules = array ;
|
| return this ;
| },

The transcoded function call, mentioned the first review comment, is
apparently changed to:- arguments.callee.base.apply(this, arguments);

That statement, used in the `init` method below, means that
`arguments.callee` *is* the `init` method. init.base is not defined in
`SC.CSSStyleSheet`. It seems as though it is defined as an extension to
`Function.prototype`, though at this point, it might be transcoded to
something else.

Transcoding sc_super() creates a confusing coupling with the build tool.
That confusion and complexity is not justified by the syntactic sugar it
provides.

To the author: Replace that statement with real code and stop trying to
be clever.

We also see that in the actual function, there is a caching mechanism.
This "get or create" can be useful, but I don't think it belongs there
in `init`.

There is also the problem in that this method will only work in a
browser that has an IE `rules` collection. That includes IE and Safari,
and I don't know what else. Instead, the standard `cssRules` property
should be used with `rules` as a fallback.

I did not look at SC.SparseArray.create. I assume it is about on quality
with everything else seen so far.

 

[Jorge]

You've been drinking the Kool-Aid. That is a line introduced by Apple's
marketing team back in mid-2008 when they announced the new MobileMe
site based on SproutCore. It makes as much sense as calling it .NET for
the web.

http://www.google.com/search?q=cocoa+site:sproutcore.com

 

[RobG]

http://www.google.com/search?q=cocoa+site:sproutcore.com

Thanks for agreeing.

Your point was that a person can't do a worthwhile review of
SproutCore unless they have a good knowledge of Cocoa. Your basis for
that seems to be a bunch of marketing material that uses Cocoa as a
similie for an MVC framework.

There is no need to know about Cocoa at all when reviewing the library
code used by SproutCore, criticisms were based purely on its use of
javascript. If you can show that a specific criticim is not justified
based on some principle of MVC architecture, design pattern, code
style or some other relevant point, please do so.

Otherwise your point is irrelevant.