2
\$\begingroup\$

I have been using HTML and CSS for a long time, and just starting to learn jQuery. I created a calculator by myself from research and trial and error. I would like someone to let me know what I'm doing wrong (pertaining to the jQuery) as far as code efficiency, structure, and comments.

Here is the HTML:

<!DOCTYPE html>
<!--[if lt IE 7]>      <html class="no-js lt-ie9 lt-ie8 lt-ie7"> <![endif]-->
<!--[if IE 7]>         <html class="no-js lt-ie9 lt-ie8"> <![endif]-->
<!--[if IE 8]>         <html class="no-js lt-ie9"> <![endif]-->
<!--[if gt IE 8]><!--> <html class="no-js"> <!--<![endif]-->
<head>
    <meta charset="utf-8">
    <title></title>
    <meta name="description" content="">
    <link rel="stylesheet" href="css/normalize.css">
    <link rel="stylesheet" href="css/main.css">
    <script src="js/vendor/modernizr-2.6.2.min.js"></script>
</head>
<body>
    <section>
        <h1>Calculator</h1>
        <table>
            <input>
            <tr>
                <td><button name="1" class="num-button">1</button></td>
                <td><button name="2" class="num-button">2</button></td>
                <td><button name="3" class="num-button">3</button></td>
                <td><button name="add" class="operation">+</button></td>
            </tr>
            <tr>
                <td><button name="4" class="num-button">4</button></td>
                <td><button name="5" class="num-button">5</button></td>
                <td><button name="6" class="num-button">6</button></td>
                <td><button name="subtract" class="operation">-</button></td>
            </tr>
            <tr>
                <td><button name="7" class="num-button">7</button></td>
                <td><button name="8" class="num-button">8</button></td>
                <td><button name="9" class="num-button">9</button></td>
                <td><button name="multiply" class="operation">x</button></td>
            </tr>
            <tr>
                <td><button name="0" class="num-button">0</button></td>
                <td><button name="." class="num-button">.</button></td>
                <td><button name="equals">=</button></td>
                <td><button name="divide" class="operation">/</button></td>
            </tr>
        </table>
        <button class="clear">Clear</button>
    </section>

    <script src="js/vendor/jquery-1.10.2.min.js"></script>
    <script src="js/main.js"></script>
</body>
</html>

Here is the jQuery that I need critiqued:

$(function() {
var input = $('input');
var equals = $('[name="equals"]');

/*when a button with the 'num-button' class is clicked,
adds the number of the button via its name to the inputs value*/
$('.num-button').on('click', function() {
    input.val(input.val() + $(this).attr('name'));
});

/*when a button with the 'operation' class is clicked,
check if the subtract button was clicked while input field
is empty, if so add - to input value, if not add inputs value
to the inputs name attribute, then remove any classes attached
to the equals button (if any) and then add the operation buttons
name to the equals button as a class */
$('.operation').on('click', function() {
    var opName = $(this).attr('name');
    if (opName == 'subtract' && input.val().length == 0) {
        input.val('-');
    } else {
        input.attr('name', input.val());
        input.val('');
        equals.removeClass();
        equals.addClass(opName);
    }
});

/*when the equals button is clicked check to find out
what the equal buttons class is, then get the values from
the inputs attribute that we converted to a name (above),
then get the current input value, then convert those string
values to floated numbers and follow with the appropriate
math operation for the two floated numbers*/
equals.on('click', function() {
    var firstInput;
    var secondInput;
    if (equals.hasClass('add')) {
        firstInput = parseFloat(input.attr('name'));
        secondInput = parseFloat(input.val());
        input.val(firstInput + secondInput);
    } else if (equals.hasClass('subtract')) {
        firstInput = parseFloat(input.attr('name'));
        secondInput = parseFloat(input.val());
        input.val(firstInput - secondInput);
    } else if (equals.hasClass('multiply')) {
        firstInput = parseFloat(input.attr('name'));
        secondInput = parseFloat(input.val());
        input.val(firstInput * secondInput);
    } else if (equals.hasClass('divide')) {
        firstInput = parseFloat(input.attr('name'));
        secondInput = parseFloat(input.val());
        input.val(firstInput / secondInput);
    }
});

//clears input field when clear button is clicked
$('.clear').on('click', function () {
    input.val('');
});
});
\$\endgroup\$
1
\$\begingroup\$

You shouldn't specify ALL of your code in the jQuery ready callback. Try and move all your callbacks, etc out. OR if your code is at the end of the page, you can just execute it straight away.

In your equals on click handle, firstInput and secondInput are always the same regards of class so you can make it like this:

equals.on('click', function() {
    var firstInput = parseFloat(input.attr('name'));
    var secondInput = parseFloat(input.val());
    if (equals.hasClass('add')) {
        input.val(firstInput + secondInput);
    } else if (equals.hasClass('subtract')) {
        input.val(firstInput - secondInput);
    } else if (equals.hasClass('multiply')) {
        input.val(firstInput * secondInput);
    } else if (equals.hasClass('divide')) {
        input.val(firstInput / secondInput);
    }
});

*UPDATES:*

You should default to using === (triple equals) when doing comparisons instead of == (double equals).

When checking .length, it's handy to remember that 0 == false so you could write your code as !input.val().length. Actually, you can make this even more concise because an empty string is also 'falsy', so instead do !input.val().

You should create a better selector for your main user input, so that if another gets introduced to the page it won't break. Since your page is only going to have one input, I'd use an ID to make it a unique input. <input id="input-calculator"> ... then query for it in your CSS and Javascript as $('#input-calculator'). This remove ambiguity from your code :)

\$\endgroup\$
  • \$\begingroup\$ Set firstInput and secondInput to variables, works. Thanks, I didn't see that. I don't understand what you mean by move all of your callbacks etc. out of the ready function. What should I move outside of the jQuery document.ready function and why? From how I understand it, that function just loads everything inside it after the document has loaded. Why move things outside of it? \$\endgroup\$ – David Jan 10 '14 at 6:53
  • \$\begingroup\$ jQuery.ready is handy if you're trying to query elements that aren't the page yet. Since your JavaScript is at the end of your DOM, you don't need to use jQuery.ready... this is slowing down your app. \$\endgroup\$ – Cobby Jan 11 '14 at 23:28
  • \$\begingroup\$ What meant about the callbacks, is to define the functions as soon as possible so the browser can parse them. Then in jQuery.ready define the handle and pass them a reference to the callback (which is a variable). Blindly wrapping all your code in jQuery.ready is a great way to slow the page for now reason. \$\endgroup\$ – Cobby Jan 11 '14 at 23:32
  • \$\begingroup\$ Makes sense. So when you say jQuery.ready is handy for trying to query elements that are not on the page yet, do you mean just the elements after the script tag, or elements that might be introduced dynamically using javascript, even if the script tag is at the end of the page? In other words, does the ready function have any impact on dynamically created content? \$\endgroup\$ – David Jan 12 '14 at 0:02
  • \$\begingroup\$ You really only want $.ready if you're JavaScript is included at the start of the page (i.e. in the <head>). But yes, then inside it you only want to query elements. You should try and move as much code out of $.ready as you can. \$\endgroup\$ – Cobby Jan 12 '14 at 3:18

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.