Skip to content

Instantly share code, notes, and snippets.

@addyosmani

addyosmani/codereview.md

Created August 25, 2011 12:11
Download ZIP

Revisions

  1. addyosmani revised this gist Aug 26, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -426,4 +426,4 @@ if (typeof MyNamespace == 'undefined') {

    @kangax has a very comprehensive post on namespacing patterns related to this here: http://perfectionkills.com/unnecessarily-comprehensive-look-into-a-rather-insignificant-issue-of-global-objects-creation/ (recommended reading)

    And that's it!. If enough readers find this useful I might post more code reviews in future. Until next time, good luck with your projects!
    And that's it!. If enough readers find this useful I might post more code reviews in future. Until next time, good luck with your own projects and reviews.
  2. addyosmani revised this gist Aug 26, 2011. 1 changed file with 2 additions and 0 deletions.
    2 changes: 2 additions & 0 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -425,3 +425,5 @@ if (typeof MyNamespace == 'undefined') {
    ```

    @kangax has a very comprehensive post on namespacing patterns related to this here: http://perfectionkills.com/unnecessarily-comprehensive-look-into-a-rather-insignificant-issue-of-global-objects-creation/ (recommended reading)

    And that's it!. If enough readers find this useful I might post more code reviews in future. Until next time, good luck with your projects!
  3. addyosmani revised this gist Aug 26, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -4,7 +4,7 @@ I was recently asked to review some code for a new JavaScript application and th

    ##Challenges & Solutions

    Code reviews go hand-in-hand with maintaining strong coding standards, however, standards don't prevent logical errors or misunderstandings about the quirks of a programming language. Even the most experienced developers can make these kinds of mistakes and code reviews can greatly assist with catching them.
    Code reviews go hand-in-hand with maintaining strong coding standards. That said, standards don't usually prevent logical errors or misunderstandings about the quirks of a programming language. Even the most experienced developers can make these kinds of mistakes and code reviews can greatly assist with catching them.

    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code.

  4. addyosmani revised this gist Aug 26, 2011. 1 changed file with 3 additions and 1 deletion.
    4 changes: 3 additions & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -4,9 +4,11 @@ I was recently asked to review some code for a new JavaScript application and th

    ##Challenges & Solutions

    Code reviews go hand-in-hand with maintaining strong coding standards, however, standards don't prevent logical errors or misunderstandings about the quirks of a programming language. Even the most experienced developers can make these kinds of mistakes and code reviews can greatly assist with catching them.

    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code.

    The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as something that can spur us to do better and improve ourselves - often, once we've calmed down, it actually does.
    The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as a learning experience that can spur us to do better and improve ourselves because in many cases, once we've calmed down, it actually does.

    It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the time spent on the input (regardless of whether you choose to use it or not).

  5. addyosmani revised this gist Aug 26, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -20,7 +20,7 @@ Twitter - it may seem like an odd suggestion, but at least half of the code revi

    ##My Review

    On to the review! A reader asked me to go through their code and provide them with some suggestions on how they might improve it. Whilst I'm certainly not an expert on code reviews, I believe that reviews should both point out the shortfalls of an implementation but also offer possible solutions and suggestions for further reading material that might be of assistance.
    On to the review - a reader asked me to go through their code and provide them with some suggestions on how they might improve it. Whilst I'm certainly not an expert on code reviews, I believe that reviews should both point out the shortfalls of an implementation but also offer possible solutions and suggestions for further reading material that might be of assistance.

    Here are the problems and solutions I proposed for the reader's code:

  6. addyosmani revised this gist Aug 26, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -10,7 +10,7 @@ The first reaction most of us have to criticism is to defend ourselves (or in th

    It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the time spent on the input (regardless of whether you choose to use it or not).

    ###JavaScript developers looking for a way to get code reviews have a few different options here:
    ####JavaScript developers looking for a way to get code reviews have a few different options here:

    JSMentors (http://jsmentors.com/) - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on their review panel (including JD Dalton, Angus Croll and Nicholas Zakas). These mentors might not always be readily available, but they do their best to provide useful, constructive feedback on code that's been submitted for review.

  7. addyosmani revised this gist Aug 26, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -10,7 +10,7 @@ The first reaction most of us have to criticism is to defend ourselves (or in th

    It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the time spent on the input (regardless of whether you choose to use it or not).

    JavaScript developers looking for a way to get code reviews have a few different options here:
    ###JavaScript developers looking for a way to get code reviews have a few different options here:

    JSMentors (http://jsmentors.com/) - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on their review panel (including JD Dalton, Angus Croll and Nicholas Zakas). These mentors might not always be readily available, but they do their best to provide useful, constructive feedback on code that's been submitted for review.

  8. addyosmani revised this gist Aug 26, 2011. 1 changed file with 12 additions and 3 deletions.
    15 changes: 12 additions & 3 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -2,9 +2,13 @@

    I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind. Code reviews are possibly the single biggest thing you can do to improve the overall quality of your solutions and if you're not actively taking advantage of them, you're possibly missing out on bugs you haven't noticed being found or suggestions for improvements that could make your code better.

    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code.
    ##Challenges & Solutions

    The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as something that can spur us to do better and improve ourselves - often, once we've calmed down, it actually does. It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the time spent on the input (regardless of whether you choose to use it or not).
    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code.

    The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as something that can spur us to do better and improve ourselves - often, once we've calmed down, it actually does.

    It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the time spent on the input (regardless of whether you choose to use it or not).

    JavaScript developers looking for a way to get code reviews have a few different options here:

    @@ -14,7 +18,12 @@ Code Review Beta (http://codereview.stackexchange.com) - You would be forgiven f

    Twitter - it may seem like an odd suggestion, but at least half of the code review requests that I submit are through social networks. This of course works best when your code is open-source, however it never hurts to try. The only thing I would suggest here is to ensure you're following or interacting with experienced developers - a code review completed by a developer with insufficient experience can sometimes be worse than no code review at all, so be careful!.

    Here's my review in full:
    ##My Review

    On to the review! A reader asked me to go through their code and provide them with some suggestions on how they might improve it. Whilst I'm certainly not an expert on code reviews, I believe that reviews should both point out the shortfalls of an implementation but also offer possible solutions and suggestions for further reading material that might be of assistance.

    Here are the problems and solutions I proposed for the reader's code:


    ####Problem: Functions to be used as callbacks (as well as objects) are passed as parameters to other functions without any type validation.

  9. addyosmani revised this gist Aug 26, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -8,7 +8,7 @@ The first reaction most of us have to criticism is to defend ourselves (or in th

    JavaScript developers looking for a way to get code reviews have a few different options here:

    JSMentors (http://jsmentors.com/) - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on their review panel (including JD Dalton and Nicholas Zakas). These mentors might not always be readily available, but they do their best to provide useful, constructive feedback on code that's been submitted for review.
    JSMentors (http://jsmentors.com/) - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on their review panel (including JD Dalton, Angus Croll and Nicholas Zakas). These mentors might not always be readily available, but they do their best to provide useful, constructive feedback on code that's been submitted for review.

    Code Review Beta (http://codereview.stackexchange.com) - You would be forgiven for confusing Code Review with StackOverflow, but it's actually a very useful, broad-spectrum subjective feedback tool for requesting peer reviews of code you've written. Whilst on StackOverflow you might ask the question 'why isn't my code working?', CR is more suited to questions like 'why is my code so ugly?'. If there's still any doubt at all about what it offers, I strongly recommend checking out their FAQs (http://codereview.stackexchange.com/faq).

  10. addyosmani revised this gist Aug 26, 2011. 1 changed file with 4 additions and 2 deletions.
    6 changes: 4 additions & 2 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -2,13 +2,15 @@

    I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind. Code reviews are possibly the single biggest thing you can do to improve the overall quality of your solutions and if you're not actively taking advantage of them, you're possibly missing out on bugs you haven't noticed being found or suggestions for improvements that could make your code better.

    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code. The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as something that can spur us to do better and improve ourselves. It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the input (regardless of whether you choose to use it or not).
    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code.

    The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as something that can spur us to do better and improve ourselves - often, once we've calmed down, it actually does. It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the time spent on the input (regardless of whether you choose to use it or not).

    JavaScript developers looking for a way to get code reviews have a few different options here:

    JSMentors (http://jsmentors.com/) - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on their review panel (including JD Dalton and Nicholas Zakas). These mentors might not always be readily available, but they do their best to provide useful, constructive feedback on code that's been submitted for review.

    Code Review Beta - You would be forgiven for confusing Code Review with StackOverflow, but it's actually a very useful, broad-spectrum subjective feedback tool for requesting peer reviews of code you've written. Whilst on StackOverflow you might ask the question 'why isn't my code working?', CR is more suited to questions like 'why is my code so ugly?'. If there's still any doubt at all about what it offers, I strongly recommend checking out their FAQs (http://codereview.stackexchange.com/faq).
    Code Review Beta (http://codereview.stackexchange.com) - You would be forgiven for confusing Code Review with StackOverflow, but it's actually a very useful, broad-spectrum subjective feedback tool for requesting peer reviews of code you've written. Whilst on StackOverflow you might ask the question 'why isn't my code working?', CR is more suited to questions like 'why is my code so ugly?'. If there's still any doubt at all about what it offers, I strongly recommend checking out their FAQs (http://codereview.stackexchange.com/faq).

    Twitter - it may seem like an odd suggestion, but at least half of the code review requests that I submit are through social networks. This of course works best when your code is open-source, however it never hurts to try. The only thing I would suggest here is to ensure you're following or interacting with experienced developers - a code review completed by a developer with insufficient experience can sometimes be worse than no code review at all, so be careful!.

  11. addyosmani revised this gist Aug 26, 2011. 1 changed file with 3 additions and 1 deletion.
    4 changes: 3 additions & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -6,7 +6,9 @@ Often the most challenging part of code reviews is actually finding an experienc

    JavaScript developers looking for a way to get code reviews have a few different options here:

    JSMentors - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on board (including JD Dalton and Nicholas Zakas) who are often available to comment on code that's been submitted for review.
    JSMentors (http://jsmentors.com/) - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on their review panel (including JD Dalton and Nicholas Zakas). These mentors might not always be readily available, but they do their best to provide useful, constructive feedback on code that's been submitted for review.

    Code Review Beta - You would be forgiven for confusing Code Review with StackOverflow, but it's actually a very useful, broad-spectrum subjective feedback tool for requesting peer reviews of code you've written. Whilst on StackOverflow you might ask the question 'why isn't my code working?', CR is more suited to questions like 'why is my code so ugly?'. If there's still any doubt at all about what it offers, I strongly recommend checking out their FAQs (http://codereview.stackexchange.com/faq).

    Twitter - it may seem like an odd suggestion, but at least half of the code review requests that I submit are through social networks. This of course works best when your code is open-source, however it never hurts to try. The only thing I would suggest here is to ensure you're following or interacting with experienced developers - a code review completed by a developer with insufficient experience can sometimes be worse than no code review at all, so be careful!.

  12. addyosmani revised this gist Aug 26, 2011. 1 changed file with 4 additions and 2 deletions.
    6 changes: 4 additions & 2 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -1,8 +1,10 @@
    #Lessons From A JavaScript Code Review

    I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind. Code reviews are possibly the single biggest thing you can do to improve your code and if you're not actively taking advantage of them, you're possibly missing out on bugs you haven't noticed being found or suggestions for improvements that could make your code better.
    I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind. Code reviews are possibly the single biggest thing you can do to improve the overall quality of your solutions and if you're not actively taking advantage of them, you're possibly missing out on bugs you haven't noticed being found or suggestions for improvements that could make your code better.

    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code. JavaScript developers looking for a way to get code reviews have a few different options here:
    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code. The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as something that can spur us to do better and improve ourselves. It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the input (regardless of whether you choose to use it or not).

    JavaScript developers looking for a way to get code reviews have a few different options here:

    JSMentors - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on board (including JD Dalton and Nicholas Zakas) who are often available to comment on code that's been submitted for review.

  13. addyosmani revised this gist Aug 26, 2011. 1 changed file with 7 additions and 1 deletion.
    8 changes: 7 additions & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -1,6 +1,12 @@
    #Lessons From A JavaScript Code Review

    I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind. Code reviews are possibly the single biggest thing you can do to improve your code and if you're not actively taking advantage of them, you're missing out on some bugs being detected or suggestions for improvements that could make your code better.
    I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind. Code reviews are possibly the single biggest thing you can do to improve your code and if you're not actively taking advantage of them, you're possibly missing out on bugs you haven't noticed being found or suggestions for improvements that could make your code better.

    Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code. JavaScript developers looking for a way to get code reviews have a few different options here:

    JSMentors - this is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on board (including JD Dalton and Nicholas Zakas) who are often available to comment on code that's been submitted for review.

    Twitter - it may seem like an odd suggestion, but at least half of the code review requests that I submit are through social networks. This of course works best when your code is open-source, however it never hurts to try. The only thing I would suggest here is to ensure you're following or interacting with experienced developers - a code review completed by a developer with insufficient experience can sometimes be worse than no code review at all, so be careful!.

    Here's my review in full:

  14. addyosmani revised this gist Aug 26, 2011. 1 changed file with 3 additions and 1 deletion.
    4 changes: 3 additions & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -1,6 +1,8 @@
    #Lessons From A JavaScript Code Review

    I was recently asked to review some code for a JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind.
    I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind. Code reviews are possibly the single biggest thing you can do to improve your code and if you're not actively taking advantage of them, you're missing out on some bugs being detected or suggestions for improvements that could make your code better.

    Here's my review in full:

    ####Problem: Functions to be used as callbacks (as well as objects) are passed as parameters to other functions without any type validation.

  15. addyosmani revised this gist Aug 26, 2011. 1 changed file with 1 addition and 0 deletions.
    1 change: 1 addition & 0 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -50,6 +50,7 @@ if(/* condition for native support*/){
    /*method logic*/
    }
    }else{
    /*fallback*/
    tools.addMethod = function(/**/){
    /*method logic*/
    }
  16. addyosmani revised this gist Aug 26, 2011. 1 changed file with 3 additions and 3 deletions.
    6 changes: 3 additions & 3 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -226,7 +226,7 @@ The reason I recommend considering === for more specific type comparison (in thi
    If you're 100% certain that the values being compared cannot be interfered with by the user, feel free to opt for ==, however === would cover your bases in the invent of unexpected input being used.


    ####Problem: An uncached array.length is being used in all for loops.
    ####Problem: An uncached array.length is being used in all for loops. This is particularly bad as you're using it when iterating through 'HTMLCollection's.

    ```javascript
    eg.
    @@ -235,7 +235,7 @@ for(var i=0; i<myArray.length;i++){
    }
    ```

    Feedback: This isn't a great idea as the the array length is unnecessarily re-accessed on each loop iteration. This can be slow, especially if working with HTMLCollections. With the latter, caching the length can be anywhere up to 190 times faster than repeatedly accessing it (as per Zakas' High-performance JavaScript).
    Feedback: This isn't a great idea as the the array length is unnecessarily re-accessed on each loop iteration. This can be slow, especially if working with HTMLCollections. With the latter, caching the length can be anywhere up to 190 times faster than repeatedly accessing it (as per Zakas' High-performance JavaScript). See below for some options you have for caching the array length.

    ```javascript
    /*cached outside loop*/
    @@ -252,7 +252,7 @@ while (len--) {
    }
    ```

    See here for a jsPerf comparing the performance benefits of caching the array length inside and outside the loop, using prefix increments, counting down and more. http://jsperf.com/caching-array-length/7
    A jsPerf test comparing the performance benefits of caching the array length inside and outside the loop, using prefix increments, counting down and more is also available: http://jsperf.com/caching-array-length/7



  17. addyosmani revised this gist Aug 26, 2011. 1 changed file with 18 additions and 0 deletions.
    18 changes: 18 additions & 0 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -207,6 +207,24 @@ function make_promise() {
    ```


    ####Problem:You're testing for explicit numeric equality of a property using the == operator, but may wish to use === in this case instead

    As you may or may not know, the identity (==) operator compares for equality after after performing any necessary type conversions. The === operator will however not do this conversion so if the two values being compared are not the same type === will just return false.

    The reason I recommend considering === for more specific type comparison (in this case) is that == is known to have a number of gotchas. Take for example the following set of boolean checks for equality with it:

    ```javascript
    3 == "3" // true
    3 == "03" // true
    3 == "0003" // true
    3 == "+3" //true
    3 == [3] //true
    3 == (true+2) //true
    ' \t\r\n ' == 0 //true
    ```

    If you're 100% certain that the values being compared cannot be interfered with by the user, feel free to opt for ==, however === would cover your bases in the invent of unexpected input being used.


    ####Problem: An uncached array.length is being used in all for loops.

  18. addyosmani revised this gist Aug 25, 2011. 1 changed file with 15 additions and 3 deletions.
    18 changes: 15 additions & 3 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -241,13 +241,25 @@ See here for a jsPerf comparing the performance benefits of caching the array le
    ####Problem: Variables are declared all over the place.

    ```javascript
    var someData = /**/
    /*example*/
    var someData = "testing";
    someMethod.apply(someData);
    var otherData = /**/, moreData = /**/
    var otherData = "data1", moreData = "data2";
    moreMethods.call(otherMethods());
    ```

    Feedback: Within functions, it's significantly more clean to use the single-var pattern to declare variables at the top of a function's scope because of variable hoisting. This provides a single place to look for all variables, prevents logical errors with a variable being used prior to being defined and is generally less code. I also noticed variables being declared without an initial value. Whilst this is fine, note that initializing variables with a value can help prevent logical errors because uninitialized vars are initialised with a value of undefined.
    Feedback: Within functions, it's significantly more clean to use the single-var pattern to declare variables at the top of a function's scope because of variable hoisting.

    ```javascript
    var someData = "testing",
    otherData = "data1",
    moreData = "data2";

    someMethod.apply(someData);
    moreMethods.call(otherMethods());
    ```

    This provides a single place to look for all variables, prevents logical errors with a variable being used prior to being defined and is generally less code. I also noticed variables being declared without an initial value. Whilst this is fine, note that initializing variables with a value can help prevent logical errors because uninitialized vars are initialised with a value of undefined.



  19. addyosmani revised this gist Aug 25, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -134,7 +134,7 @@ Feedback: You may benefit from using Deferred execution (via promises and future

    I've written a relatively comprehensive post on this previously with Julian Aubourg if interested in doing this through jQuery (http://msdn.microsoft.com/en-us/scriptjunkie/gg723713), but it can of course be implemented with vanilla JavaScript as well.

    Micro-framework Q offers a CommonJS compatible implementation of promises/futures that is relatively comprehensive and can be used as follows:
    Micro-framework Q (https://github.com/kriskowal/q) offers a CommonJS compatible implementation of promises/futures that is relatively comprehensive and can be used as follows:

    ```javascript
    /*define a promise-only delay function that resolves when a timeout completes*/
  20. addyosmani revised this gist Aug 25, 2011. 1 changed file with 17 additions and 1 deletion.
    18 changes: 17 additions & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -1,6 +1,6 @@
    #Lessons From A JavaScript Code Review

    I was recently asked to review some code and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind.
    I was recently asked to review some code for a JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind.

    ####Problem: Functions to be used as callbacks (as well as objects) are passed as parameters to other functions without any type validation.

    @@ -31,6 +31,8 @@ var toType = function(obj) {
    ```




    ####Problem: There are a number of cases where checks for app specific conditions are repeatedly being made throughout the codebase (eg. feature detection, checks for supported ES5 features).

    Feedback: You might benefit from applying the load-time configuration pattern here (also called load-time or init-time branching). The basic idea behind it is that you test a condition only once (when the application loads) and then access the result of that initial test for all future checks.This pattern is commonly found in JavaScript libraries which configure themselves at load time to be optimized for a particular browser.
    @@ -107,6 +109,8 @@ if (typeof window.addEventListener === 'function') {
    }
    ```



    ####Problem: Object.prototype is being extended in many cases.

    Feedback: Extending native objects is a bad idea - there are few if any majorly used codebases which extend Object.prototype and there's unlikely a situation where you absolutely need to extend it in this way. In addition to breaking the object-as-hash tables in JS and increasing the chance of naming collisions, it's generally considered bad practice and should be modified as a very last resort (this is quite different to extending host objects).
    @@ -123,6 +127,7 @@ if(typeof Object.prototype.myMethod != 'function'){
    @kangax has a great post that discusses native & host object extension here: http://perfectionkills.com/extending-built-in-native-objects-evil-or-not/



    ####Problem: Some of what you're doing is heavily blocking the page as you're either waiting on processes to complete or data to load before executing anything further.

    Feedback: You may benefit from using Deferred execution (via promises and futures) to avoid this problem. The basic idea with promises are that rather than issuing blocking calls for resources, you immediately return a promise for a future value that will be eventually be fulfilled. This rather easily allows you to write non-blocking logic which can be run asynchronously. It's common to introduce callbacks into this equation so that the callbacks may be executed once the request completes.
    @@ -202,6 +207,7 @@ function make_promise() {
    ```



    ####Problem: An uncached array.length is being used in all for loops.

    ```javascript
    @@ -230,6 +236,8 @@ while (len--) {

    See here for a jsPerf comparing the performance benefits of caching the array length inside and outside the loop, using prefix increments, counting down and more. http://jsperf.com/caching-array-length/7



    ####Problem: Variables are declared all over the place.

    ```javascript
    @@ -242,6 +250,8 @@ moreMethods.call(otherMethods());
    Feedback: Within functions, it's significantly more clean to use the single-var pattern to declare variables at the top of a function's scope because of variable hoisting. This provides a single place to look for all variables, prevents logical errors with a variable being used prior to being defined and is generally less code. I also noticed variables being declared without an initial value. Whilst this is fine, note that initializing variables with a value can help prevent logical errors because uninitialized vars are initialised with a value of undefined.




    ####Problem: I noticed that jQuery's $.each() is being used to iterate over objects and arrays in some cases whilst for is used in others.

    Feedback: Whilst the lower level $.each performs better than $.fn.each(), both standard JavaScript for and while loops perform much better as proven by this jsPerf
    @@ -271,6 +281,8 @@ Given that this is a data-centric application with a potentially large quantity
    each object or array, it is recommended considering a refactor to use one of these.




    ####Problem: There are a number of aspects to the application which you may wish to make more easily configurable.

    Feedback: At the moment such settings as stored in
    @@ -286,6 +298,8 @@ var config = {
    };
    ```



    ####Problem: JSON strings are being built in-memory using string concatenation.

    Feedback: Rather than opting to do this, why not use JSON.stringify() - a method that accepts a JavaScript object and returns its JSON equivalent. Objects can generally be as complex or as deeply-nested as you wish and this will almost certainly result in a simpler, shorter solution.
    @@ -328,6 +342,8 @@ console.log(jsonData);
    */
    ```



    ####Problem: The namespacing pattern used is technically invalid

    Although namespacing across the rest of the application is implemented correctly, the initial check for namespace existence used is invalid. Here's what you have currently have:
  21. addyosmani revised this gist Aug 25, 2011. 1 changed file with 80 additions and 0 deletions.
    80 changes: 80 additions & 0 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -122,6 +122,86 @@ if(typeof Object.prototype.myMethod != 'function'){
    ```
    @kangax has a great post that discusses native & host object extension here: http://perfectionkills.com/extending-built-in-native-objects-evil-or-not/


    ####Problem: Some of what you're doing is heavily blocking the page as you're either waiting on processes to complete or data to load before executing anything further.

    Feedback: You may benefit from using Deferred execution (via promises and futures) to avoid this problem. The basic idea with promises are that rather than issuing blocking calls for resources, you immediately return a promise for a future value that will be eventually be fulfilled. This rather easily allows you to write non-blocking logic which can be run asynchronously. It's common to introduce callbacks into this equation so that the callbacks may be executed once the request completes.

    I've written a relatively comprehensive post on this previously with Julian Aubourg if interested in doing this through jQuery (http://msdn.microsoft.com/en-us/scriptjunkie/gg723713), but it can of course be implemented with vanilla JavaScript as well.

    Micro-framework Q offers a CommonJS compatible implementation of promises/futures that is relatively comprehensive and can be used as follows:

    ```javascript
    /*define a promise-only delay function that resolves when a timeout completes*/
    function delay(ms) {
    var deferred = Q.defer();
    setTimeout(deferred.resolve, ms);
    return deferred.promise;
    }

    /*usage of Q with the 'when' pattern to execute a callback once delay fulfils the promise*/
    Q.when(delay(500), function () {
    /*do stuff in the callback*/
    });
    ```


    If you're looking for something more basic that can be read through, Douglas Crockford's implementation of promises can be found below:

    ```javascript
    function make_promise() {
    var status = 'unresolved',
    outcome,
    waiting = [],
    dreading = [];

    function vouch(deed, func) {
    switch (status) {
    case 'unresolved':
    (deed === 'fulfilled' ? waiting : dreading).push(func);
    break;
    case deed:
    func(outcome);
    break;
    }
    };

    function resolve(deed, value) {
    if (status !== 'unresolved') {
    throw new Error('The promise has already been resolved:' + status);
    }
    status = deed;
    outcome = value;
    (deed == 'fulfilled' ? waiting : dreading).forEach(function (func) {
    try {
    func(outcome);
    } catch (ignore) {}
    });
    waiting = null;
    dreading = null;
    };

    return {
    when: function (func) {
    vouch('fulfilled', func);
    },
    fail: function (func) {
    vouch('smashed', func);
    },
    fulfill: function (value) {
    resolve('fulfilled', value);
    },
    smash: function (string) {
    resolve('smashed', string);
    },
    status: function () {
    return status;
    }
    };
    };
    ```


    ####Problem: An uncached array.length is being used in all for loops.

    ```javascript
  22. addyosmani revised this gist Aug 25, 2011. 1 changed file with 0 additions and 10 deletions.
    10 changes: 0 additions & 10 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -72,16 +72,6 @@ if(window.XMLHttpRequest){
    }
    }

    /*or alternatively if you wish to opt for using a ternary there's:*
    var utils = {
    getXHR: (function() {
    return window.XMLHttpRequest ?
    new XMLHttpRequest:
    new ActiveXObject('Microsoft.XMLHTTP');
    }())
    };
    /*thanks for @peolanha for this variation.*/

    ```

  23. addyosmani revised this gist Aug 25, 2011. 1 changed file with 12 additions and 0 deletions.
    12 changes: 12 additions & 0 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -71,6 +71,18 @@ if(window.XMLHttpRequest){
    return new ActiveXObject('Microsoft.XMLHTTP');
    }
    }

    /*or alternatively if you wish to opt for using a ternary there's:*
    var utils = {
    getXHR: (function() {
    return window.XMLHttpRequest ?
    new XMLHttpRequest:
    new ActiveXObject('Microsoft.XMLHTTP');
    }())
    };
    /*thanks for @peolanha for this variation.*/

    ```

    Stoyan Stefanov also has a great example of applying this to attaching and removing event listeners cross-browser in his book 'JavaScript Patterns':
  24. addyosmani revised this gist Aug 25, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -246,7 +246,7 @@ console.log(jsonData);
    */
    ```

    ####Problem: The namespacing pattern used is insufficient
    ####Problem: The namespacing pattern used is technically invalid

    Although namespacing across the rest of the application is implemented correctly, the initial check for namespace existence used is invalid. Here's what you have currently have:

  25. addyosmani revised this gist Aug 25, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -4,7 +4,7 @@ I was recently asked to review some code and thought I might share some of the f

    ####Problem: Functions to be used as callbacks (as well as objects) are passed as parameters to other functions without any type validation.

    Feedback: At minimum, 1) test to ensure the callback exists and 2) do a typeof check to avoid issues with the app attempting to execute input which may not in fact be a valid function at all.
    Feedback: For functions, at minimum 1) test to ensure the callback exists and 2) do a typeof check to avoid issues with the app attempting to execute input which may not in fact be a valid function at all.

    ```javascript
    if (callback && typeof(callback) === "function"){
  26. addyosmani revised this gist Aug 25, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -2,7 +2,7 @@

    I was recently asked to review some code and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind.

    ####Problem: Functions to be used as callbacks and objects are passed as parameters to other functions without any type validation.
    ####Problem: Functions to be used as callbacks (as well as objects) are passed as parameters to other functions without any type validation.

    Feedback: At minimum, 1) test to ensure the callback exists and 2) do a typeof check to avoid issues with the app attempting to execute input which may not in fact be a valid function at all.

  27. addyosmani revised this gist Aug 25, 2011. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -2,7 +2,7 @@

    I was recently asked to review some code and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind.

    ####Problem: Functions and objects to be used as callbacks are passed as parameters to other functions without any type validation.
    ####Problem: Functions to be used as callbacks and objects are passed as parameters to other functions without any type validation.

    Feedback: At minimum, 1) test to ensure the callback exists and 2) do a typeof check to avoid issues with the app attempting to execute input which may not in fact be a valid function at all.

  28. addyosmani revised this gist Aug 25, 2011. 1 changed file with 8 additions and 2 deletions.
    10 changes: 8 additions & 2 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -259,8 +259,14 @@ if (!MyNamespace) {
    The issue here is that !MyNamespace will throw a ReferenceError because the MyNamespace variable was never declared. A better pattern might take advantage of boolean conversion with an inner variable declaration as follows:

    ```javascript
    if (!MyApp) {
    var MyApp = { };
    if (!MyNamespace) {
    var MyNamespace = { };
    }

    //or

    if (typeof MyNamespace == 'undefined') {
    var MyNamespace = { };
    }
    ```

  29. addyosmani revised this gist Aug 25, 2011. 1 changed file with 20 additions and 0 deletions.
    20 changes: 20 additions & 0 deletions codereview.md
    View file
    Original file line number Diff line number Diff line change
    @@ -245,3 +245,23 @@ console.log(jsonData);
    {"dataA":["a","b","c","d"],"dataB":{"animal":"cat","color":"brown"},"dataC":{"vehicles":[{"type":"ford","tint":"silver","year":"2015"},{"type":"honda","tint":"black","year":"2012"}]},"dataD":{"buildings":[{"houses":[{"streetName":"sycamore close","number":"252"},{"streetName":"slimdon close","number":"101"}]}]}}
    */
    ```

    ####Problem: The namespacing pattern used is insufficient

    Although namespacing across the rest of the application is implemented correctly, the initial check for namespace existence used is invalid. Here's what you have currently have:

    ```javascript
    if (!MyNamespace) {
    MyNamespace = { };
    }
    ```

    The issue here is that !MyNamespace will throw a ReferenceError because the MyNamespace variable was never declared. A better pattern might take advantage of boolean conversion with an inner variable declaration as follows:

    ```javascript
    if (!MyApp) {
    var MyApp = { };
    }
    ```

    @kangax has a very comprehensive post on namespacing patterns related to this here: http://perfectionkills.com/unnecessarily-comprehensive-look-into-a-rather-insignificant-issue-of-global-objects-creation/ (recommended reading)
  30. addyosmani revised this gist Aug 25, 2011. No changes.