Thomas said:
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/datastor...
That depends on SC.Object.extend, defined here:-
http://github.com/sproutit/sproutcore/blob/master/frameworks/runtime/...
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/...
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/...
End.
How's that?
