3
\$\begingroup\$

All of the code is self-explanatory and pretty much complete.

What the code does:

  • Calculates using onscreen numerics and operators
  • Validates any erroneous operations, such as 4 ** 8 or *9

Good features of the code:

  • The code has separate functions handling the validation, evaluation, etc.
  • It's much more modular

Recommended further improvements on the following basis:

  • Understandability
  • Modularity
  • Efficiency
  • Repetition

(function() {
  // private variable accessible by all the functions below
  var keys, input = "",
    result;

  function equate() {
    console.log('the equal button was pressed')

    result = eval(input);
    $('.screen').text(result);
  }

  function clear() {
    input = "";
    $('.screen').text(input);
    result = "";
    console.log('the clear button was pressed')
  }


  function isOperator(_input) {
    return (['+', '-', '*', '%', '/', '.'].indexOf(_input) >= 0)
  }

  function lastInputIsOperator() {
    return isOperator(input.charAt(input.length - 1))
  };


  // created a new module validate input and moved it out of expressInMath
  function validateInput(_input) {
    // prevents any operations like ++, -- etc 
    if (isOperator(_input)) {
      // check the last character input
      if (lastInputIsOperator(input)) {
        if (input.length < 1) {
          input = '';
        } else if (_input === '.' && input.indexOf('.') >= 0) {
          console.log('invalid');
        } else {
          input = input.slice(0, input.length - 1) + _input;
        }
        // avoids any operation of the kind 0.6.7.5 
      } else if (_input === '.' && input.indexOf('.') >= 0) {
        console.log('invalid');
      } else {
        if (input.length < 1) {
          input = '';
        } else {
          input += _input;
        }
      }
      console.log('Operation prohibited')
    } else {
      input += _input;
    }
  }

  function expressInMath(_input) {
    validateInput(_input);
    result = '';
    $('.screen').text(input);
  }

  // analyzes the keys pressed by the user
  function analyze() {

    //private vars
    let $this = $(this),
      _input = $this.text();

    // if clear button is pressed
    if (_input == "C") {
      clear();
    }

    // if equals to button is pressed
    else if (_input == "=") {
      equate();
    }

    // if key or operator button is pressed
    else {
      expressInMath(_input);
    }
  }

  // self invoking initializing function
  (function initialize() {
    $('#calculator span').on('click', analyze);
  }());
}())
/* Basic reset */

* {
  margin: 0;
  padding: 0;
  box-sizing: border-box;
  font: bold 14px Arial, sans-serif;
}

html {
  height: 100%;
  background: radial-gradient()circle, #fff, #ccc;
  background-size: cover;
}

#calculator {
  width: 375px;
  height: 340px;
  margin: 100px auto;
  padding: 20px 20px 9px;
  background: yellowgreen;
  border-radius: 3px;
  box-shadow: 0px 4px #009de4, 10px 15px 0px rgba(0, 0, 0, 0.2);
}


/* top portion */

.top span.clear {
  float: left;
}

.top .screen {
  height: 40px;
  width: 212px;
  float: right;
  background: rgba(0, 0, 0, 0.2);
  box-shadow: inset 0px 4px #009de4;
  padding: 0px 10px;
  /* typography */
  font-size: 17px;
  line-height: 40px;
  color: white;
  text-shadow: 1px 1px 2px rgba(0, 0, 0, 0.2);
  text-align: right
}


/* Clear floats */


/*.keys, .top {overflow: hidden};*/

.keys span,
.top span.clear {
  float: left;
  position: relative;
  top: 0;
  cursor: pointer;
  width: 66px;
  height: 40px;
  background: white;
  border-radius: 2px;
  box-shadow: 0px 4px rgba(0, 0, 0, 0.2);
  margin: 0 7px 11px 0;
  line-height: 40px;
  text-align: center;
  user-select: none;
}

.keys span.operator {
  background: #FFF0F5;
  margin-right: 0px;
}

.keys span.eval {
  background: #f1ff92;
  box-shadow: 0px 4px #9da853;
  color: #888e5f;
}

.top span.clear {
  background: #ff9fa8;
  box-shadow: 0px 4px #ff7c87;
  color: white;
}


/* hover effect */

.keys span:hover {
  background: #9c89f6;
  box-shadow: 0px 4px #6b54d3;
  color: white;
}

.keys span.eval:hover {
  background: #f1ff92;
  box-shadow: 0px 4px #9da853;
  color: #888e5f;
}

.top span.clear:hover {
  background: #ff9fa8;
  box-shadow: 0px 4px #ff7c87;
  color: white;
}


/* active state */

.keys span:active {
  box-shadow: 0px 4px #6b54d3;
  top: 4px;
}

.keys span.eval:active {
  box-shadow: 0px 4px #9da853;
  top: 4px;
}

.top span.clear:active {
  top: 4px;
  box-shadow: 0px 0px #d3545d;
}
<html>

<head>
  <link rel="stylesheet" href="/style.css">
</head>

<body>
  <div id="calculator">
    <div class="top">
      <span class="clear">C</span>
      <div class="screen">0</div>
    </div>
    <div class="keys">
      <span>7</span>
      <span>8</span>
      <span>9</span>
      <span>+</span>
      <span>4</span>
      <span>5</span>
      <span>6</span>
      <span>-</span>
      <span>1</span>
      <span>2</span>
      <span>3</span>
      <span>/</span>
      <span class="num">0</span>
      <span class="decimal">.</span>
      <span class="eval">=</span>
      <span>*</span>
    </div>
  </div>

  <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/2.1.3/jquery.min.js"></script>
  <script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.4/lodash.min.js"></script>
  <script src="new.js"></script>
  <!-- <script src="script.js"></script> -->
</body>

</html>

\$\endgroup\$
1
\$\begingroup\$

Possible bug

This part in validateInput looks like a bug to me:

if (isOperator(_input)) {
  // check the last character input
  if (lastInputIsOperator(input)) {
    // ...
  }
  console.log('Operation prohibited')

That is, when isOperator(_input) is true, the Operation prohibited message is logged. If that's intentional, I don't understand why.

Simplify

This complex chain of conditions can be simplified:

  // check the last character input
  if (lastInputIsOperator(input)) {
    if (input.length < 1) {
      input = '';
    } else if (_input === '.' && input.indexOf('.') >= 0) {
      console.log('invalid');
    } else {
      input = input.slice(0, input.length - 1) + _input;
    }
    // avoids any operation of the kind 0.6.7.5 
  } else if (_input === '.' && input.indexOf('.') >= 0) {
    console.log('invalid');
  } else {
    if (input.length < 1) {
      input = '';
    } else {
      input += _input;
    }
  }

Most notably, you check for multiple dots whether the last input is an operator or not, so that can be lifted out of the chain to appear only once.

Also, when input.length < 1, then input is already the empty string.

This is equivalent:

if (input.length > 0) {
  if (_input === '.' && input.indexOf('.') >= 0) {
    console.log('invalid');
  } else if (endsWithOperator(input)) {
    input = input.slice(0, input.length - 1) + _input;
  } else {
    input += _input;
  }
}

I also took the liberty and renamed lastInputIsOperator to endsWithOperator which doesn't repeat "input" which is implied by the function's parameter, and sounds like a very natural sentence.

Performance

The isOperator function creates a temporary array every time it's called. It would be better to store ['+', '-', '*', '%', '/', '.'] in a constant.

Also note that indexOf does a linear search. It would be better to emulate a hash table by storing the operators in an object.

As yet another alternative, when the operators are all a single character, you could have used a simple string to store them.

\$\endgroup\$

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.