4
\$\begingroup\$

Since I am self taught, I would greatly appreciate some input on my code for this JS calculator:

  1. What redundancies i'm failing to see and remove?
  2. How is my naming for var's and functions? Is it clear or vague?
  3. How is my overall readability? Is what i'm writing even clear, or is it hard to decipher?
  4. Are my comments helpful or redundant?
  5. What stands out to you that needs to be fixed the most?

Just fyi, this code wont run on its own as i have not included the HTML/CSS that accompanies this. I would like to not worry about the front end design in this particular post. All input in appreciated.

const buttons = ['CE', 'AC', 'x', '7', '8', '9', '/', '4', '5', '6',
 '-', '1', '2', '3', '+', '0', '.', '='
];

var currentEntry = [],
 totalEntry = [];
var equals = true;

//const testNum = /[0-9]/g;
const regexOperands = /[+\-\/x=]/;

const totalArea = document.getElementById("totalArea");
const currentArea = document.getElementById("currentArea");
const numberArea = document.getElementById("numberArea");
const faceHappy = document.getElementById("face-happy");

window.onload = () => {
 makeButtons();
}

function applyClick(userInput) { //all our clicking behaviors for buttons
 let btn = document.getElementById("b" + userInput);

 btn.onclick = () => {
  let totalAreaLength = totalArea.textContent.length;
  //first we clear the face
  changeDisplay(userInput);
  if (equals) { //clear after =, or for first entry
   if (!isNaN(userInput)) { //if there is pre-existing numbers after hitting equals then delete
    currentArea.textContent = '';
   } else {
    //places total from previous calculation as first entry
    currentArea.textContent = totalEntry;
   }
   totalArea.textContent = '';
   currentEntry = [];
   totalEntry = [];
   equals = false;
  }
  //first we restrict input length to 17
  if (currentArea.textContent.length > 17 || totalArea.textContent.length > 17) {
   alert("Number Limit Reached!");
   currentArea.textContent = "";
   totalArea.textContent = "";
   equals = true;
  } else if (!isNaN(userInput)) { //test for number
   equals = false;
   currentArea.textContent = (currentArea.textContent == "0") ? userInput : currentArea.textContent + userInput;
  } else if (isNaN(userInput)) { //**for all non numerics**\\
   if (equals) { //restricts equals being pressed twice
    return;
   } else {
    if (userInput === "=") { //to get answer
     currentEntry = filterUserInput(userInput);
     let saveUserInput = currentArea.textContent;
     operateOnEntry(currentEntry);
     equals = true;
     totalEntry = currentEntry[0]; //will save answer for next calculation
     currentArea.textContent = saveUserInput; //will display equation
     totalArea.textContent = currentEntry; //will display answer
    } else if (userInput === ".") {
     let lastEntry = filterUserInput(userInput);
     if (!lastEntry.includes(".")) { //test for pre-existing period
      currentArea.textContent = currentArea.textContent + userInput;
     }
    } else if (userInput === "AC" || userInput === "CE") {
     if (userInput === "AC") {
      changeDisplay(userInput);
      currentArea.textContent = "";
      totalArea.textContent = "";
     } else if (userInput === "CE") {
      let clearedLastEntry = filterUserInput(userInput);
      currentArea.textContent = clearedLastEntry.join('');
     }
    } else { //this is default operator behavior
     let lastEntry = filterUserInput(userInput);
     //limits operators from printing if there is a pre-existing operator as last user input
     currentArea.textContent = (regexOperands.test(lastEntry)) ? currentArea.textContent : currentArea.textContent + userInput;
    }
   }
  }
 }
}

function operateOnEntry(userEntry) {
 //this is where the calculations occur when hitting =
 let a, b, c, index;
 if (userEntry.includes("x")) {
  index = userEntry.indexOf('x');
  a = Number(userEntry[index - 1]);
  b = Number(userEntry[index + 1]);
  c = a * b;
  userEntry.splice((index - 1), 3, c);
  return operateOnEntry(userEntry);
 } else if (userEntry.includes("/")) {
  index = userEntry.indexOf('/');
  a = Number(userEntry[index - 1]);
  b = Number(userEntry[index + 1]);
  c = a / b;
  userEntry.splice((index - 1), 3, c);
  return operateOnEntry(userEntry);
 } else if (currentEntry.includes("+") || currentEntry.includes("-")) {
  index = userEntry[1];
  a = Number(userEntry[0]);
  b = Number(userEntry[2]);
  console.log("index: " + index);
  if (index == '+') {
   c = a + b;
   userEntry.splice(0, 3, c);
   return operateOnEntry(userEntry);
  } else {
   c = a - b;
   userEntry.splice(0, 3, c);
   return operateOnEntry(userEntry);
  }
 }
 return userEntry;
}

function filterUserInput(userInput) {
 //this function converts the user input into an array
 let testCurrentEntry;
 if (userInput === ".") {
  testCurrentEntry = currentArea.textContent.split(regexOperands);
  return testCurrentEntry.pop();
 } else if (userInput === "=") {
  testCurrentEntry = currentArea.textContent; //.split(regexOperands)
  testCurrentEntry = testCurrentEntry.split(/([+\-\/x=])/g);
  return testCurrentEntry;
 } else if (userInput === "CE") {
  testCurrentEntry = currentArea.textContent.split("");
  testCurrentEntry.pop()
  return testCurrentEntry;
 } else {
  testCurrentEntry = currentArea.textContent.split('');
  return testCurrentEntry.pop();
 }
}

function changeDisplay(userInput) {
 numberArea.style.display = 'block';
 if (userInput == 'AC') {
  numberArea.style.display = 'none';
  faceHappy.style.display = "block";
 }
}

function makeButtons() {
 for (var i = 0; i < buttons.length; i++) {
  var btn = document.createElement("BUTTON");
  var t = document.createTextNode(buttons[i]);
  var container = document.getElementById('container');
  btn.id = "b" + buttons[i];
  btn.className = "button";
  btn.appendChild(t);
  container.appendChild(btn);
  applyClick(buttons[i]);
 }
}
\$\endgroup\$

2 Answers 2

Reset to default
1
\$\begingroup\$

Building reusable programs

Just fyi, this code wont run on its own as i have not included the HTML/CSS that accompanies this. I would like to not worry about the front end design in this particular post.

Therein lies the top advice I can give to you. Try this exercise:

  1. Implement the main logic as a module with no UI (ideally unit tested)

  2. Implement a snippet of code that demonstrates using the module, either by using hard-coded parameters, or input from the console

  3. Implement the UI that uses the module, operations triggered by mouse or keyboard events, taking parameters from the events or the DOM

You can do all that without using any frameworks or anything fancy. Step 1 could go in a single program.js file, with no code execution in the global scope. Step 2 could be a function called main in the same file, and it could be executed in global scope. At this point the program should be executable by node program.js. No frameworks used.

By building up your programs this way, you end up with reusable and testable components.

Your main questions

  1. What redundancies i'm failing to see and remove?

Not a lot, congrats! In operateOnEntry, the parsing of the operators have some similar elements, essentially following the logic:

  • Find the index of an operator in the input array (which is a mix of numbers and operators)
  • Apply the operator using as arguments the left and right element
  • Replace the 3 values in the array with the result of the operation
  • Repeat until a single value remains

Instead of spelling out the steps for each operator one by one, you could have a mapping of operators to operations, loop over the operators according to their precedence, and follow the common logic as outlined above.

  1. How is my naming for var's and functions? Is it clear or vague?

operateOnEntry doesn't describe well what it does. What kind of operation? What kind of entry? It actually evaluates a mathematical expression. I'd call it evaluate and name its parameter expression, and have this as a function of an abstraction (prototype) called Calculator.

filterUserInput doesn't describe well what it does. To "filter" usually means to eliminate some values from the input. This function takes a string as input, it operates on some global values, and returns either an array or a string. I think this function needs to be redesigned, naming alone cannot fix its issues.

let a, b, c is clearly not great. And it's not great to declare variables up front when they can be declared when they are initialized. I would have used let arg1 = ... and let arg2 = ..., and use operator(arg1, arg2) inlined without assigning it to let result = ....

Take a look at the different kind of uses of the index variable:

 } else if (userEntry.includes("/")) {
  index = userEntry.indexOf('/');
  a = Number(userEntry[index - 1]);
  b = Number(userEntry[index + 1]);
  c = a / b;
  userEntry.splice((index - 1), 3, c);
  return operateOnEntry(userEntry);
 } else if (currentEntry.includes("+") || currentEntry.includes("-")) {
  index = userEntry[1];
  a = Number(userEntry[0]);
  b = Number(userEntry[2]);
  console.log("index: " + index);
  if (index == '+') {

The first use is good, it indicates the index of an array. In the second use it's not actually an index, but something else.

Not related to naming, but to using variables, consider this:

  testCurrentEntry = currentArea.textContent; //.split(regexOperands)
  testCurrentEntry = testCurrentEntry.split(/([+\-\/x=])/g);

After the first assignment, testCurrentEntry has a string value. After the second assignment, the same variable has an array value. This quickly gets confusing. A type-safe language would not let you do this, for your own good. I suggest to adopt the habit of not reusing a variable with multiple types, even when the language allows it.

Another issue with variables is in operateOnEntry, userEntry and currentEntry are used, but they actually point to the same value. This is both extremely confusing and extremely error-prone.

  1. How is my overall readability? Is what i'm writing even clear, or is it hard to decipher?

I find it hard to understand, for several reasons:

  • The calculator logic and the UI are mixed together
  • The names don't describe themselves well
  • There are many global variables (ideally there should be none), and global state is always hard to follow
  1. Are my comments helpful or redundant?

I haven't found a helpful comment, but I haven't tried to understand the entire code. Since you ask, here are some examples to improve.

This comment states the obvious:

  //first we restrict input length to 17
  if (currentArea.textContent.length > 17 || totalArea.textContent.length > 17) {

This comment is actually wrong, the function returns non-array values in some cases.

function filterUserInput(userInput) {
 //this function converts the user input into an array

Incorrect comments cause harm.

It's best when the code can speak for itself without comments. It's not always possible, but a good goal to strive for.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Instead of nesting the if statements, would it be better to use a switch statement? janos, thank you so much. Without a teacher this is incredibly difficult to get this kind of concise evaluation of my code. I'm sure this comment took a while to write and words don't express my gratitude. \$\endgroup\$ Commented Aug 12, 2018 at 19:16
1
\$\begingroup\$

First thing that feels off is that you're mixing using var together with let and const (check here: var in es-6)

As for names of variables, don't be afraid to make them a bit more descriptive. For example "equals" doesn't explain much. If variable is a boolean it's a common practice to name them starting with "is" (isEqual can be true or false and it's indicated just by looking at variables name). You also should say what equals to what, that would explain more.

It would be also good to avoid nesting so many if statements.

Oh, and remember to remove all console.logs in the final version

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.