SproutCore--over 20000 lines of new code!
in [JavaScript]
|
From: Thomas 'PointedEars' Lahn on 4 Jan 2010 01:14 David Mark wrote: > Thomas 'PointedEars' Lahn wrote: >> 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). >> 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. 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 -- Prototype.js was written by people who don't know javascript for people who don't know javascript. People who don't know javascript are not the best source of advice on designing systems that use javascript. -- Richard Cornford, cljs, <f806at$ail$1$8300dec7(a)news.demon.co.uk>
From: David Mark on 4 Jan 2010 01:18 On Jan 4, 1:14 am, Thomas 'PointedEars' Lahn <PointedE...(a)web.de> wrote: > David Mark wrote: > > Thomas 'PointedEars' Lahn wrote: > >> 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). > > >> 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. > > 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? :)
From: Thomas 'PointedEars' Lahn on 4 Jan 2010 01:24 David Mark wrote: > Thomas 'PointedEars' Lahn wrote: >> David Mark wrote: >> > Thomas 'PointedEars' Lahn wrote: >> >> 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. >> >> 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 We (I think I can speak for all subscribers regarding this) do not need *such* "code dumps" here, thank you very much. >> 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? :) 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 -- var bugRiddenCrashPronePieceOfJunk = ( navigator.userAgent.indexOf('MSIE 5') != -1 && navigator.userAgent.indexOf('Mac') != -1 ) // Plone, register_function.js:16
From: David Mark on 4 Jan 2010 01:55 On Jan 4, 1:24 am, Thomas 'PointedEars' Lahn <PointedE...(a)web.de> wrote: > David Mark wrote: > > Thomas 'PointedEars' Lahn wrote: > >> David Mark wrote: > >> > Thomas 'PointedEars' Lahn wrote: > >> >> 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. > > >> 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 > > 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. :) > > >> 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? :) > > 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.
From: Garrett Smith on 9 Jan 2010 03:11
Thomas 'PointedEars' Lahn wrote: > David Mark wrote: > >> Thomas 'PointedEars' Lahn wrote: >>> David Mark wrote: >>>> Thomas 'PointedEars' Lahn wrote: >>>>> 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. >>> 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 > > We (I think I can speak for all subscribers regarding this) do not need > *such* "code dumps" here, thank you very much. > >>> 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? :) > > 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? -- Garrett comp.lang.javascript FAQ: http://jibbering.com/faq/ |