Front End Libraries Project - React Calculator

Code Feedback
1

Hey campers,

I recently finished the last project I needed to get the front-end libraries certification! It’s an immediate evaluation calculator made using React.

There are several decorative buttons :sweat_smile:

I’d love some feedback.

Here’s the link to the project on Codepen: https://codepen.io/LisaLoop/pen/xxdMQWd

I’m looking forward to doing the NEW responsive web design curriculum next!

2

Cool, congratulations.

Now, keep in mind, I am a really picky code reviewer…

The design …

It’s a little simplistic design, but it’s functional.

If I type a really big number, it overruns the display box.

The code …

You have everything in one component - that isn’t very React way to do things. One of the strengths of React is it makes it easy to compartmentalize. It’s like if you bought a high tech closet with adjustable and labelable shelves and cubby holes - and then just pulled out all the dividers and put all the clothes and shoes in one pile.

You do have a sub component, but you have them buried inside your One Component. “keypad” should be its own component, outside of Calculator and PascalCased. I think a more React-y structure would be:

Calculator --|
             |
             -- Display
             |
             -- KeyPad --|
                         |
                          -- Key

It may seem like overkill on something so small, but we practice these skills when it is small and manageable.

  const [display, updateDisplay] = React.useState(0);
  const [calculatorState, updateCalculatorState] = React.useState([Num(0)]);

Definitely nitpicky, but it’s more typical to use the verb “set” for the state setter. It took me a second when I was further down in the code to realize what it was. It sounds tiny (and it is) but when you have to read 1000s of lines of code, every tiny, tiny little thing matters.

This:

  const ids = [

I would have put outside the component. As it is, it has to rebuild and tear it down on each rerender.

  const keypad = ids.map((button, i) => (
    <button onClick={() => handleKeypadPress(i)} className={ids[i]} id={ids[i]}>
      {i}
    </button>
  ));

As mentioned, this should be a component. Generally, any time you return JSX, that should be a component. True, this isn’t returning, but still, it should be. If not, I would at least want to memoize that (React.memo) so it isn’t run every time we render.

  return (
    <>
      <div className="container">
      <div className="display" id="display">
        {display}
        </div>
        {keypad}

You have some non-standard indenting here. Again, it sounds like a tiny thing, but in the long run, it’s hugely important. One of your goals as a coder is readability. You want another coder to be able to figure it out as fast as possible. Things like variable naming, organization, and formatting (indenting) are huge helps. In “real” coding in a local IDE, you can have it format it for you automatically. In CodePen, you can use the pulldown menu in the upper right of the JS pane, “Format JavaScript”.

Also, why is the div wrapped in a Fragment? In this case, that Fragment doesn’t do anything.

<button onClick={() => handleClear()} className="clear" id="clear">

Usually, if you are not passing parameters, you don’t have to wrap it in an arrow function - you’re just passing a function to call your function - that is redundant - just pass the function:

<button onClick={handleClear} className="clear" id="clear">

The exceptions are if you need to pass in parameters other than the default ones that onClick passes, or if you your function has other, optional parameters that the default ones from onClick might trigger (I’ve had that happen with other libraries.)

const end = (tokens) => tokens[tokens.length - 1];

I don’t like this function name. When I read it earlier in the code, I had to find the function to figure out what it does. The name should tell me what it does. How about “getLastElement”? And why make it specific to tokens? Why not:

const getLastElement = (array) => array[array.length - 1];

I should also point out that JS now has at:

const getLastElement = (array) => array.at(-1);

But at that point, I think the function is more wordy than useful.

const Op = (op) => ({ type: "op", value: op });
const Num = (num) => ({ type: "number", value: `${num}` });

Again, bad variable names. And not only do I not know what they do, but I assume that they are some class or something because they are PascalCased. Only classes, components, and enums should do that. When I saw Num(0) in the code above, at first I thought it was the Number object wrapper.

I might call this “createNum”.

const replaceLast = (list, item) => [...list.slice(0, -1), item];

That’s a good variable name - I know exactly what it does from the name.

const isGoddamnedTerribleFakeNegativeNumberOperator = (token) =>
  token.type === "number" && token.value === "-0";

OK, I get the frustration. It made me chuckle. It reminds me of a code review I did for a junior. He put some snarky comment in the code out of frustration. I had to ask him to remove it - it’s company code, not his therapy blog.

Yeah, negative 0 can be frustrating.

if (isZero(lastToken) && digit != ".") {

Don’t use != or ==. They can lead to unwanted type coercion. This specific case wouldn’t, but it’s also a little slower.

Cool, good job. Again, keep in mind that I’m a very picky reviewer. Have fun on the next thing.

1 Like
3

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.