5
\$\begingroup\$

This is a simple calculator app that involves entering two numbers and allowing them to be editable and swappable. Calculations are done on the numbers, so the calculations should only be done when the inputs are blurred. The boxes' values need to be updated when the values are swapped. However, they also need to be editable. Also, the results table should only show operations that are checked. I'd really appreciate any advice on style and efficiency. Here is a live version.

import React from 'react';
import ReactDOM from 'react-dom';
/* import './style.css'; */


/* Calculator operations */
const add      = (n1, n2) => { return n1 + n2; };
const subtract = (n1, n2) => { return n1 - n2; };
const multiply = (n1, n2) => { return n1 * n2; };
const divide   = (n1, n2) => { return n1 / n2; };


class Doc extends React.Component{
  componentDidMount(){
    document.title = "React Calculator"
  }

  render() {
    return(
      <Calculator />    /* Set num1= and num2= for initial numbers */
    )
  }
}


class Calculator extends React.Component {
  constructor(props) {
    super(props);

    this.state = {
      num1: props.num1,
      num2: props.num2,
      box1: props.num1 ? props.num1 : '',
      box2: props.num2 ? props.num2 : '',

      doAdd: true,
      doSub: true,
      doMul: true,
      doDiv: true,
    };

    this.handleBoxEdit = this.handleBoxEdit.bind(this);
    this.handleChange = this.handleChange.bind(this);
    this.handleSwap = this.handleSwap.bind(this);
  }

  /* Allows an input box to be edited without recalculating */
  handleBoxEdit = (event) => {
    const target = event.target;
    this.setState({
      [target.id]: target.value,
    });
  }

  /* Updates the state's numbers and operations on change */
  handleChange = (event) => {
    const target = event.target;
    const value = target.type === 'checkbox' ? target.checked : target.value;
    const name = target.name;

    this.setState({
      [name]: value,
    })

    console.log(`${name} was changed to ${value}`);
  }

  handleSwap = (event) => {
    this.setState({
      num1: this.state.num2,
      num2: this.state.num1,
      box1: this.state.num2 === null ? '' : this.state.num2,
      box2: this.state.num1 === null ? '' : this.state.num1,
    });
  }

  render() {
    return (
      <div>
        <Top />
        <NumberBox
          handleEdit={this.handleBoxEdit}
          handleSwap={this.handleSwap}
          handleChange={this.handleChange}
          box1={this.state.box1}
          box2={this.state.box2}
        />

        <Operations
          handleChange={this.handleChange}
          doAdd={this.state.doAdd}
          doSub={this.state.doSub}
          doMul={this.state.doMul}
          doDiv={this.state.doDiv}
        />
        <Output
          doAdd={this.state.doAdd}
          doSub={this.state.doSub}
          doMul={this.state.doMul}
          doDiv={this.state.doDiv}
          num1={this.state.num1}
          num2={this.state.num2}
        />
      </div>
    );
  };
}


function Top(props) {
  return (
    <div id='top'>
      <h1>Javascript Calculator</h1>
      <p>This is a calculator project to begin learning Javascript, CSS, and React.</p>
    </div>
  );
}


function NumberBox(props) {
  return (
    <form id='numbers-box'>
      <NumberPrompt key='box1' num='1' val={props.box1} handleChange={props.handleChange} handleEdit={props.handleEdit} />
      <NumberPrompt key='box2' num='2' val={props.box2} handleChange={props.handleChange} handleEdit={props.handleEdit} />
      <SwapButton key='swap' handleSwap={props.handleSwap} />
    </form>
  )
}

function NumberPrompt(props) {
  return (
    <label className='number-prompt'>
      {`Number ${props.num}: `}
      <input
        key={`prompt${props.num}`}
        id={`box${props.num}`}
        name={`num${props.num}`}
        type='number'
        onChange={props.handleEdit}  // Updates the box without changing the underlying value
        value={props.val}
        onBlur={props.handleChange}   // Updates underlying value when focus is lost to avoid each character causing recalculation
      />
    </label>
  );
}

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


function Operations(props) {
  return (
    <form>
      <fieldset id='operations'>
        <legend>Operations</legend>
        <OpBox key='addBox' name='doAdd' label='Add'      handleChange={props.handleChange} checked={props.doAdd} />
        <OpBox key='subBox' name='doSub' label='Subtract' handleChange={props.handleChange} checked={props.doSub} />
        <OpBox key='mulBox' name='doMul' label='Multiply' handleChange={props.handleChange} checked={props.doMul} />
        <OpBox key='divBox' name='doDiv' label='Divide'   handleChange={props.handleChange} checked={props.doDiv} />
      </fieldset>
    </form>
  );
}

function OpBox(props) {
  return (
    <div>
      <label>
        <input type='checkbox' name={props.name} checked={props.checked} onChange={props.handleChange} />
        <span>{props.label}</span>
      </label>
    </div>
  );
}


function Output(props) {
  if (isNaN(props.num1) || props.num1 === '' || isNaN(props.num2) || props.num2 === '') {
    return (
      <div id='output'>
        <span>Please enter a number for "Number 1" and "Number 2".</span>
      </div>
    );
  }

  if (!props.doAdd && !props.doSub && !props.doMul && !props.doDiv) {
    return (
      <div id='output'>
        <span>Please check at least one operation to perform.</span>
      </div>
    );
  }

  return generateTable(props);
}

const generateTable = (props) => {
  const tableData = [];
  const n1 = parseInt(props.num1);
  const n2 = parseInt(props.num2);
  if (props.doAdd)  tableData.push({key: 'addRow', op: 'Addition',       num: add(n1, n2)});
  if (props.doSub)  tableData.push({key: 'subRow', op: 'Subtraction',    num: subtract(n1, n2)});
  if (props.doMul)  tableData.push({key: 'mulRow', op: 'Multiplication', num: multiply(n1, n2)});
  if (props.doDiv)  tableData.push({key: 'divRow', op: 'Division',       num: divide(n1, n2)});

  return (
    <div id='output'>
      <ResultTable data={tableData} />
    </div>
  );
}

function ResultTable(props) {
  let rows = props.data.map(row => {
    return <ResultRow key={row.key} op={row.op} num={row.num} />
  });

  return (
    <table id='result'>
      <thead>{<ResultHead />}</thead>
      <tbody>{rows}</tbody>
    </table>
  );
}

function ResultHead() {
  return (
    <tr>
      <th>Operation</th>
      <th>Result</th>
    </tr>
  );
}

function ResultRow(props) {
  return (
    <tr>
      <td>{props.op}</td>
      <td>{props.num}</td>
    </tr>
  );
}


// ====================================================

ReactDOM.render(
  <Doc />,
  document.getElementById('root')
);
\$\endgroup\$
4
+100
\$\begingroup\$

As a preface: Your 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

This is 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).
\$\endgroup\$
  • \$\begingroup\$ I'm sorry, I thought I'd commented before and just realized I hadn't. Thank you so much for your thorough and thoughtful answer! As you can tell, I come back to it, especially to review destructuring. :) \$\endgroup\$ – BrainFRZ Sep 24 at 19:03
  • \$\begingroup\$ No worries and thanks for your response. Since you agree with the answer, would you mind accepting it? \$\endgroup\$ – gitcdn Sep 27 at 9:09
  • \$\begingroup\$ Sorry about that. I just assumed when I awarded the bounty that it accepted it automatically and I didn't notice it hadn't. \$\endgroup\$ – BrainFRZ Sep 28 at 16:25
1
\$\begingroup\$

This code looks good. There are only a couple improvements I would suggest.

Loop over operations

The code is somewhat repetitive for the operations, which goes against the Don't Repeat Yourself principle. Those operations could be added to an array and iterated over. You might have to get creative with calling functions but the operation functions could be added to that array as well.

Arrow function simplification

With arrow functions that only have a single statement that gets returned, the braces and return statement can be omitted. So the calculator operations, i.e.

/* Calculator operations */
const add      = (n1, n2) => { return n1 + n2; };
const subtract = (n1, n2) => { return n1 - n2; };
const multiply = (n1, n2) => { return n1 * n2; };
const divide   = (n1, n2) => { return n1 / n2; };

Can be simplified as such:

const add      = (n1, n2) =>  n1 + n2; 
const subtract = (n1, n2) => n1 - n2;
const multiply = (n1, n2) =>  n1 * n2; 
const divide   = (n1, n2) => n1 / n2; 

Simplify ternaries using logical OR

Instead of using a ternary operator to make a fallback value, like this

 box1: props.num1 ? props.num1 : '',

Logical OR can be used in the same fashion because of short-circuiting evaluation:

box1: props.num1 || '',
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.