Return to Answer

• 1

• 29.5k
• 16
• 45
• 201

• 431
• 2
• 3

As a preface: You're code works good, is readable and does its job efficiently. The following review, though long, doesn't mean there is anything substantially wrong with it :-)

General comments

In the constructor, you're binding the event handlers to this. However, since you declare these event handlers as arrow functions, they are already bound to the current instance and you can therefore skip binding them explicitly.

The functionality to only recalculate on blur and swap is nice and well implemented, but it is probably not needed for efficiency's sake, since the recalculation is (at the moment at least) very cheap.

generateTable is a component, but it's not written the same way as other components (it's not capitalized and declared as const). I also wouldn't mind you incorporating the logic of this component directly into Output, as this component would then still be of readable length and concern itself only with one aspect.

You have added a key prop to every React component you render. However, this is only necessary if you render a collection of components (i.e. if you declare an array of either DOM or React elements to be rendered). So it is only needed in the ResultTable (line 218).

You can insert JavaScript variables in text with the same {var} notation you use for attribute values and element children, so {`Number ${props.num}: `} on line 133 can be replaced by Number {props.num}:

In your components NumberBox and NumberPrompt: onChange and handleChange mean very different things in your code. However, onChange is such a common sight that is has a clear meaning attached to it. I would therefore strongly discourage using a name close to it (handleChange) for something different ("committing" the value on blur) than it suggests. I think you already sensed as much, seeing your comment next to the props. Why don't you use a separate onBlur handler, so that the prop names align with the handler names?

Functional components

You could destructure the props received by a functional component as its argument. This allows you to more easily read the "signature", what props it depends on, and allows you to save some typing:

function SwapButton({handleSwap}) {
 return (
 <button key='swap' type='button' onClick={handleSwap}>Swap</button>
 );
}

You could also opt for writing components as arrow-functions, as you already do in generateTable. Especially since you 'only' return JSX in a lot of components, this can make your component definitions shorter:

const SwapButton = ({handleSwap}) => <button type='button' onClick={handleSwap}>Swap</button>

Result components

In my opinion, the splitting into ResultTable, ResultHead and ResultRow results in more cognitive overhead than just simply writing:

function ResultTable({data}){
 return (
 <table id='result'>
 <thead>
 <tr>
 <th>Operation</th>
 <th>Result</th>
 </tr>
 </thead>
 <tbody>
 {data.map(({key, op, num}) => (
 <tr key={key}>
 <td>{op}</td>
 <td>{num}</td>
 </tr>
 ))}
 </tbody>
 </table>
 );
}

This way, you clearly see how the HTML table is constructed and which information ends up where. Note also the same destructuring of the argument inside map.

### Naming

The hardest part of any programming task. These are my personal opinions:

• Your component and function names generally don't use abbreviations and are very readable. I would extend this to your state and rethink the names num1 and doAdd.
• I didn't understand initially what was meant by box. Now that I know it, I think box is not the right word. Since this state holds the immediate value of the input fields, I'd opt for something like inputValue1.
• While it is technically true that doAdd: false (suggesting an action) prevents any addition from being done, I'd opt for something like showAdditionResult: false (suggesting a state).