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.

Hello to everyone!

First of all, i have to thank everyone who contributes to this great site. It has been a source of many tipps for me while starting to code in javascript!

I've read the FAQs and perhaps this isn't the real spot for posting my support question. So if some of the experienced users want to close this question, they are probably right. But I'll give it a try anyway. As a background: I've just started to learn javascript and html with some online tutorials and a lot of searching the web. But I'm still a the very beginning. None of my friends is interested in coding - so there's is no "real person" i can ask for support.

I try to write a little page which will help me - when it ever gets finished - in my job to get some stuff done easier and faster. But before I have to rewrite everything I would really appreciate it, if someone with experience in coding websites could give me some hints and tricks for better structuring the code, i.e. to better divide logic from ui.

For this reason I paste an example of what i already did. And if someone with some spare time will give me a few hints, it would be great!

HTML File:

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>Testpage</title>
</head>
<body>
    <div id="form-container">
        <h2>Step 1</h2>
        <form name="step01">
            <fieldset>
                <p>
                    <label for="selectScanner">Choose a scanner</label>
                    <select id="selectScanner">
                        <optgroup label="Group 1">
                            <option value="scanner1">Test Scanner 1</option>
                            <option value="scanner2">Test Scanner 2</option>
                        </optgroup>
                        <optgroup label="Group 2">
                            <option value="scanner3">Test Scanner 3</option>
                            <option value="scanner4">Test Scanner 4</option>
                        </optgroup>
                    </select>
                </p>
                <p>
                    <label for="selectProtocol">Choose a protocol</label>
                    <select id="selectProtocol">
                    </select>
                </p>
            </fieldset>
        </form>
        <h2>Step 2</h2>
        <form name="step02">
            <fieldset>
                <p>
                    <label for="dateOfExam">Date Of Examination</label>
                    <input type="date" id="dateOfExam" />
                </p>
                <p>
                    <label for="dateOfBirth">Date Of Birth</label>
                    <input type="date" id="dateOfBirth" />
                </p>
                <p>Age: <span id="age"></span>
                </p>
            </fieldset>
            <fieldset>
                <legend>Choose sex</legend>
                <input type="radio" name="sex" value="male">Male<br>
                <input type="radio" name="sex" value="female">Female</fieldset>
                <fieldset>
                    <legend>Height [cm] and weight [kg]</legend><input type="number" id="height" name"height" placeholder="cm" >
                    <input type="number" id="weight" name="weight" placeholder="kg" >
                    <p>BSA (Mosteller): <span id="bsa"> </span></p>
                    <p>BMI: <span id="bmi"> </span></p>
                </fieldset>
            </form>
        </div>
        <script>
        window.jQuery || document.write('<script src="js/jquery-1.8.2.min.js"><\/script>')
        </script>
        <script src="js/main.js"></script>
    </body>
    </html>

I use jQuery a bit (need to expand my knowledge of using it) and will finally probably use twitter bootstrap as a framework.

Here comes the main.js

/*
* This section is about the "select" input elements
* 
* This should realize a cascading dropdown, which
* shows options in the second select input elemen
* "protocols" depending on the selection of a scanner
*
* Found this solution on the web an adopted it ab bit
* to my needs.
*/

function appendOptionToSelect(sel, opt) {
    try {
        sel.add(opt, null);
    } catch (e) {
        //for IE7 and earlier
        if (e.name == "TypeError") {
            sel.options[sel.options.length] = opt;
        } else {
            throw e;
        }
    }
};

function removeAllChildNodes(element) {
    if (element.hasChildNodes()) {
        while (element.childNodes.length >= 1) {
            element.removeChild(element.firstChild);
        };
    };
};

function selChanged(sel, data, dependentSel) {
    var selection = sel.options[sel.selectedIndex].value;
    var arrOptions = data[selection];
    var opt;
    removeAllChildNodes(dependentSel);
    for (var i in arrOptions) {
        opt = new Option(arrOptions[i]);
        appendOptionToSelect(dependentSel, opt);
}
};






// This should be the object which holds the protocols specific
// for a scanner
var scanner = {
    "scanner1" : ["--choose--", "protocol 1", "protocol 2", "protocol 3"],
    "scanner2" : ["--choose--", "protocol 2", "protocol 4", "protocol 5"],
    "scanner3" : ["--choose--", "protocol 1", "protocol 2", "protocol 5", "protocol 6"],
"scanner4" : ["--choose--", "protocol 3a", "protocol 4b", "protocol xyz"]
};

// Needed down in the eventhandlers
var selectScanner = document.getElementById("selectScanner");
var selectProtocol = document.getElementById("selectProtocol");



// Calculation the age in years (found this on the web)
function getAge() {
    var dateOfExam = new Date(document.getElementById('dateOfExam').value);
    var dateOfBirth = new Date(document.getElementById('dateOfBirth').value);

function isLeap(year) {
    return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
}

// age in days
var days = Math.floor((dateOfExam.getTime() - dateOfBirth.getTime()) / 1000 / 60 / 60 / 24);
var age = 0;
// calculating years
for (var y = dateOfBirth.getFullYear(); y <= dateOfExam.getFullYear(); y++) {
    var daysInYear = isLeap(y) ? 366 : 365;
    if (days >= daysInYear) {
        days -= daysInYear;
        age++;
        // just increment the year if there are enough days for a year
    }
}
return age;
}






/* Bodysurface and BMI
* 
* Ok, now it gets chaotic, this is my solution.
* The aim is, to calculate both BSA and BMI depending on the input-values from
* the elements "weight" and "height".
* I need to get
* - a string like (for BSA): 2,3 m2
* - a string like (for BMI): 25,5
* - global(?) variables which stores the floats for bmi and bsa
* I guess my way is weird...
*/

var bsa, bmi, kof;

function Kof(cm, kg) {
    var mosteller = Math.sqrt(Number(cm) * Number(kg) / 3600);
    var bmi = Number(kg) / Math.pow((Number(cm) / 100), 2);
    this.getStringMosteller = function() {
        return mosteller.toFixed(1).replace(/\./, ",") + " m<sup>2</sup>";
    };
    this.getStringBmi = function() {
        return bmi.toFixed(1).replace(/\./, ",");
    };
    this.getMosteller = function() {
        return mosteller;
    };
    this.getBmi = function() {
        return bmi;
    };
}


function setBmiAndBsa() {
    var cm = parseFloat($('#height').val());
    var kg = parseFloat($('#weight').val());
    if (!isNaN(cm) && !isNaN(kg)) {     // checking that there are numbers
        kof = new Kof(cm, kg);      
        bsa = kof.getMosteller();       // getting the floats for bsa
        bmi = kof.getBmi();             // and bmi

    // and writing the string values to the html-page
    $('#bsa').html(kof.getStringMosteller());
    $('#bmi').html(kof.getStringBmi());
};
}



$(document).ready(function() {

    // set the current date as the date of examination
    $('#dateOfExam').val(new Date().toJSON().substring(0, 10));

    // calculate the age when a date of birth is entered
    $('#dateOfBirth').change(function() {
        $('#age').html(getAge());       // and writing it to the page
    });


    // The eventhandlers for height and weight
    $('#height').change(function() {
        setBmiAndBsa();
    });
    $('#weight').change(function() {
        setBmiAndBsa();
    });
    /*
     * Why the heck aren't these handlers working?
     *
     * $('#selectScanner').change(function() {
     * selChanged(selectScanner, scanner, selectProtocol);
     * });
     * $('#selectProtocol').change(function() {
     * alert(selectProtocol.options[selectProtocol.selectedIndex].value);
     * });
     * 
     * I have to write them like this:
     */
});
selectScanner.onchange = function() {
    selChanged(selectScanner, scanner, selectProtocol);
};
selectProtocol.onchange = function() {
    alert(selectProtocol.options[selectProtocol.selectedIndex].value);
};

And to everyone who reads up until here: thumbs up & thank you for even considering helping me out!!!

Phil

share|improve this question
    
If you're still looking for an answer, can you say whether your code works as intended or not? Are there any specific problems with it, other than the one about jQuery event handlers not working? –  Stuart Dec 21 '12 at 23:54
1  
Maybe you could also say what your motivation is for making the object Kof instead of just, say, having functions getBMI(cm, kg) and getBSA(cm, kg), and why you are storing bmi and bsa as global variables. –  Stuart Dec 22 '12 at 0:09
    
Hello Stuart. Thank you for finding, viewing and answering to this question. Of course I'm still interested in any hints how to write better scripts. To answer your questions: The code works like intended, but it is just a snippet out of the project i'm trying to code. About my motivation for making the object Kof: I thought it is a possibility to structure the code a bit more, and it's a component i need on multiple pages. Actually i'm trying to develop a "patient" or "human" object with a lot of attributes and methods, but I tried to start simple. –  Phil Dec 23 '12 at 16:20
    
I used bmi and bsa as global variables because i wanted to separate some logic (mainly generating reports). And that logic (partially in the eventhandlers - i dislike it that way and have to change it) needs to get these values. But while reading more about scripting i just reached the chapter about self invoking anonymous functions - surely I should start using them in some parts. By the way: I changed the structure of the site, so no more selecting scanners and protocols is necessary... Thanks again - Phil. –  Phil Dec 23 '12 at 16:20
1  
Okay. Then I have nothing to say about the code, it looks fine to me. I don't see anything 'weird' about it. If Kof becomes a person/patient object you might want to start recording the weight and height as properties of the object this.cm = cm; this.kg = kg. I would not bother having the functions getMosteller and getBMI until/unless you need them. You can write $('#height').change(setBMIandBSA) instead of $('#weight').change(function() { setBmiAndBsa();}) –  Stuart Dec 23 '12 at 19:53

1 Answer 1

This is only about your HTML as I don't know much about JS. I guess your page will only be used internally by you, so it is not that important, but it may help others that come by.

  • legend must only be used in fieldset
  • label can only include "phrasing content", so h6 is not allowed there (in general I wouldn't use headings inside of the form at all)
  • you should "connect" the label and it's input/select: with the for attribute
  • omit <option>choose</option> as it is (probably) no valid choice
  • group the two gender radio boxes in a fieldset with a legend like "Select gender"
    • same with "Height [cm] and weight [kg]" (fieldset>legend instead of h6)
share|improve this answer
    
Thanks for your hints @unor, I'll try them out asap. –  Phil Nov 21 '12 at 9:59
    
Tried to fix the points you mentioned above. Thank you again for your help! –  Phil Nov 21 '12 at 15:31
    
Great. I don't think that my answer deserves the green checkmark, because I didn't review your JS. Feel free to give it to someone that might do that in the future. –  unor Nov 21 '12 at 17:00
    
I'm a complete newbie regarding posting on stackexchange. I thought it might be a kind of saying: your answer helped me. But I'm going to take a look at the vote / check system of stackexchange. Thanks for the hint. –  Phil Nov 21 '12 at 17:26
    
@Phil: It also means that you probably don't need any more help, because the checked answer solved all your questions. So it is often a good idea to wait some time for more answers until you accept an answer. –  unor Nov 22 '12 at 11:05

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.