10
\$\begingroup\$

To calculate torque I need to look up a value from a table which I've hardcoded based on three conditions: grade, size, and thread. Then I need to multiply it by coefficients associated with the selected lubricants.

I've written the code to be a web-app, but it will also just run as an html file sitting on a company drive, not connected to the internet. Because it's offline, I needed to hardcode the values instead of look them up from a data file because of same-origin policy and JavaScript security settings. It's not ideal style, but I couldn't think of an alternative.

It works, but I want to make sure it's really robust and easy for anyone to maintain after my internship is done. It's one of the first programs I've written in this language and I don't get easily offended, so please be thorough! I'm not sure what's considered good style or efficient. Also, this needs to be run on IE8 so some things I've done have reflected the limitations of that browser.

JSFiddle

JavaScript

    var size;
        var grade;
        var thread;
        var value;

        document.getElementById("message").innerHTML = "Select grade.";

        function fetch_grade_sizes() {


            // get grade from select menu
            var grade_menu = document.getElementById("grade_menu");
            grade = grade_menu.options[grade_menu.selectedIndex].value;

            // get thread from radio buttons
            var radios = document.getElementsByName("radio");
            for (var i = 0, length = radios.length; i < length; i++) {
                if (radios[i].checked) {
                    thread = radios[i].value;
                    break;
                }
            }

            // tell user to select any missing values       
            if( grade === undefined || grade == ""){
                document.getElementById("message").innerHTML = "Select grade.";
                return;         
            }
            else if ( thread === undefined || thread === null){
                document.getElementById("message").innerHTML = "Select thread.";
                return; 
            }
            else 
                document.getElementById("message").innerHTML = "Select size.";

            // get the sizes corresponding to that grade and thread
            var size_keys = [];
            for (var key in torque_table[grade][thread]) {
                size_keys.push(key);
            }

            // populate the drop down selet menu with those sizes
            create_menu("size_menu_location", "size_menu", size_keys);
        }


        function calculate(form_data) {

            document.getElementById("message").innerHTML = "Select lubricant.";

            // get selected size
            var size_menu = document.getElementById("size_menu");
            size = size_menu.options[size_menu.selectedIndex].value;

            // if anything hasn't been selected yet, prompt the user        
            if( grade === undefined || grade == ""){
                document.getElementById("message").innerHTML = "Select grade.";
                return;         
            }
            else if ( thread === undefined || thread === null){
                document.getElementById("message").innerHTML = "Select thread.";
                return; 
            }
            else if (size === undefined || size == "" ){
                document.getElementById("message").innerHTML = "Select size.";
                return;
            }

            // look up the torque
            var torque = torque_table[grade][thread][size];

            // multiply each checked value together and
            // create a string of the values separated by multiplication signs
            var check_number = 1;
            var check_string = "";
            for (i = 0; i < form_data.check_menu.length; i++) {
                if (form_data.check_menu[i].checked) {
                    check_string += " &times; " + form_data.check_menu[i].value;
                    check_number *= form_data.check_menu[i].value;

                    // clear the command to select something after a lubricant is selected
                    document.getElementById("message").innerHTML = "<br/>";
                }
            }

            // round the resulting value to one decimal place
            value = Math.round((torque * check_number) * Math.pow(10, 1)) / Math.pow(10, 1);
            var value_in = Math.round((torque * check_number * 12) * Math.pow(10, 1)) / Math.pow(10, 1);
            var value_SI = Math.round((torque * check_number * 1.35581795) * Math.pow(10, 1)) / Math.pow(10, 1);

            // display the multiplication string and the resulting value
            document.getElementById("number").innerHTML = value + " ft-lbs ";
            document.getElementById("string").innerHTML = torque + " ft-lbs " + check_string + " = ";
            document.getElementById("additional_info").innerHTML = "<br>Other units: " + value_in + " in-lbs, " + value_SI + " Nm"; 
        }

        function create_menu(div_id, sel_id, menu_options) {

            var myDiv = document.getElementById(div_id);

            // wipe what's already in the div we're putting this menu
            myDiv.innerHTML = "";

            // create select menu and size
            var selectList = document.createElement("select");
            selectList.setAttribute("id", sel_id);
            selectList.setAttribute("onclick", "calculate(this.form);");
            myDiv.appendChild(selectList);

            // make an unselectable placeholder
            var option = document.createElement("option");
            //option.setAttribute("disabled","true"); 
            option.innerHTML = "Select size";
            option.setAttribute("value", "");
            selectList.appendChild(option);

            // populate the menu with the options from the array
            for (var i = 0; i < menu_options.length; i++) {
                var option = document.createElement("option");
                option.setAttribute("value", menu_options[i]);
                option.innerHTML = menu_options[i];
                selectList.appendChild(option);
            }
        };

    grade_1 = {
            'coarse': {
                    '1/4"': 3.28,
                    '5/16"': 6.75,
                    '3/8"': 12.0,
                    '7/16"': 19.2,
                    '1/2"': 29.3,
                    '9/16"': 42.2,
                    '5/8"': 58.3,
                    '3/4"': 103,
                    '7/8"': 167,
                    '1"': 250,
                    '1-1/8"': 354,
                    '1-1/4"': 500,
                    '1-3/8"': 655,
                    '1-1/2"': 869
            },
                'fine': {
                '1/4"': 3.75,
                    '5/16"': 7.49,
                    '3/8"': 13.6,
                    '7/16"': 21.4,
                    '1/2"': 33.0,
                    '9/16"': 47.1,
                    '5/8"': 66.0,
                    '3/4"': 115,
                    '7/8"': 184,
                    '1"': 273,
                    '1-1/8"': 397,
                    '1-1/4"': 553,
                    '1-3/8"': 746,
                    '1-1/2"': 978
            }
        };

        //grade_5, grade_8 = .... same things

        var torque_table = {
            "grade_1": grade_1,
            "grade_5": grade_5,
            "grade_8": grade_8,
            "stainless": stainless,
            "monel": monel,
            "ASTM_A325": ASTM_A325
        };

HTML

<font size="5"><b>Torque Calculator
</b></font>
<FORM NAME="calculation">
    <br/>
    <select id="grade_menu" onchange="fetch_grade_sizes();">
        <option value="" disabled selected>Select grade</option>
        <option value="grade_1">Grade 1</option>
        <option value="grade_5">Grade 5</option>
        <option value="grade_8">Grade 8</option>
        <option value="ASTM_A325">ASTM A325</option>
        <option value="stainless">316 Stainless</option>
        <option value="monel">Monel</option>

    </select>
    <INPUT TYPE="radio" name="radio" id="radio_menu" onclick="fetch_grade_sizes();" VALUE="coarse" />coarse &nbsp;
    <INPUT TYPE="radio" name="radio" id="radio_menu" onclick="fetch_grade_sizes();" VALUE="fine" />fine
    <br/>
    <div id="size_menu_location">
        <select id="size_menu">
            <option value="" disabled selected>Select size</option>
        </select>
    </div>
    <br><b>Check lubricant used:</b> <br>
    <br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="1.00" />Steel (clean, dry, non-plated)<br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="1.00" />Black oxide<br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.90" />Silver grade anti-seize<br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.80" />WD-40 <br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.75" />Grease with nickel and graphite flakes <br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.70" />Motor oil with cadmium plating <br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.65" />Nickel based anti-seize <br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.45" />Never-seize<br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.80" />243<br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.85" />246<br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="0.95" />248<br/>
    <INPUT TYPE="Checkbox" id="check_menu" onclick="calculate(this.form);" VALUE="1.00" />277

</FORM>
<br/>
<div id="message"></div>
<table>
    <td>
        <div id="string"></div>
    </td>
    <td>
        <div id="number"></div>
    </td>
</table>
<div id="additional_info"></div>

<font size='1'><br>About:<br> 
Calculated by multiplying standard dry torque with a stress of 70% minimum <br>
tensile strength or 75% the proof strength by a coefficient representing <br>
the effect of anti-seize compounds, lubricants, platings, coatings, etc. <br>
For steel, this cooefficient is equal to the torque coefficient (K) times 5.0 <br>

<br>Values are from <i>Pocket Ref</i> by Thomas J. Glover, 4th ed., 2011. pg 213</font>
\$\endgroup\$
  • \$\begingroup\$ I'm a little surprised that I can select multiple types of grease at once. I wouldn't expect to be able to do that as a user. \$\endgroup\$ – RubberDuck Jul 9 '14 at 16:46
  • \$\begingroup\$ Good observation! I actually put a bit of effort into allowing that and creating the string that showed that because before I was multiplying by more than lubricants, and it sort of encompassed a variety of additional factors to consider. You're right that maybe that's no longer necessary. \$\endgroup\$ – Charles Clayton Jul 9 '14 at 16:50
  • \$\begingroup\$ I'm not convinced it's the right thing to do, but so long as it's by design. \$\endgroup\$ – RubberDuck Jul 9 '14 at 16:57
  • \$\begingroup\$ Yeah, makes sense. I think I'll change it to be exclusive. Thanks for the suggestion. \$\endgroup\$ – Charles Clayton Jul 9 '14 at 17:19
  • \$\begingroup\$ This was fun to play with, although I know that your issue with storing data in a separate file could easily be solved with a different language. I'm not sure if you have any experience with languages like Java or say Python, but it'd be a cinch then. I like the code you have though, it's a very good start! \$\endgroup\$ – Alex L Jul 9 '14 at 18:26
5
\$\begingroup\$

A few things caught my eye here.

Firstly, since you need to store all the data in the files (which, I agree, is the best way to go about this, given the constraints) why not store all the data there? By which I mean build the lubricant list and grades in JS too. Or flip it around and keep everything in the HTML (hiding and showing the appropriate dropdowns as needed). Eitherway, I think it'd be better to keep all the source data in one place, rather than split it up. This will make the code a lot more maintainable.

And if you include the "select only one type of lubricant" constraint you've talked about in the comments, everything can be done as single-selection dropdowns, making the work of building the UI simpler. You just need a buildDropdown function.

I'd probably choose to define the values in a separate JS file and load that in the html. Loading a local JS file with a <script src=... is allowed even with same-origin constraints.

Second, a little more separation in the JS would be nice. For instance, the calculate function which does more than just calculate. It also reads the form, validates it, provides feedback to the user etc.. Just pass it the given values when you're ready, and let it perform the math - nothing more. Move rest of the logic elsewhere, breaking it into smaller chunks as you go. This will also make the code more maintainable.

For instance, your validation/feedback code is duplicated in two places, which is a sure sign that it can be extracted.

I'd also avoid calling document.getElementById so many times. For instance, you can just store the "message" element in a variable after you've gotten it once.

And there's no need to set an onclick attribute on an element you've created or fetched from the DOM. onclick is simply a property on that object. So instead of this

selectList.setAttribute("onclick", "calculate(this.form);");

you can just do

selectList.onclick = function () { calculate(this.form) };

Then again, since there's only the one form, it's a little redundant to pass that to the calculate function. Especially right now when calculate does so much other DOM work already. So, if we imagine that calculate (after more or less refactoring) knows to get the form, you can simply do:

selectList.onclick = calculate;

Of course, you should maybe use the more modern addEventListener function instead of directly assigning something to onclick. But for a simple, contained page like this, it's not really necessary.

I'd also advice you to use the usual JS camelCase-naming convention for function etc., and avoid polluting the global scope. But again, for a single-purpose, self-contained page like this, it matters somewhat less than for a big site. Still, it's good hygiene.

Anyway, you've got all the basic building blocks already. Rework the structure a bit, and it should be quite neat! I'll try to work up a basic example of all of this.

FYI, I might choose to use "unrolled" dropdowns (i.e. select elements with a size attribute higher than 1), to make it quicker to drill down the "tree".


Update: Here's a basic attempt at a very different structure. It's not perfect, but hopefully you can glean something from it. I've skipped including the code here, because it's really not relevant, review-wise, since it's so different. I'm not saying you should necessarily go this way, though, but I think it's cleaner.

Basically, what I've done is declare the relationship between dropdowns using a data-* attribute (also used one to declare the validation message). So picking a grade populates the thread list and picking a thread populates the size list, but the code is identical for all the dropdowns, including the lubricants list. In other words, I've tried to the keep the JS mostly generic, and keep the data separated.

A few more things I noticed, going through the code:

  • Don't rely on the order of items in objects. It usually works out, but the order of properties of objects is not guaranteed. So options may not show up in dropdowns in the same order as you defined them.
    I've also used objects, so the same warning applies there.

  • Don't mess around with multiplying and dividing and rounding to get a specific number of decimals. Just use number.toFixed(numberOfDecimals)

\$\endgroup\$
  • \$\begingroup\$ Wow! Thank you so much for all your feedback! I'd be lying if I said I wasn't a little humbled that you built my entire project in less than an hour in your spare time... \$\endgroup\$ – Charles Clayton Jul 9 '14 at 23:24
  • 1
    \$\begingroup\$ @crclayton Heh, no need to be humbled. My code is still pretty rough around the edges. Again, don't think of it as the "right way to do things"; think of it as a rough sketch of a one possible way to do it. Besides, you already did the hard part: Defining what the code has to actually accomplish. So you had the answer worked out, I just took a different route to get there. \$\endgroup\$ – Flambino Jul 9 '14 at 23:48
0
\$\begingroup\$

Turns out I did exactly the same page, with database interaction (php /mysql), but the core idea is exactly the same.

You can see the code here

The main differences are:

  1. I recommend using of jquery. Plain javascript is obsolete, and Jquery is compliant in almost every browser.

  2. As the previous comment says, I would have put all the select values in one place.

  3. Why show the lubricant values in a checkbox? It would be more elegant to have the lubricant in a select, as you did with the rest of the values. You have the option onChange to trigger the calculate function in case the select changes.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I recommend editing the parts of your code relevant to your points into the question, links can break and then the code would be lost. \$\endgroup\$ – SuperBiasedMan Jan 5 '16 at 14:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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