Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a beginner and I have made Colorful Lights using check-boxes in HTML. I'm pretty sure it will look horrible to any developer out there, but hey, that's why I've posted it.

I'd like a general review of this. I'm especially concerned about the quality and enhancements of this form.

<html> 
<head> 
<title> 
Javascript 
</title> 
</head> 
<body topmargin=15><p align=center>
<script> 
var time = 0 
for (var num1 = 0; num1 <= 32; num1+=2) 
{ 
for (var num2 = 0; num2 <= num1; num2++) 
{ 
// if (num1 >= 26 && num2 == (10 || 11) ; else 
randomnumber=Math.floor(Math.random()*2323232) 
document.write('<INPUT type="checkbox" onmouseover="document.bgColor='+"'"+randomnumber

+"'"+'">') 
} 
num2 = 0 
document.write("<br>") 
} 
for (var num5 = 30; num5 >= 0; num5-=2) 
{ 
for (var num6 = 0; num6 <= num5; num6++) 
{ 
randomnumber=Math.floor(Math.random()*2323232) 
document.write('<INPUT type="checkbox" onmouseover="document.bgColor='+"'"+randomnumber

+"'"+'">') 
} 
num6 = 0 
document.write("<br>") 
} 

</script> 
</p> 
</body> 
</html>
share|improve this question
    
Why do you use checkboxes for this? Any element can have mouseover events, and using checkboxes when you don't seem to use them as, well, checkboxes seems odd –  Flambino Mar 20 at 6:02
add comment

1 Answer 1

up vote 10 down vote accepted

1) I'd start with a better HTML template, you forgot the DOCTYPE and meta tags:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>

</body>
</html>

2) Don't use topalign and align HTML attributes. Keep the styles and layout separate, use CSS:

body {
  margin: 15px 0;
  text-align: center;
}

3) In JavaScript it is ussually preferred to put braces on the same line, because the ASI (automatic semicolon insertion) feature can be problematic, consider this buggy example:

return
{
  foo: 'foo'
}

JavaScript will do this:

return; //<--!!!!
{
  foo: 'foo'
}

See the semicolon it added? Now you return undefined, and not an object.

Now, you forgot a bunch of semicolons. Again, ASI can fill them up for you, but I don't recommend you rely on it if you're a beginner, so my advice is always put the semicolons.

5) Check your code in JSHint, this is what I got:

13 warnings
1   Missing semicolon.
5   Missing semicolon.
6   document.write can be a form of eval.
6   Missing semicolon.
8   Missing semicolon.
9   document.write can be a form of eval.
9   Missing semicolon.
13  Missing semicolon.
14  document.write can be a form of eval.
14  Missing semicolon.
16  Missing semicolon.
17  document.write can be a form of eval.
17  Missing semicolon.
One undefined variable
5   randomnumber
6   randomnumber
13  randomnumber
14  randomnumber
One unused variable
1   time

Go one by one, try to fix all those warnings.

6) Avoid document.write, see Why is document.write considered bad practice. In other words, use the DOM.

7) Inline events are bad practice, again, use the DOM, with addEventListener.

8) Using the DOM can be expensive. Try to always add elements all at once. Document fragments help you improve performance, because you don't mess with the DOM on every iteration of the loop.

9) Finally, separate concerns; the single responsibility idea. This is how I would write it taking all those points into consideration:

// Use the DOM to create elements
function makeInput() {
  var input = document.createElement('input');
  input.type = 'checkbox';
  return input;
}

function append(ele, parent) {  
  (parent||document.body).appendChild(ele);
}

function makeLights() {
  var i, j;
  var br = document.createElement('br');

  // Use a document frag to improve performance
  var frag = document.createDocumentFragment();

  // No need to reset "j" on every iteration
  for (i=0; i<=32; i+=2) {
    for (j=0; j<=i; j++) append(makeInput(), frag);
    append(br.cloneNode(), frag);
  }

  for (i=30; i>=0; i-=2) {
    for (j=0; j<=i; j++) append(makeInput(), frag);
    append(br.cloneNode(), frag);
  }

  // Append to document all at once
  append(frag);  
}

function randomColor() {
  return Math.floor(Math.random() * 2323232);
}

// Use event delegation for best performance
document.body.addEventListener('mouseover', function(e) {
  if (e.target.tagName == 'INPUT') {
     document.bgColor = randomColor();
  }
});

makeLights(); // init

Demo: http://jsbin.com/yiyic/1

share|improve this answer
2  
Nice answer. But why the j = 0; after each loop? They're not necessary –  Flambino Mar 20 at 6:09
    
Good point, I edited the answer, I just copy/pasted from OPs code. –  elclanrs Mar 20 at 6:11
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.