Snowfall in HTML

Question

I am a beginner and I have made Snowfall in HTML for my mom. I'm pretty sure it will not look that awesome 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 HTML.

<h1>Happy Winter!</h1><script>

---

var snowmax=35
var snowcolor=new Array("#aaaacc","#ddddFF","#ccccDD")
var snowtype=new Array("Arial Black","Arial Narrow","Times","Comic Sans MS")
var snowletter="*"
var sinkspeed=0.6
var snowmaxsize=22
var snowminsize=8
var snowingzone=3
var snow=new Array()
var marginbottom
var marginright
var timer
var i_snow=0
var x_mv=new Array();
var crds=new Array();
var lftrght=new Array();
var browserinfos=navigator.userAgent 
var ie5=document.all&&document.getElementById&&!browserinfos.match(/Opera/)
var ns6=document.getElementById&&!document.all
var opera=browserinfos.match(/Opera/) 
var browserok=ie5||ns6||opera
function randommaker(range) { 
 rand=Math.floor(range*Math.random())
 return rand
}
function initsnow() {
 if (ie5 || opera) {
 marginbottom = document.body.clientHeight
 marginright = document.body.clientWidth
 }
 else if (ns6) {
 marginbottom = window.innerHeight
 marginright = window.innerWidth
 }
 var snowsizerange=snowmaxsize-snowminsize
 for (i=0;i<=snowmax;i++) {
 crds[i] = 0; 
 lftrght[i] = Math.random()*15; 
 x_mv[i] = 0.03 + Math.random()/10;
 snow[i]=document.getElementById("s"+i)
 snow[i].style.fontFamily=snowtype[randommaker(snowtype.length)]
 snow[i].size=randommaker(snowsizerange)+snowminsize
 snow[i].style.fontSize=snow[i].size
 snow[i].style.color=snowcolor[randommaker(snowcolor.length)]
 snow[i].sink=sinkspeed*snow[i].size/5
 if (snowingzone==1) {snow[i].posx=randommaker(marginright-snow[i].size)}
 if (snowingzone==2) {snow[i].posx=randommaker(marginright/2-snow[i].size)}
 if (snowingzone==3) {snow[i].posx=randommaker(marginright/2-snow[i].size)+marginright/4}
 if (snowingzone==4) {snow[i].posx=randommaker(marginright/2-snow[i].size)+marginright/2}
 snow[i].posy=randommaker(2*marginbottom-marginbottom-2*snow[i].size)
 snow[i].style.left=snow[i].posx
 snow[i].style.top=snow[i].posy
 }
 movesnow()
}
function movesnow() {
 for (i=0;i<=snowmax;i++) {
 crds[i] += x_mv[i];
 snow[i].posy+=snow[i].sink
 snow[i].style.left=snow[i].posx+lftrght[i]*Math.sin(crds[i]);
 snow[i].style.top=snow[i].posy
 if (snow[i].posy>=marginbottom-2*snow[i].size || parseInt(snow[i].style.left)>(marginright-3*lftrght[i])){
 if (snowingzone==1) {snow[i].posx=randommaker(marginright-snow[i].size)}
 if (snowingzone==2) {snow[i].posx=randommaker(marginright/2-snow[i].size)}
 if (snowingzone==3) {snow[i].posx=randommaker(marginright/2-snow[i].size)+marginright/4}
 if (snowingzone==4) {snow[i].posx=randommaker(marginright/2-snow[i].size)+marginright/2}
 snow[i].posy=0
 }
 }
 var timer=setTimeout("movesnow()",50)
}
for (i=0;i<=snowmax;i++) {
 document.write("<span id='s"+i+"' style='position:absolute;top:-"+snowmaxsize+"'>"+snowletter+"</span>")
}
if (browserok) {
 window.onload=initsnow
}

• 200_success: I hope your mom isn't using IE 5/6.
• Ducky: She's using FF Latest Version
• boxed__l: @Ducky Use ❄ for snowflakes
• Bergi: var browserok = !!document.getElementById; is equivalent to your browser detection thingy
• Madara's Ghost: Gah! document.write()! MY EYES! MY EYES!!!

Answer 1

Some suggestions:

1. Use more whitespaces and semicolons

2. Initialise Arrays with []

   var snowcolor = ["#aaaacc", "#ddddFF", "#ccccDD"];
   

3. Use naming conventions for variables and method names to make them more readable

   initSnow() or init_snow()
   browserInfos or browser_infos
   

4. In JavaScript it is recommended to combine several variable declerations. So instead of

   var marginbottom
   var marginright
   var timer
   

   you can write

   var marginbottom,
    marginright,
    timer;
   

5. Instead of passing a string to setTimeout, you can pass a reference to the function directly

   var timer = setTimeout(movesnow, 50);
   

6. There are some magic numbers in your code, that you might want to extract into variables.

7. Since JavaScript doesn't have final static variables (afaik), consider marking them via naming conventions. For example:

   var SNOW_MAX_SIZE = 22;
   

8. The four if statements inside initsnow and movesnow seem to be the same, so you could move them to a seperate function. However, you currently have snowingzone set to a fixed value and you don't change it. So as it is, the if statements are unnecessary.

9. Some variable names are rather cryptic: x_mv, lftrght, etc.

10. There are unused variables: e.g. i_snow

11. The browser identification and the "calculation" of browserok seem to be good candidates for a seperate function.

---

Update Added 8. - 11.

If you are feeling confident or want to try static code analysis, you might want to look into something like http://www.jslint.com/ or http://www.jshint.com/ . They can warn you about global variables, unused variables and more.

ad 4) As Schism mentioned, this advices may lead to unwanted global variables if you mix up , and ;. So for example:

function foo() {
 var a,
 b; // Oops! ; should be ,
 c; // c is now a global variable
}

• Schism: Re: 4. It's not always a good idea to do this. You may, for instance, find that you might accidentally use a semicolon instead of a comma; in that case, you'd be unintentionally polluting global scope.
• HC_: @schism can you elaborate on this? I'm still trying to learn all the nuances of js variable scope when declaring with var versus without -- first time hearing about commas vs ;'s having an affect as well.
• Ryan: @HC_ it's not that a ; effects global space and a , doesn't. A semicolon in javascript marks the end of a statement and any code following it is considered to be a new statement. So if you accidentally use a semicolon you now have code that looks like this: var foo; bar, baz; which implicitly declares bar, baz as variables in global scope, because they are now their own statement separated from the var keyword. Schism was just pointing out that its an easy mistake to make and a quick way to gum up global space.
• Syjin: @Schism you are right, added a short example to the answer
• STEEL: also, please dont use document.write. instead you could use body.appendChild. Also, parseInt radix is missing.

Answer 2

Instead of retriggering setTimeout() on every call to movesnow(), I suggest calling setInterval() just once for the entire program.

You have some duplicated code between initsnow() and movesnow() (the snowingzone switch) that should be factored out. It appears that snowingzone is hard-coded to 3, though, so I'm not sure what your real intention is.

It's considered good practice to end every statement with an explicit semicolon, even though JavaScript doesn't require it.

I suggest renaming the snow array to flakes, and possibly renaming a few other variables in the same way. I'm not sure what the crds variable stands for — the abbreviation is too cryptic for me.

• and31415: Judging by the code, I believe crds stands for coordinates. But I do agree it's cryptic, and should be renamed for better clarity.
• user36: I would even use requestAnimationFrame instead of setInterval, since it’s used for animations and not for I/O.

Answer 3

I haven't done any web dev in a very long time, so I'll leave your actual question to the experts. I just want to point out a small style point. Your variable assignments could use some breathing space.

var crds = new Array();

Same thing in your for loops.

for (i = 0; i <= snowmax; i++) {

Answer 4

In addition to the answers above (with which I agree wholeheartedly), two bits from me:

1. Use a switch statement and/or smart code factoring, so that

   if (snowingzone==1) {snow[i].posx=randommaker(marginright-snow[i].size)}
   if (snowingzone==2) {snow[i].posx=randommaker(marginright/2-snow[i].size)}
   if (snowingzone==3) {snow[i].posx=randommaker(marginright/2-snow[i].size)+marginright/4}
   if (snowingzone==4) {snow[i].posx=randommaker(marginright/2-snow[i].size)+marginright/2}
   

   changes to

   switch( snowingzone ) {
    case 1:
    snow[i].posx = randommaker( marginright - snow[i].size ); break;
    case 2:
    snow[i].posx = randommaker( marginright / 2-snow[i].size ); break;
    case 3:
    snow[i].posx = randommaker( marginright / 2-snow[i].size ) + marginright / 4; break;
    case 4:
    snow[i].posx = randommaker( marginright / 2-snow[i].size ) + marginright / 2; // break; is optional here, you may use it for clarity
   }
   

   or, even more tersely, to

   snow[i].posx = randommaker( marginright / ( snowingzone == 1 ) ? 1 : 2 - snow[i].size);
   if ( snowingzone == 3 )
    snow[i].posx += marginright / 4;
   else if ( snowingzone == 4 )
    snow[i].posx += marginright / 2;
   

   https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch

2. Don't create DOM elements with:

   document.write("<span id='s"+i+"' style='position:absolute;top:-"+snowmaxsize+"'>"+snowletter+"</span>");
   

   Instead you should use:

   snow[i] = document.createElement("span"); // etc
   

   This allows you to create the array of flakes directly and entirely skip calling snow[i]=document.getElementById("s"+i).

   https://developer.mozilla.org/en-US/docs/Web/API/document.createElement