React Calculator Finally Finished

Hi,

I have completed the React Calculator project. This project, I found really frustrating but I’m glad I persevered with it and am happy with the end result.

I redone the logic on this about three times to get it to work how I wanted and think it has aged me.

I wrote my own version of the eval() function as read it was a security risk, although probably would be alright in this situation as all input is limited to available keys, I do not know enough about security to judge and enjoyed getting my own version to work.

I found a design for a calculator on dribbble and recreated it using Sass. I did plan to add a dark/light mode and perhaps a backspace or percentage button but as this took me so long I am submitting it now so I can progress onto the next project, but maybe I will come back and add those features later on.

I really appreciate any feedback on my code anyone can provide.

Live Version:
https://rs-calculator.netlify.app/

Sandbox:

Repo:

Yup. We all know the feeling.

It seems to work well, hastily looking at the code…

          {buttons.map((btn, index) => {
            return (
              <Button key={index} btn={btn} input={input} setInput={setInput} currentNumber={currentNumber} setCurrentNumber={setCurrentNumber} operator={operator} setOperator={setOperator} equalPressed={equalPressed} setEqualPressed={setEqualPressed} />
            )

It is generally considered bad practice to use the index as the key. something to be avoided. It’s better to use some unique value from your data. How about the button id? And most people would probably use an implicit return here, but that gets into stylistic choices.

       <div className="keyboard">
         <!-- ... -->
       </div>

For organization, I would have put this section in it’s own component, mirroring the Display component. They should be hierarchically on the same level - but that’s getting nitpicky.

    const handleClick = (value, type) => {
        switch (type) {
            case 'reset':
                resetState();
                break;
            case 'negative':
                reverseNumber();
                break;
            case 'zero': // do not allow number to start with zero
                if (currentNumber === '0') {
                    break;
                }
            // eslint-disable-next-line
            case 'number':
                appendNumber(value);
                break;
            case 'operator':
                appendOperator(value);
                break;
            case 'decimal':
                addDecimal();
                break;
            case 'equals':
                calculateSum();
                break;
            default:
                return currentNumber;
        }
    }

There are a couple of oddities here. First of all, why are we returning anything from this? And if we return something sometimes, I think we would want to return something always, even if it’s just undefined.

And did you intend the fall-through after “zero”? For readability, if that’s what you want, I might do something like:

            // ...
            case 'zero': 
            case 'number':
                if (currentNumber !== '0') {
                    appendNumber(value);
                }
                break;
            // ...

To me that would be clearer - if that’s what you’re after.

setInput(`${newInput} <span class="operator"> ${op} </span> `);

I’m not sure how I feel about setting JSX in your state. I would rather set values and have the JSX constructed elsewhere. I don’t know that it is per se “wrong”, it just looks weird to me.

onClick={() => handleClick(btn.label, btn.class)}

It seems like that info could be stored on the button and gotten off the event in the handler. Then you wouldn’t have to wrap it in an anonymous function - that would be cleaner and more performant.

Oh well, it looks good. Those are just some hasty observations. Have fun on the next project.

1 Like

Thank you for taking time to review my code I really appreciate it.

I will avoid using index as a key in future.

I returned as default from the switch statement as I didn’t know what else to do if I’m honest as I’d covered all options but believed switch statements should have a default option.

The fall through was intentional although I didn’t like I had to hide the warning. This was to stop numbers starting with a 0 but I like your way better.

Sure, it should have a default, but does it need a return? You could just leave the default blank.

The fall through was intentional although I didn’t like I had to hide the warning. This was to stop numbers starting with a 0 but I like your way better.

Yeah, either way works. I just think what I suggested is more readable. I had to stare at yours for a minute before figuring out what was happening. I think it’s weird to have conditional breaks. I suppose you could do it more clearly if there wasn’t fall-through, but I think this is clearer.

1 Like