Javascript calculator...can't implement switching operators

I have been on this problem for a while now and I think I have found the regex that will sanitize the input so it can’t have any weird cases like 2 decimals in a number. But now when I try to think of an implementation to let the user replace their previous operation I can’t get it to work and the number of my passing tests go down.

My code so far

import React from "react";
import ReactDOM from "react-dom";

import "./styles.css";

// componentWillMount()
// componentDidMount()
// componentWillReceiveProps() //good for comparison
// shouldComponentUpdate()// must return a boolean
// componentWillUpdate()
// componentDidUpdate()
// componentWillUnmount()

class Calculator extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      inputString: ""
    };
    this.handleInput = this.handleInput.bind(this);
    this.handleClear = this.handleClear.bind(this);
    this.handleEquals = this.handleEquals.bind(this);
  }

  handleInput(event) {
      this.setState({
        inputString: this.state.inputString + event.target.value
      });
  }
  handleClear(event) {
    this.setState({
      inputString: ""
    });
  }

  handleEquals() {
    this.setState({
      inputString: eval(this.state.inputString)
    });
  }

  shouldComponentUpdate() {
    let regex = /(\d*\.\d*\.)|\.{2}|\+{2}|\-{2}|^[*\/]|^00|(?<=\b)0{2}/g;
    return !regex.test(this.state.inputString);
  }

  render() {
    return (
      <div className="Calc">
        <button id="equals" value="=" onClick={this.handleEquals}>
          =
        </button>
        <button id="zero" onClick={this.handleInput} value="0">
          0
        </button>
        <button id="one" onClick={this.handleInput} value="1">
          1
        </button>
        <button id="two" onClick={this.handleInput} value="2">
          2
        </button>
        <button id="three" onClick={this.handleInput} value="3">
          3
        </button>
        <button id="four" onClick={this.handleInput} value="4">
          4
        </button>
        <button id="five" onClick={this.handleInput} value="5">
          5
        </button>
        <button id="six" onClick={this.handleInput} value="6">
          6
        </button>
        <button id="seven" onClick={this.handleInput} value="7">
          7
        </button>
        <button id="eight" onClick={this.handleInput} value="8">
          8
        </button>
        <button id="nine" onClick={this.handleInput} value="9">
          9
        </button>
        <button id="add" value="+" onClick={this.handleInput}>
          +
        </button>
        <button id="subtract" value="-" onClick={this.handleInput}>
          -
        </button>
        <button id="divide" value="/" onClick={this.handleInput}>
          /
        </button>
        <button id="multiply" value="*" onClick={this.handleInput}>
          *
        </button>
        <button id="decimal" value="." onClick={this.handleInput}>
          .
        </button>
        <button id="clear" onClick={this.handleClear}>
          AC
        </button>
        <div id="display">
          {this.state.inputString === "" ? 0 : this.state.inputString}
        </div>
      </div>
    );
  }
}

const rootElement = document.getElementById("root");
ReactDOM.render(<Calculator />, rootElement);

Does anyone know how I can go about replacing operators? I’m trying to think of a solution that uses the replace method but can’t see a solution. Thank you for reading.

I didn’t implement this with one massive regex, like some people seem to want to. But, looking at your code, it seems to me that “shouldComponentUpdate” is a little late to be sanitizing input. At that point, you’ve already changed the value of this.state.inputString by tacking on event.target.value. I think I handled my input sanitization when the input was given (in your case, before the this.setState() call in handleInput(). This approach will let you decide whether state should be updated, and let React decide when to re-render as a pure function of this.state.

I also noticed that your regex is trying to decide all the cases you DON’T want, which is an interesting choice, but forces you to code for all edge cases, instead of ignoring input that doesn’t conform to the pattern you want. Reading your regex, i get:
(patterns we don’t want are):

  • a decimal after a decimal between two digit strings
  • two decimals in a row
  • two of the SAME operator in a row (summing up four repetitive regexes)
  • multiply or divide if no digit has been entered since power on/clear
  • two zeroes at the beginning of the string (doesn’t cover the case of “5+005”)
  • two zeroes at the beginning of a generic digit string (using positive lookbehind when lookbehind isn’t supported on all javascript engines)

Were I you, I would comment the heck out of my code to explain to future-me-a-week-after-I-moved-on-to-another-project what I was thinking without having to decrypt my regex. You can cut/paste my comments here. Let’s call it pair-programming, since I am translating code to English for comments.

I think you are causing yourself more headaches, but you are pretty close to a solution, so instead of telling you to scrap everything to be able to replace the last operator if another operator is entered, I ask you to ask yourself: if you used handleInput() as the gatekeeper for input, could you use multiple regexes serially to handle things that translate to:
“do I want to allow this input?”
“do I want to allow this input, but replace something in the current input string if certain conditions are met?”

You have the skills, so that should be enough of a hint.

Thank you for the feedback. You are right, I have been causing myself headaches with this problem.

I think I get get your saying about shouldComponentUpdate, I thought it meant whether it should also update the contents of the state and not just the UI. I haven’t even gotten around to styling this thing yet! I’m thinking of trying to come back to this problem at a later time.

Nah, stick with it. You can always come back later to refactor, but frustration is part of the process. Building frustration tolerance is critical. Also critical is learning to ask for help. Implementing my changes is really simple:

1 - copy the regex test into handleClick, before the setState call.
2 - add another line before the setState call that can handle the replacement in the double-operator case.

I just don’t see it. Here it what I have

var operatorRegex = /[*\/\-\+]/; //defined outside the class

  handleInput(event) {
    if (!regex.test(this.state.inputString)) {
      if (operatorRegex.test(event.target.value)) {
        inputString: this.state.inputString.replace(
          /[*\/\-\+]$/,
          event.target.value
        );
      } else {
        this.setState({
          inputString: this.state.inputString + event.target.value
        });
      }
    }
  }

This is about the longest stall I’ve had on FCC yet. I feel like I can’t even write the logic down on paper. If I’m taking this long on the calculator I can only imagine how much the Pomodoro clock is going to take me.

Note: If you don’t reply to me, I don’t get notified. It was chance that I looked up and the in the “updated topics” list I saw this thread and remembered that I was in it.

Now, on to it. Below is your code, commented with numbered footnotes. Get into the habit of translating your control statements with inline comments. It helps, trust me.

var operatorRegex = /[*\/\-\+]/; //defined outside the class

  handleInput(event) {
    if (!regex.test(this.state.inputString)) { //[1]if our "patterns we don't want" regex doesn't match anything...
      if (operatorRegex.test(event.target.value)) {//if the new input is an operator...
        inputString: this.state.inputString.replace( //[2]
          /[*\/\-\+]$/,
          event.target.value
        ); //[2.5] replace any operator we find on the end of the `this.state.inputString` with the input.
      } else { //if the new input isn't an operator...
        this.setState({//update state with the new input
          inputString: this.state.inputString + event.target.value
        });
      }
    }
  }
  1. assuming regex is defined to be the same as in earlier post, this means that we only run this code if we are OK with the CURRENT value of this.state.inputString. It would be better if we tested the POTENTIAL new value, and if it were OK, then pushed that update to the state. That may be why you’re getting stuck.
  2. Does this syntax even work? It looks like you’re trying to create an object literal, like the one you would pass into this.setState().
    2.5. If that isn’t what you meant to do, this is a BAD IDEA: modifying state directly is an anti-pattern in React. Use a call to this.setState() instead.

So, in the final assessment, you’re close.

Do you still have any code in shouldComponentUpdate? If so, move it over. (like, where are you declaring regex in the input handler?) The point of shouldComponentUpdate is when a shallow comparison of state and props fails to show a reason for React to re-render. This basically (for the beginner) only applies if your state variable is nested multiple levels deep, and the changes to state were only in those deeper levels.

Remember: test what COULD be the new state for unwanted patterns. If it passes, you have a decision to make:

  • if the input key was an operator AND the last key was also an operator, we have to update state with a truncated version of previous state and the new key.
  • otherwise, we can update state with the previous state and the new key concatenated on.

It’s easy to get lost in the weeds. Try to write the procedure in english. Say it out loud, and write those inline comments before you write code. It’s called pseudocode, and it’s a powerful technique.

@vipatron So now I have reached 14 tests passings which is the most so far. Thank you sir.

Now the real problem that is staring my in the face is reconfiguring my main regex. Which now I beginning to question if a all out regex was the way to go. The only two tests that are failing are the multiple decimals in one number and the leading zeros which you demonstrated to me earlier as not being accounted for by me regex. I thought my first capture group would have caught the multiple decimals in one number but it does not.

to be fair, I was translating the regex for my own understanding as I wrote that, and just wrote what the meaning of the pattern between the alternation operator, |, was. To quote myself:

  • two zeroes at the beginning of the string (doesn’t cover the case of “5+005”)
  • two zeroes at the beginning of a generic digit string (using positive lookbehind when lookbehind isn’t supported on all javascript engines)

Regex isn’t inherently “wrong”. It can be a very dense and efficient way to run multiple tests, and paying the price in skull sweat now can make you just that much more confident later. For example, in my favorite text editor, Notepad++, I pulled a regex replace to break the long “Logic” comment down into lines by what in Javascript would be a call to: commentLine.replace(/(.{70,77}\b)/g, " * $1\n"); in order to break it up to terminal-width lines.

What your code lacks is pseudocode comments describing the logical flow: what you’re testing for, and how you expect to catch cases. You began to see how I comment in my earlier replies. I leave as much pseudocode in as I can in the final product, and use a block comment to describe function logic before the actual function:

/* handleInput
 * =========
 * Validates any key once input, and appropriately updates state or not
 * @param: event
 * @returns: undefined
 * Logic: We test the key pressed (event.target.value) against the existing 
 * validated input (this.state.input) to see if we want to allow the input. If 
 * we do, we place a call to this.setState() by appending our input onto the 
 * inputString. In the case of an operator input, we truncate the inputString by
 *  1 if there are any trailing operators so we effectively replace the last 
 * operator.
  */

If I ever decide the logic of my function needs to change, or I need to add new parameters, or change the return value, I start with the comment, not with the code. It makes it clear to me what what I need to change. My refactoring workflow:

  1. Add my new functionality description to the comment (without deleting the old - nobody cares how big a comment gets, especially while still in development).
  2. Add my new code while quickly commenting-out the lines of code I am replacing so I don’t break it too much.
    1. Repeat until all the planned functionality exists
  3. Remove the old code
  4. Remove the now-defunct description of the old code from the comments
  5. Give everything a once-over to clean up
    1. remove unnecessary comments
    2. Remove any residual debug statements ( Oh console.log(), why can’t I quit you?!?)
    3. Reword badly-written comments (typos, grammar errors, meaningless or unclear)

@vipatron I wil take this very much into consideration going forward.

As of right now I’m taking a look at my regex and it feels that it isn’t doing much of anything with regards to how it should be filtering the inputString. I recently just changed the regex to

var regex = /p/g

just for the heck of it and yet I’m still passing the same amount of tests that I was passing earlier
with the longer regex that I wrote. I mean sure there is a difference when I play and test my calculator on it’s own but the tests say otherwise. I guess I went with regex because I want my code to be concise as possible. Solutions that take a minimal amount of code look nice but now I realize it takes time to reach those kind of solutions.

Again, look at my comment block. You’re NOT supposed to be filtering the inputString. You’re supposed to be checking the CONCATENATION of the current State (inputstring) and the potential new addition (event.target.value) and seeing if that meets acceptable patterns (i.e., doesn’t match your unwanted-pattern-regex). Then you’re supposed to see whether you can just go ahead and update the new state with concatenation result from before, or whether you need to modify inputString before concatenation and updating would be okay.

You are so close. I can’t just tell you the answer because that increases the time until you can fly on your own. If, after you succeed, you want to to refactor from regex to another way, feel free. But it is possible and you’re 90% of the way there.

As for the user stories, I don’t know what you’re talking about. Unless you have duplicated code testing the big regex somewhere, it should be necessary with your project as I understand it for numbers 10, 11, and maybe 12.

You know, I don’t think you’ve linked to a live example of your code yet. I’m operating based on “optimistic updates.”

@vipatron

import React from "react";
import ReactDOM from "react-dom";

import "./styles.css";

// a decimal after a decimal between two digit strings
// two decimals in a row
// two of the SAME operator in a row(summing up four repetitive regexes)
// multiply or divide if no digit has been entered since power on / clear
// two zeroes at the beginning of the string(doesn’t cover the case of “5 + 005”)
// two zeroes at the beginning of a generic digit string(using positive lookbehind when lookbehind isn’t supported on all javascript engines)

var regex = /(\d*\.\d+\.)|\.{2,}|\+{2,}|-{2,}|\*{2,}|^[*/]|^00|(?<=[/+\-*])0{2}/g;
// var regex = /a/g;
// tests for an operator
var operatorRegex = /[*\/\-\+]/;
// tests whether a operator is the last character in inputString
var operatorLast = /[*\/\-\+]$/;

class Calculator extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      inputString: ""
    };
    this.handleInput = this.handleInput.bind(this);
    this.handleClear = this.handleClear.bind(this);
    this.handleEquals = this.handleEquals.bind(this);
  }

  handleInput(event) {
    // console.log(this.state.inputString)
    // if (!regex.test(this.state.inputString)) { //If our 'patterns we don't want ' regex doesn't match anything
      if (
        operatorRegex.test(event.target.value) && //If event value passed in is an operator
        operatorLast.test(this.state.inputString) //If the last value of our inputString is an operator
      ) {
        this.setState({
          inputString: this.state.inputString.replace(
            //replace the old operator with thte new one that was clicked
            operatorLast,
            event.target.value
          )
        });
      } else {
        this.setState({
          inputString: this.state.inputString + event.target.value
        });
      }
    // }
  }

/* 
handleClear will change inputString to it's defined state of "" which is what the tests call for
Displying 0 in the view is taken care of via the ternary operTION in the display declaration
*/

  handleClear(event) {
    this.setState({
      inputString: ""
    });
  }

  handleEquals() {
    this.setState({
      inputString: eval(this.state.inputString)
    });
  }



  // shouldComponentUpdate() {
  //   let regex = /(\d*\.\d*\.)|\.{2}|\+{2}|\-{2}|^[*\/]|^00|(?<=\b)0{2}/g;
  //   return !regex.test(this.state.inputString);
  // }

  render() {
    return (
      <div className="Calc">
        <button id="equals" value="=" onClick={this.handleEquals}>
          =
        </button>
        <button id="zero" onClick={this.handleInput} value="0">
          0
        </button>
        <button id="one" onClick={this.handleInput} value="1">
          1
        </button>
        <button id="two" onClick={this.handleInput} value="2">
          2
        </button>
        <button id="three" onClick={this.handleInput} value="3">
          3
        </button>
        <button id="four" onClick={this.handleInput} value="4">
          4
        </button>
        <button id="five" onClick={this.handleInput} value="5">
          5
        </button>
        <button id="six" onClick={this.handleInput} value="6">
          6
        </button>
        <button id="seven" onClick={this.handleInput} value="7">
          7
        </button>
        <button id="eight" onClick={this.handleInput} value="8">
          8
        </button>
        <button id="nine" onClick={this.handleInput} value="9">
          9
        </button>
        <button id="add" value="+" onClick={this.handleInput}>
          +
        </button>
        <button id="subtract" value="-" onClick={this.handleInput}>
          -
        </button>
        <button id="divide" value="/" onClick={this.handleInput}>
          /
        </button>
        <button id="multiply" value="*" onClick={this.handleInput}>
          *
        </button>
        <button id="decimal" value="." onClick={this.handleInput}>
          .
        </button>
        <button id="clear" onClick={this.handleClear}>
          AC
        </button>
        <div id="display">
          {this.state.inputString === "" ? 0 : this.state.inputString}
        </div>
      </div>
    );
  }
}

const rootElement = document.getElementById("root");
ReactDOM.render(<Calculator />, rootElement);

TODO for after you get the core functionality down: I noticed your Calculator.render() method was really redundant. Except for the last button you define, #clear, you are only changing two pieces of data, and using one of those twice. If you stored that data in an array, you could use Array.prototype.map() to DRY up your code. There was a lesson on using array mapping in React in the FCC curriculum

Since the only place you use the regexes is in handleInput(), you should move them there, and switch from var to const.

I’m running your code on Firefox, and copying it to Codepen, with which I’m more comfortable, and I’m getting a syntax error re: invalid regexp on both that and CodeSandbox. The only regex which potentially had a problem was the one with a lookbehind (not a standard JS engine feature). So, I replaced it with a pattern that tests for 00 after an operator, which I made for you in regexr. (If you don’t know about regexr, it’s past time). Once I did that, it rendered.

I also noticed that in your HTML, your #root div was not INSIDE the body, but above it. Is that causing problems for you re: the testing suite? I know it renders to the body element, so you can’t tell React to directly render to the body.

You are only shy 2 tests from passing (10 & 11). You can do this.

A huge thing I’m not seeing is any debug statements. Unless CodeSandbox has a debugger I don’t know about, console.log() is a lifesaver. As a matter of course, the top line of my projects is always: console.clear(), and I only get rid of it once they are polished to a gleam. Copy/paste this code over the first two lines of your inputhandler, open the console (F12 key), enter some input,and see if you can figure out what I have been saying about you TESTING THE WRONG STRING (inputString):

  handleInput(event) {
    console.log("entered input handler");
    console.log("this.state.inputString:", this.state.inputString);
    console.log("event.target.value:", event.target.value);
    if (!regex.test(this.state.inputString)) {
1 Like

@vipatron I got all tests passing(what a relief). I did some refactoring on the button elements like you said. Are you saying that I can refactor even more…i.e not having a button element by itself for the equals operator?

import React from "react";
import ReactDOM from "react-dom";

import "./styles.css";

// a decimal after a decimal between two digit strings
// two decimals in a row
// two of the SAME operator in a row(summing up four repetitive regexes)
// multiply or divide if no digit has been entered since power on / clear
// two zeroes at the beginning of a generic digit string(using positive lookbehind when lookbehind isn’t supported on all javascript engines)

const BUTTONS = [
  "zero",
  "one",
  "two",
  "three",
  "four",
  "five",
  "six",
  "seven",
  "eight",
  "nine"
];

const OPERATIONSDECIMAL = {
  add: "+",
  subtract: "-",
  multiply: "*",
  divide: "/",
  decimal: "."
};

class Calculator extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      inputString: ""
    };
    this.handleInput = this.handleInput.bind(this);
    this.handleClear = this.handleClear.bind(this);
    this.handleEquals = this.handleEquals.bind(this);
  }

  handleInput(event) {
    const regex = /(\d*\.\d+\.)|\.{2,}|\+{2,}|-{2,}|\*{2,}|^[*/]|^00|[+\-\/*]0{2}/g;
    // tests for an operator
    const operatorRegex = /[*/\-+]/;
    // tests whether a operator is the last character in inputString
    const operatorLast = /[*/\-+]$/;
    console.log("entered input handler");
    console.log("this.state.inputString:", this.state.inputString);
    console.log("event.target.value:", event.target.value);
    if (!regex.test(this.state.inputString + event.target.value)) {
      //If our 'patterns we don't want ' regex doesn't match anything
      if (
        operatorRegex.test(event.target.value) && //If event value passed in is an operator
        operatorLast.test(this.state.inputString) //If the last value of our inputString is an operator
      ) {
        this.setState({
          inputString: this.state.inputString.replace(
            //replace the old operator with thte new one that was clicked
            operatorLast,
            event.target.value
          )
        });
      } else {
        this.setState({
          inputString: this.state.inputString + event.target.value
        });
      }
    }
  }

  handleClear(event) {
    this.setState({
      inputString: ""
    });
  }

  handleEquals() {
    this.setState({
      inputString: eval(this.state.inputString)
    });
  }

  render() {
    // create button elements for the number buttons
    const renderButtons = BUTTONS.map((element, index) => (
      <button id={element} value={index} onClick={this.handleInput}>
        {index}
      </button>
    ));
    // create button elements for the Operators and Decimal plce
    const renderOpDec = Object.keys(OPERATIONSDECIMAL).map(element => (
      <button
        id={element}
        value={OPERATIONSDECIMAL[element]}
        onClick={this.handleInput}
      >
        {OPERATIONSDECIMAL[element]}
      </button>
    ));

    return (
      <div className="Calc">
        <button id="equals" value="=" onClick={this.handleEquals}>
          =
        </button>
        {renderButtons}
        {renderOpDec}
        <button id="clear" onClick={this.handleClear}>
          AC
        </button>
        <div id="display">
          {this.state.inputString === "" ? 0 : this.state.inputString}
        </div>
      </div>
    );
  }
}

const rootElement = document.getElementById("root");
ReactDOM.render(<Calculator />, rootElement);

Congratulations! How do you feel now that you have climbed Mount Dhoom and thrown the ring into that sucker?

Now that you stand on the mountaintop, it’s time to look down and see what better paths up to the summit you could have taken. This is the the time, because as the living legend Professor Donald Knuth says: premature optimization is the root of all evil (i.e.: make it work first, then worry about making it work faster or cheaper).

So, one optimization that I see is that you could use operatorLast as the the regex to test both event.target.value and inputString, because event.target.value is 1 character long for all the operator strings, so inevitably it will match if it is an operator key.

As to your question about your render method, you’ve made one big booboo: unnecessary global variables. Basically, the only time I want a global variable to exist is if it is stand-in for data that could theoretically come from somewhere else, like an API to get quotes or photos from a server. In this case, the only place you use the arrays BUTTONS and OPERATIONSDECIMAL is in the render method. If you look back at your earlier code posted in this thread, you’ll see that what I said holds true-ish:

Except for the last button you define, #clear , you are only changing two pieces of data, and using one of those twice.

(I forgot that clear AND equals have different event handlers than all the other buttons - they have be rendered individually).

Since you are using React, it’s important to get the concept of “state” down. State is the minimal representation of what can’t be looked up by a function call, input by the user, or calculated from either of those. You can have state that represents your UI, and the buttons are such state. In the earlier version of the code, you had them hard-coded, which isn’t very clean because it isn’t DRY (dont-repeat-yourself) code. In this version, your code is still kinda “wet” (Wasteful Excessive Typing? I dunno, I made that one up) because you have multiple state representations (2) and an equal number of procedures to operate on those representations.

For each numberic, decimal, and operator button that you can summarize, you need to know:

  • the id as a string
  • the value as string that will be accessed as event.target.value

So, you will likely need an array of minimal state representations, where each index holds the info for one button. But what do you put into each index of the array? Well, you have two types of information, each of which referrable to by a name: “id” and “value”. So, if you have multiple pieces of named data for one enitity (the button), it’s probably best to use an Object. Using an Array can get annoying (who wants to remember if you put the id at outerArray[buttonIndex][0] or outerArray[buttonIndex][1]? It would be better to be able to address that data by name, semantically, like:
outerArray[buttonIndex].id or outerArray[buttonIndex].value

So, INSIDE render(), before the return, or in the constructor(as global as you want to get), I’d define an array called

buttons = [
  {id: "zero", value: "0"},
  {id: "one", value: "1"},
  {id: "two", value: "2"},
      ...
  {id: "add", value: "+"},
      ...
  {id: "divide", value: "/"},
  {id: "decimal", value: "."}
]; //your assignment syntax will vary depending on where you store this array.

You can see how one buttons.map() call will generate all your buttons EXCEPT clear and equals because they don’t follow the pattern re: click handlers. If you had a lot more variability in the click handlers, you could store some minimal represenation of WHICH handler to use for each object in the array, and in your map() call, pick the correct one based on that. I hope this helps you see how to think about state.