5
\$\begingroup\$

i build a calc & want to a review that what i did wrong & what is right method to do.

    <html>
  <script>
    
    var inputIdFirst = "valueOfX",inputIdSecond = "valueOfY",outputId = "resultHere";
    var getInputs = function(id) {
      return parseInt(document.getElementById(id).value);
    }
    var showOutput = function(outputValue, outputIdAsArg) {
      //if no argument is given then by default "outputId" taken
      document.getElementById(outputId).innerHTML = outputValue;
    }
    var manuplateAs = function(operationName, valueOfX, valueOfY) {
      if(operationName == 'add')
        return valueOfX + valueOfY;
      else if(operationName == 'sub')
        return valueOfX - valueOfY;
      else if(operationName == 'mul')
        return valueOfX * valueOfY;
      else if(operationName == 'div')
        return valueOfX / valueOfY;
      //can add as many as you wish 
    } 
    var operation = function(operationName){
      x = getInputs(inputIdFirst);
      y = getInputs(inputIdSecond);
      output = manuplateAs(operationName, x, y);
      showOutput(output);
      console.log(x + " " + operationName + " " + y + " = " + output)
      //see console to understand this more
      console.log(this)
    };
  </script>
</head>
<body>
      Enter first number
      <input type="number" id="valueOfX">
      Enter second number
      <input type="number" id="valueOfY">
      <button onclick="operation('add')"> Add </button>
      <button onclick="operation('sub')"> Sub</button>
      <button onclick="operation('mul')"> Mult</button>
      <button onclick="operation('div')"> Divi</button>
      <h1 id="resultHere"></h1>
</body>
</html>

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
3
\$\begingroup\$

To get the ball rolling, a few things that come to mind:

JavaScript

Storing DOM-elements

In getInputs and showOuput you use document.getElementById to get the same DOM elements over and over again. You can simply store or "cache" them in variable an re-use them anytime you want:

var inputX = document.getElementById('x'),
    inputY = document.getElementById('y'),
    output = document.getElementById('output');

showOutput which seems to have an unused value in its signature, would now look like:

var showOutput = function(value) {
    output.innerHTML = value;
}

Filter input

getInputs already uses parseInt to filter the user's input. This will still result in NaN for all inputs that can't be parsed to integer, like "hey".

You should test for this scenario and give the user a hint that something failed:

var operation = function(operationName) {
    var x = getInputs(inputX),
        y = getInputs(inputY);

    if (isNaN(x) || isNaN(y)) {
        // show an error message or give the inputs a red border etc.
        return;
    }

    // […]
};

By the way, your calculator doesn't make it clear, that it can only process integer values, which might lead to confusing results with inputs like this:

1.5 + 2.5 = 3

Improve the UX and leave the visitor a note.

manuplateAs

I think you meant to call the function manipulate. But why don't you simply call it calculate?

You can also use a switch-statement and shorter variable names to increase readability:

var calculate = function(operation, x, y) {
    switch(operation) {
        case 'add': return x + y;
        case 'sub': return x - y;
        case 'mul': return x * y;
        case 'div': return x / y;
    };
}

Again your calculator doesn't handle edge cases. For example it will show Infinity for division by 0 or NaN if numbers get to big.

You could test the result for NaN or Infinity and show a proper message to the user:

var result = calculate('div', x, y);

if (!isFinite(result)) {
    // show an error message
    return;
}

HTML

<head>

Event though it's a very simply example, add these two things in your header:

<meta charset="UTF-8">
<title>Simple Calculator</title>

It makes sure that:

  • The correct character set is chosen.
  • The user can find the tab with the calculator more easily.

Form

You're using form elements that are not descendants of a form element, nor do you link them to a form using the form-attribute. That is invalid HTML. Simply wrap everything into a form:

<form>
    <input type="number" id="x">
    <input type="number" id="y">
</form>

A button is by default a submit button. You can change this by setting the type-attribute:

<button type="button" onclick="operation('add')">Add</button>

To further improve UX create labels to couple your text with a corresponding input. This also makes sure, that the input is focused as soon as a user clicks the label's text:

<label>
    First number
    <input type="number" id="x">
</label>

Or alternatively:

<label for="x">First number</label>
<input type="number" id="x">

Personally I wouldn't use a h1-element to output the result. Maybe a disabled input field works here, too.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ thank you very much bro, i'll always remember your given advice \$\endgroup\$
    – Praveen Soni
    May 14, 2017 at 11:11

Your Answer

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

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