Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

As a part of a study-group I did a very simple little JavaScript calculator. It's not made to look pretty.

Any ideas on how I can make the code prettier?

/*
 * This is a simple JS calculator
 * Made by Gemtastic 2016-05-08
 */

// Gather and set up DOM elements
var numberScreen = 'screen';
var numBtns = document.getElementsByClassName('number');
var operators = document.getElementsByClassName('operator');

// Mini statemachine of if we're in the middle of operating on a number.
var operating = false;
var operand = '';

/*
 * Clears the content from the screen.
 */
function clearScreen() {
  document.getElementById(numberScreen).innerHTML = "0";
  resetOperating();
}

/*
 * Evaluates the entered numbers with the given operator.
 */
function evaluate() {
  var result;

  /* If it isn't operating or the last entered is the operator 
   * there's no point in doing anything.
   */
  if(operating && isLastEnteredNumber()){
    var content = document.getElementById(numberScreen).innerHTML;
    var numbers = content.split(operand);

    // Parse strings to be able to operate on them.
    var firstNumber = parseInt(numbers[0], 10);
    var secondNumber = parseInt(numbers[1], 10);

    // Switch the stored operator.
    switch(operand) {
      case '+':
        result = firstNumber + secondNumber;
        break;
      case '÷':
        if(firstNumber !== 0 && secondNumber !== 0) {
          result = firstNumber / secondNumber;
        } else {
          result = 0;
        }
        break;
      case '×':
        result = firstNumber * secondNumber;
        break;
      case '–':
        result = firstNumber - secondNumber;
        break;
      default:
        console.log('Something went terribly wrong! ' + operand + " is not a supported operator!");
    }
  }

  resetOperating();
  document.getElementById(numberScreen).innerHTML = result || 0;
  return result || 0;
}

/*
 * Transforms the current input or the input's value if it's a 
 * full expression but not evaluated into a percentile of itself.
 */
function percent() {
  var content = document.getElementById(numberScreen).innerHTML;
  // Cecks if you've operated two numbers but not yet pressed '=' for evaluation
  if(operating && isLastEnteredNumber()) {
    var result = evaluate();      // Evaluates the screen data
    if(result != '0') {           // if the result isn't 0 make the transformation
      content = content / 100;
    } else {                      // if it is 0 then just append a decimal for user feedback.
      content += '.0';
    }
  } else if (!operating && isLastEnteredNumber && content != '0') { // If you haven't started an operation an a not 0 character do the transformation
      content = content / 100;
  } else {
    content += '.0';              // failsafe append a decimal for user feedback.
    console.log('Bug in percent function! Operating: ' + operating + ', screen content:' + content); // Log, because if this happens we have a bug lulz.
  }
  document.getElementById(numberScreen).innerHTML = content;
  resetOperating();
}

/*
 * Adds a decimal to the screen number.
 */
function decimal() {
  document.getElementById(numberScreen).innerHTML += ".";
}

/*
 * Changes the value to be positive or negative.
 */
function editPositiveNegativeValue() {
    var content = document.getElementById(numberScreen).innerHTML;

    // Simple check if the first character on screen is - or not. If it is it just removes it.
    if(content.charAt(0) == '-') {
      content = content.slice(1);
    } else {
      content = "-" + content;
    }
    document.getElementById(numberScreen).innerHTML = content;
}

/*
 * Resets the state-machine of operation to "off".
 */
function resetOperating() {
  operating = false;
  operand = '';
}

/*
 * Checks if the last character on the screen is a number.
 */
function isLastEnteredNumber() {
  var content = document.getElementById(numberScreen).innerHTML;
  return !isNaN(content.charAt(content.length - 1));
}

/*
 * If the screen isn't showing a simple 0 append
 * to it. Else it will remplace the 0 with the
 * button's number.
 */
function numberClick(event) {
  var content = document.getElementById(numberScreen).innerHTML;
  var btnNum = event.target.innerHTML;
  
  if(content != "0"){
    content += btnNum;
  } else {
    content = btnNum;
  }
  document.getElementById(numberScreen).innerHTML = content;
}

/*
 * Called when you've pressed an operator button.
 */
function operatorClick(event) {
  var operator = event.target.innerHTML;
  var content = document.getElementById(numberScreen).innerHTML;

  switch(operator) {
    case '=':
      evaluate();
      break;
    case 'C':
      clearScreen();
      break;
    case '%':
      percent();
      break;
    case '+/-':
      editPositiveNegativeValue();
      break;
    case '.':
      decimal();
      break;
    default:
      operating = true;
      operand = operator;
      content += operator;
      document.getElementById(numberScreen).innerHTML = content;
      break;
  }
}

// Set up listeners for the operands
for(var o = 0; o < operators.length; o++) {
  operators[o].addEventListener('click', operatorClick, false);
}

// Set up listeners for the buttons
for(var b =  0; b < numBtns.length; b++) {
numBtns[b].addEventListener('click', numberClick, false);
}

clearScreen();
.btn {
  display: inline-block;
  border: 1px solid #ddd;
  padding: 5px;
  margin: 2px;
  border-radius: 5px;
  width: 1em;
  text-align: center;
}

.btn:hover {
  background-color: #eee;
  cursor: pointer;
}

.dummy {
  border: 1px solid #fff;
}
.dummy:hover {
  background-color: transparent;
}

.pad {
  position: absolute;
}

#screen {
  display: inline-block;
  margin-bottom:5px;
  padding: 5px;
  height: 1em;
  width: 90%;
  border-radius: 5px;
  border: 1px solid #ddd;
  text-align: right;
}
<div class="pad">
  <div id="screen"></div>
  <div class="row">
    <div class="btn operator">C</div>
    <div class="btn operator">%</div>
    <div class="btn operator">+/-</div>
    <div class="btn operator">&divide;</div>
  </div>
  <div class="row">
    <div class="btn number">7</div>
    <div class="btn number">8</div>
    <div class="btn number">9</div>
    <div class="btn operator">&#8211;</div>
  </div>
  <div class="row">
    <div class="btn number">4</div>
    <div class="btn number">5</div>
    <div class="btn number">6</div>
    <div class="btn operator">+</div>
  </div>
  <div class="row">
    <div class="btn number">1</div>
    <div class="btn number">2</div>
    <div class="btn number">3</div>
    <div class="btn operator">&times;</div>
  </div>
  <div class="row">
    <div class="btn dummy">&nbsp;</div>
    <div class="btn number">0</div>
    <div class="btn operator">.</div>
    <div class="btn operator">=</div>
  </div>
</div>

Known issues:

  • Modulus (%) is not modulus as in 5%2 = 3 it turns the number on the display into a percentage.
  • There's no limit to amount of numbers in the display
  • It doesn't understand order of operations or many calculations at once. 5 + 56 - 2 = bork. You have to do 5 + 56 = 61 - 2 = 59
share|improve this question
    
Possible bug :- 1 / 3 * 3 gives 3. – A---B 9 hours ago
2  
Definite bug: 0.3 x 3 = 0 – Tyrsius 9 hours ago
    
It's not made to support automatic correct order of calculations so it's not a bug, it's also probably because of your browser's JS interpreter, but the 0.3 x 3 is indeed a bug as I never implemented decimals :S – Gemtastic 9 hours ago
    
5+56-2 == 3 what now? – njzk2 7 hours ago
    
@njzk2 It doesn't handle many operations in one go either. Simple claculator, but I should probably fix that :) Updated question to clarify this. – Gemtastic 6 hours ago

To solve "There's no limit to amount of numbers in the display", If you change your javascript to:

if(content != "0"){
    if (content.length < 16)
        content += btnNum;
  } else {
    content = btnNum;
  }

You'll be saying "only add more numbers if there is less than 16 numbers on my screen". (I put 16 because I tested your code and 16 is the max quantity until your div starts to expand)

What do you mean with "Modulus is broken"?

share|improve this answer
    
Modulus doesn't do modulus, it converts the number to percent. – Gemtastic 10 hours ago
    
@Gemtastic Sorry, I still don't get it. In your code, it seems like you want to % convert do percent, but you actually want to it calculate modulus? Or you were only warning us that you "changed" the function of the symbol "%"? – Cris Motinha 10 hours ago
    
It is an intentional choice, to make the button do SOMETHING but modulous is a different operator that does different calculations :) – Gemtastic 10 hours ago
    
Oh, that's fine. In all calculators "%" mean percentage, so that's okay :) I just don't get why you said it was broken... Your percentage function is broken, is that it? – Cris Motinha 10 hours ago
2  
Is there anyway to try 5%2? Because at the exactly time you click % you get the percentage result. I don't think you should call it a broken thing, although you could create an "help sheet" (where you could say that % is percentage, not modulus) :) – Cris Motinha 10 hours ago

Interesting question,

  • I noticed that 9-3*3 does not return 0, but 27, you are not following PEMDAS
  • Personally, I would make sure that the input only contains numbers, dots, and math operators, and then use eval. eval("9-3*3") does return correctly 0.
  • 0.0022 * 2 returns zero.
  • I would assign html elements like the one found from document.getElementById(numberScreen) to a higher scope variable for readability and performance
  • You do not check for existing periods in the current number, allowing for numbers like 12.24.2016
  • Using a pure UI function (innerHTML) to drive the logic is wrong. It's okay for a beginner class, but anything more advanced should drive of (id). You dont want to replace 'x' with '*' and also having to change the JS code
  • Your indentation is not perfect, use http://jsbeautifier.org/
  • You have zero warnings on jshint.com, which is amazing
share|improve this answer
    
"You have zero warnings on jshint.com, which is amazing". Indeed – Tyrsius 9 hours ago
    
"anything more advanced should drive of (id)." -- Was this sentence supposed to be refactored or something, because i don't get it. – Scimonster 8 hours ago
3  
9-3*3 giving 27 isn't even a PEMDAS issue as far as I can tell. (9-3)*3 would give 18. I'm not exactly sure how it's parsing 9-3*3. – Dason 7 hours ago
1  
I'm not sure how it parses 9-3*3 either because I obviously haven't implemented multiple calculations :P I do get the ID part. – Gemtastic 6 hours ago

The code looks pretty good. Here are a few suggestions:

You reference document.getElementById(numberScreen) quite a few times. Why not assign it a shorter variable name at the top of your JavaScript file, like you did with numBtns and operators?

You could group your code into sections, to make it easier to find things. For example, have a variable-declaring section, then a functions section, then the code execution section. The sections can be differentiated with comment lines, like:

//--------- functions -------------------

That's just one way to group the code. Another way might work better for you.

You could also group your functions with similar functions, to make them easier to find. As one example, clearScreen() and resetOperating() could be next to each other, and evaluate() could be toward the bottom. While grouping your functions, you can check to make sure no functions are called before they are declared -- it won't make the code run better, but it will increase readability.

share|improve this answer

code layout

For code cleanliness, I recommend rearranging your code so that the function definitions are all together at the end, after the imperative commands.

// declare variables
// set up event listeners
// clear the screen
// function definitions

avoiding global namespace pollution

Since your logic (wisely) does not depend on global variable declarations, you can avoid polluting the global namespace by wrapping all your code in an immediately executing function expression (IEFE).

(function(){
    // your code here
})();

Since JavaScript variables are function-scoped, this essentially creates a new scope in which your code and functions can refer to variables defined within. Code outside the scope (such as elsewhere on the page) can then use the same variable and function names without running into any collisions.

share|improve this answer

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.