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

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

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.

share|improve this answer
1  
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 at 12:15
    
I would even use requestAnimationFrame instead of setInterval, since it’s used for animations and not for I/O. –  rightfold Jun 7 at 13:57
add comment

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++) {
share|improve this answer
add comment

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>");
    

    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

share|improve this answer
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.