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

function movesnow() {
    for (i=0;i<=snowmax;i++) {
        crds[i] += x_mv[i];

        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}
    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) {
  • 13
    \$\begingroup\$ I hope your mom isn't using IE 5/6. \$\endgroup\$ Commented Jun 6, 2014 at 9:44
  • 7
    \$\begingroup\$ She's using FF Latest Version \$\endgroup\$
    – Ducky
    Commented Jun 6, 2014 at 9:46
  • 8
    \$\begingroup\$ @Ducky Use &#10052; for snowflakes \$\endgroup\$
    – boxed__l
    Commented Jun 6, 2014 at 12:04
  • \$\begingroup\$ var browserok = !!document.getElementById; is equivalent to your browser detection thingy \$\endgroup\$
    – Bergi
    Commented Jun 7, 2014 at 15:19
  • 8
    \$\begingroup\$ Gah! document.write()! MY EYES! MY EYES!!! \$\endgroup\$ Commented Jun 8, 2014 at 13:30

4 Answers 4


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,
  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
  • 5
    \$\begingroup\$ 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. \$\endgroup\$
    – Schism
    Commented Jun 6, 2014 at 13:37
  • \$\begingroup\$ @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. \$\endgroup\$
    – HC_
    Commented Jun 6, 2014 at 17:44
  • 2
    \$\begingroup\$ @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. \$\endgroup\$
    – Ryan
    Commented Jun 6, 2014 at 18:14
  • \$\begingroup\$ @Schism you are right, added a short example to the answer \$\endgroup\$
    – Syjin
    Commented Jun 6, 2014 at 19:39
  • \$\begingroup\$ also, please dont use document.write. instead you could use body.appendChild. Also, parseInt radix is missing. \$\endgroup\$
    – STEEL
    Commented Jun 17, 2014 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.

  • 2
    \$\begingroup\$ Judging by the code, I believe crds stands for coordinates. But I do agree it's cryptic, and should be renamed for better clarity. \$\endgroup\$
    – and31415
    Commented Jun 6, 2014 at 12:15
  • \$\begingroup\$ I would even use requestAnimationFrame instead of setInterval, since it’s used for animations and not for I/O. \$\endgroup\$
    – user36
    Commented Jun 7, 2014 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;


  2. Don't create DOM elements with:

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



Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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