Incorrect JavaScript

1.
var Person = {
    "firstname" : "Mike",
    "middlename" : "D.",
    ...,
}
for (var item in Person) {
    document.writeln(Person[item]);
}
------------------------------------
for-in loop is used to iterate over nonarray object.
It makes no guarantee about the order of enumerated item.
For sequential integers, we should use an array; otherwise, use an object.
It's recommended to avoid the for-in loop unlesss your intent is to iterate over 
an unknown number of object properties. The for-in loop is significantly slower than
the other loops.
So we should use the following instead:
var myarray = ['firstname', 'middlename', ...];
for (var i = 0, max = myarray.length; ++i < max; ) {
    document.writeln(Person[myarray[i]]);
}
Please note: we should cache the length of the array and combine the test and increment 
of the loop variable to remove one name lookup. This can fast your code.
Similar case happens in Java, see "IncorrectJava.txt".

2.
var myURL = "myURL";
function func() {
    alert(myURL);  //   "undefined"
    var myURL = "otherURL";
    alert(myURL); //   "otherURL"
}
func();
--------------------------------------
The first alert() tries to use global var but fails
because the var is declared in the same scope later.

3.
// global function
function cat() {
  alert("global cat");
}
function dog() {
  alert("global dog");
}
function myPets() {
  cat(); // "local cat"
  dog(); // dog is not a functiion
  function cat() {
    alert("local cat");
 }
 var dog = function() {
   alert("local dog");
 };
}
myPets();
--------------------------------------------
This is a similar case as above case 2.
Local function definition cat() hoists and overwrites the global, 
but for dog() only its variable declaration dog is hoisted.

4.
var  item = "name";
alert(eval("obj." + item)); 
var mystring = "var num = 1; alert(num);";
eval(mystring);
--------------------------------------
Should avoid using eval() as much as possible.
Instead, using alert(obj[item]);
To prevent var num to be automatic global, use an immediate function:
(functiion () {
     eval(mystring);
}());

5.
setTimeout("myFunc()", 1000);
-----------------------------------------
Should use:
setTimeout(myFunc, 1000);
Javascript doesn't have threads, but we can use setTimeout() to emulate it.
Due to the thread nature of setTimeout(), the following code will have the unexpected result:
var myvar = 'Hello'; 
setTimeout(function() { alert(myvar); }, 1000);  // Alert prints out  Goodbye 
var myvar = 'Goodbye';
The code should be:
setTimeout(function() { alert(myvar); var myvar = 'Goodbye';}, 1000);  

6.
var Person = fuction (name) {
   this.name = name;
   this.say = fuction () {
       return "I am " + this.name;
   };
};
var Ming = new Person("Ming");
Ming.say(); // "I am Ming"
-----------------------------------------------
Every time we call new Person(), constructor function, the new function this.say() will be created; 
it's inefficient because the say() doesn't change from one instance to another. 
It's better to add the say() to the prototype of function object Person:
Person.prototype.say = fuction () {
       return "I am " + this.name;
};

7.
var greet = "Hello there";  // primitive string
greet.split(' ')[0]; // "Hello", greet was only temporarily converted to an object to make split() work
greet.smile = true; // try to augment the value and persist state, but it doesn't work although there is no an error.
---------------------------------------------------
Use the wrapper object to solve this problem:
var greet = new String("Hello there");

8.
var days = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat'],
       today = new Date(),
       message = 'Today is ' + days[today.getDay()] + ', ' + today.getDate();
alert(message);
----------------------------------------------------
Above code should be wrapped in an immediate function so the variables days etc.
wouldn't be global variables:
(functiion () {
       var days = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat'],
            today = new Date(),
            message = 'Today is ' + days[today.getDay()] + ', ' + today.getDate();
       alert(message);
}());

9.
function main(links) {
    for (var i = 0, len = links.length; ++i <  len;) {
       links[i].onclick = function() {
          alert(i+1);
      };
   }
}
main(document.getElementsByTagName("a"));
-----------------------------------------
This is a common error for Closure design. The function is bound to
the variable i not its value.
Code should be:
function main(links) {
   for (var i = 0, len = links.length; ++i < len;) {
       links[i].onclick = (function(i) {
           return function() {
                 alert(i+1);
           }
       })(i);
   }
}
main(document.getElementsByTagName("a"));
----------------------------------
Another example is:
function test() {
   var arr = [];
   for (var i = 0; ++i < 0;) {
        var v = i;
        arr[i] = function() {
            return v;
        };
    }
    for (var i = 0; ++i < 0;) {
       alert(arr[i]());
   }
}
Code should be:
function test() {
   var arr = [];
   for (var i = 0; ++i < 0;) {        
        arr[i] = function() {
            var v = i;
            return v;
        };
    }
    for (var i = 0; ++i < 0;) {
       alert(arr[i]());
   }
}

10.
function Screen() {
    // private member
   var specs = {
       screen_width: 320,
       screen_height: 480
   };
   // public function
   this.getSpecs = function () {
       return specs;
   };
}
-------------------------------
The problem is the specs can be changed:
  var scrn = new Screen(),
     specs = scrn.getSpecs();
     specs.screen_width = 300;
Should use approach  like extend() function to fix this problem.
Similar case happens in Java, see "IncorrectJava.txt". 

11.
for (var i = 0; ++i < 100;) {
    document.getElementById("result").innerHTML += i + ", ";
}
------------------------------------------------
Better code is 
var content = "";
for (var i = 0; ++i < 100;) {
    content += i + ", ";
}
document.getElementById("result").innerHTML += content;
because DOM access is expensive we should reduce it to minimum.
In jQuery, there is similar problem, see book "jQuery Cookbook" p. 105 and p. 117

12.
getCustomerProfile() {
 // do somthing
}
getCustomerProfile(id, type) {
  // do other thing
}
getCustomerProfile();
-----------------------------------------------
Above getCustomerProfile(); will invoke getCustomerProfile();
but if getCustomerProfile() is missing, it will invoke getCustomerProfile(id, type)
with id and type as undefined arguments.
Better practice is to use different function name for a function which has different function and arguments.

13.
function init() {
     with (document) {
          var  bd = body,
              links = getElementsByTagName("a"),
              i = 0,
              len = links.length;
       while (i < len) {
          //
       }
         //
    }
}
-------------------------------------
Should avoid using "with" because it didn't improve performance. Instead, use the following code:
function init() {
      var doc = document,
              bd = doc.body,
              links = doc.getElementsByTagName('a'),
              i = 0,
              len = links.length;
       while (i < len) {
          //
       }
         //
}

14.
var errs = [],
divs = document.getElementsByTagName('div'),
classname = ' ';
for (var i = 0, len = divs.length; ++i < len;) {
    classname = divs[i].classname;
    if (classname === 'notice' || classname === 'warning') {
         errs.push(divs[i]);
    }
}
--------------------------------------------------------
Better to use querySelectorAll() instead:
var errs = document.querySelectorAll('div.warning, div.notice');

15.
var el = document.getElementsById('mydiv');
el.style.borderLeft = '1px';
el.style.borderRight = '2px';
el.style.padding = '5px';
------------------------------------------
Better to code:
var el = document.getElementsById('mydiv');
el.style.cssText = 'border-left: 1px; border-right : 2px; padding : 5px;';
Or :
var el = document.getElementsById('mydiv');
el.classname = 'active';

16.
/cat|bat/.test(str);
endsWithComma = /,$/.test(str);
----------------------------------------
In Regular Expression, character classes are faster than alternation.
So use /[cb]at/ instead of /cat|bat/, use /rea?d/ instead of /red|read/, 
use /r(?:ed|aw)/ instead of /red|raw/, use /[sS]/ instead of /(.|t|n)/, etc.
Usually regexes are faster, but in above code, what the rexes end up 
doing is stepping through the entire string. Each time a comma is found,
the regexes have to check the token ($) and take longer time. Better to code:
endsWithComma = str.charAt(str.length - 1) == ",";   

17.
var myObject = new Object();
myObject.name = "Ming";
...
----------------------------------------------
Use literals when you create object and array because literals are evaluated and
executed faster.  Better to code:
var myObject = {
    name : "Ming",
    ...
};

18.
var obj1 = {
    yell: function(n){
    return n > 0 ? obj1.yell(n-1) + "a" : "hi";
  }
};
var obj2 = { yell: obj1.yell };
var obj1 = {};
obj2.yell(4); // it doesn't work
------------------------------------------------------------
Should name the yell anonymous:
var obj1 = {
    yell: function yell(n){
    return n > 0 ? obj1.yell(n-1) + "a" : "hi";
  }
};
var obj2 = { yell: obj1.yell };
var obj1 = {};
obj2.yell(4); // it works

19.
var myFun = function myOther(){
  // do something
};
myFun ();
myOther();  //  it doesn't work; myOther can only be used inside function.
--------------------------------------
Better to code:
var myFun = function myFun(){
  // do something
};

20.
Number.prototype.add = function(num){
   return this + num;
};
5.add(3); //  it doesn't work
-------------------------------------------
Should code:
var n = 5;
n.add(3) ; // it works
Or:
(5).add(3); // it works also.

21.
if (String.prototype.trim) {
  function trim(str) {
    return str.trim();
 }
} else {
  function trim(str) {
   return str.replace(/^s+|s+$/g, "");
 }
}
----------------------------------------
Functiion declarations are not allowed inside blocks, e.g., an if-else statement.
However, some browers will run this script.  But when this happens, we always end up 
with the second implementation due to function hoisting.  Should code like:
var trim;
if (String.prototype.trim) {
  trim = function (str) {
    return str.trim();
 }
} else {
  trim = function (str) {
   return str.replace(/^s+|s+$/g, "");
 }
}
Or:
if (!String.prototype.trim) {
  String.prototype.trim = function () {
   return this.replace(/^s+|s+$/g, "");
 }
}

22.
var circle = {
   radius: 6,
   diameter: function() {
       return this.radius * 2;
   }
};
alert(circle.diameter());  // it works fine
var myDiameter = circle.diameter;
alert(myDiameter());
radius = 2;
alert(myDiameter());
------------------------------------
myDiameter() will implicitly bind this to the global object
Using radius = 2; myDiameter() will rely on implicit globals.  
Similar error is like:
function Circle(radius) {
  this.radius = radius;
}
var circle = Circle(6); // calling the constructor(without new) as a function; circle is undefined.
Another example is:
new function() {
   this.appName = "myApp";
   document.body.addEventListener("click", function(e) {
       alert(this.appName);  // appName will be undefined.   May use jQuery .proxy() to fix it.
   }, false);
};
In jQuery, there is similar problem on "this", see book "jQuery Cookbook" p. 88

23.
var lightbox = {
  open: function() {
     ajax.loadFragment(this.url, {
       target: this.create()
    })
     return false;
  },
  create: function() {
    // ...
  }
  //  ...
};
function anchorLightbox(anchor, options) {
   var lb = Object.create(lightbox);
   lb.url = anchor.href;
   Object.extend(lb, options);
   anchor.onclick = lb.open;
   return lb;
}
---------------------------------------- 
When the link(anchor.onclick) is clicked, the expected behavior fails.
The reason is that when we assign the lb.open method, we lose the implicit
binding of this to the lb object.  The code changes to be:
  anchor.onclick = function() {
      return lb.open();
  };
it will work fine.

24.
function Circle(radius) {
  this.radius = radius;
}
Circle.prototype = {
  diameter: function () {
    return this.radius * 2;
  },
  circumference: function () {
    return this.diameter() * Math.PI;
  ),
  //  ...
};
var circle = new Circle(6); 
var circle1 = new circle.constructor(9); // it doesn't create a new object of the same type
----------------------------------------
Providing an object literal to the prototype (avoid repeating Circle.prototype)
won't create a constructor property.  Fix it by assigning the constructor property manually:
Circle.prototype = {
  constructor: Circle,
  // ...
};

25.
$(document).ready(functiion() {
   var button = document.getElementById('button');
   button.onclick = function() {
       $.print('hello');
       return false;
  };
});
---------------------------------------
The click handler creates a closure with button. However, button now contains a reference 
back to the closure--the onclick property itself.  Thus, it causes a reference loop which cannot
be released by some version of IE (memory leak).  The code should be:
function hello {
   $.print('hello');
       return false;
}
$(document).ready(functiion() {
   var button = document.getElementById('button');
   button.onclick = hello;
});

 

登录后才可评论.