# Snowfall in HTML

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) {
}

-
I hope your mom isn't using IE 5/6. –  200_success Jun 6 '14 at 9:44
She's using FF Latest Version –  Ducky Jun 6 '14 at 9:46
@Ducky Use &#10052; for snowflakes –  boxed__l Jun 6 '14 at 12:04
var browserok = !!document.getElementById; is equivalent to your browser detection thingy –  Bergi Jun 7 '14 at 15:19
Gah! document.write()! MY EYES! MY EYES!!! –  Madara Uchiha Jun 8 '14 at 13:30

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.

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
}

-
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. –  Schism Jun 6 '14 at 13:37
@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. –  HC_ Jun 6 '14 at 17:44
@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. –  ryan Jun 6 '14 at 18:14
@Schism you are right, added a short example to the answer –  Syjin Jun 6 '14 at 19:39
also, please dont use document.write. instead you could use body.appendChild. Also, parseInt radix is missing. –  STEEL Jun 17 '14 at 11:05

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.

-
Judging by the code, I believe crds stands for coordinates. But I do agree it's cryptic, and should be renamed for better clarity. –  and31415 Jun 6 '14 at 12:15
I would even use requestAnimationFrame instead of setInterval, since it’s used for animations and not for I/O. –  рытфолд Jun 7 '14 at 13:57

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++) {

-

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("&lt;span id='s"+i+"' style='position:absolute;top:-"+snowmaxsize+"'>"+snowletter+"&lt;/span>");


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

-