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
}

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
}

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.

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