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]
