Thanks for the feedback everyone. I'm going to refactor my code using everybody's suggestions as soon as I get some free time and I'll reshare the updated code. Thanks again, I really do appreciate everyone's time.
Ryan On Oct 6, 7:17 am, "Fyodorov "bga" Alexander" <[email protected]> wrote: > bittersweetryan: > > > > > > > > > > > I've been trying quite hard to get more professional with my > > JavaScript development lately. I've been doing a lot of reading (blog > > posts, Crockford's book, and Haverbeke's book). Since reading about > > JavaScript is only part of the puzzle I've written a chrome extension > > to try to put what I've read to use. > > > Unfortunately I work for a small company and I don't have anybody to > > do a code review to help me see my errors and learn from them. I > > totally agree with Addy Osmani's comments on his blog post "Avoiding > > The Quirks: Lessons From A JavaScript Code Review" on how valuable a > > code review can be. I've tried to post my code on > > codereview.stackexchange.com, however I haven't received the quality > > of feedbck that I'm looking for. > > > The codebase isn't huge ~300 lines and its up on github - > >https://github.com/bittersweetryan/GitHub-News-Filter. If anybody has > > the time to look at it and give me some advise on what I could be > > doing better I'd really, really appreciate it. > > > Ryan > > 1) convert your code to > { > const singletonObject = (function(){ > // helper fns > > return { > // privare members > newsItems_: [], > > _init: function(){ > // your object init here > delete this._init // prevent double init > return this > }, > > // methods > getFilters: function(){ > > } > }._init() > })()} > > 2) replace {!localStogare[foo]} to {localStogare[foo] != null} > 3) put 1 extra space after comma in fn ivoke code { _fn(a, b) } > 4) you have monster fn {createElement}. Too many args. Plz use cfg > object. {createElement('div', {id: 'foo'})} > 5) you can forget about semicolon > 6) try to use fn style in js maximally, not > {getElementsByClass('div', 'foo')}, > { > el.getElementsByTag('div')._map(function(v){ return > v.getElementsByClass('foo') })._reduce(function(v1, v2){ return > v1.concat(v2) }) > } > 7) use {localStogare.getItem}, not {localStogare[]} > 8) alloc vars in place where you use it. For example in {getNewsItems} > you alloc {found} in top, but must in 68 line > 9) you use direct DOM building using {document.createElement}, use > templates, its more readable and maintainable than set of > {appendChild} and {createElement} > 10) in lines 71 - 75 you have {Array.prototype.indexOf} > 11) its good that you use rule 1 var = 1 line -- To view archived discussions from the original JSMentors Mailman list: http://www.mail-archive.com/[email protected]/ To search via a non-Google archive, visit here: http://www.mail-archive.com/[email protected]/ To unsubscribe from this group, send email to [email protected]
