3
\$\begingroup\$

problem statement: using React and Redux build an interest calculator that, given a principal, rate of annual interest, and number of years, will display the total principal plus interest using the formula TOTAL = principal * (1 + (rate * years))

Any feedback welcome on the style, approach, etc.

<!doctype html>
<html><body>
<section></section>
<script src="https://unpkg.com/react/umd/react.production.min.js"></script>
<script src="https://unpkg.com/react-dom/umd/react-dom.production.min.js"></script>
<script src="https://unpkg.com/redux"></script>
<script src="https://unpkg.com/react-redux"></script>
<script src="https://unpkg.com/htm"></script>
<script>
const jsx = htm.bind(React.createElement);

function reducer(model={
    total: 0,
}, action={type:'', payload: null}){
    let store;
    switch(action.type){
    case 'TOTAL_COST':
        store = { total: action.payload.total };
    break;
    default:
        store = model;
    }
    return store;
}
const actions = {
    updateTotal: function({ principal, years, rate }){
        return {
            type: 'TOTAL_COST',
            payload: {total: principal * (1 + (rate * years))}
        }
    }
};
const store = Redux.createStore(reducer);

class App extends React.Component{
    constructor(props){
        super(props);
        this.state = {
            principal: 1000,
            years: 7,
            rate: 0.025
        };
        this.submit = this.submit.bind(this);
        this.update = this.update.bind(this);
    }
    componentWillMount(){
        this.submit(new CustomEvent('init'));
    }
    update(e){
        //const val = e.target.value * 1;
        let { valueAsNumber, value, name } = e.target;
        const state = {...this.state};
        if(isNaN(valueAsNumber)){
            valueAsNumber = 0;
        }
        state[ name ] = valueAsNumber;
        this.props.dispatch( actions.updateTotal(state) );
        this.setState({[name]: valueAsNumber});
    }
    submit(e){
        e.preventDefault();
        this.props.dispatch( actions.updateTotal(this.state) );
    }
    //TOTAL = principal * (1 + (rate * years))
    render(){
        const { principal, years, rate } = this.state;
        return jsx`<form onSubmit=${ this.submit }>
<style>
label{display:block;}
</style>
<h2>interest calculator</h2>
<h1><label> total <span>${ this.props.total.toFixed(2) }</span> </label></h1>
<fieldset>
<label> principal <input name=principal onInput=${ this.update } type="number" min=1 defaultValue=${ principal } /> </label>
<label> years <input name=years onInput=${ this.update }type="number" min=1 max=300 defaultValue=${ years } /> </label>
<label> rate <input name=rate onInput=${ this.update } type="number" min=0 max=100 defaultValue=${ rate } step=0.0001 /> </label>
</fieldset>
 <button>calculate</button>
</form>`;
    }
}

const ConnectedApp = ReactRedux.connect((store)=>{return {total:store.total}})(App);

ReactDOM.render(
    jsx`<${ReactRedux.Provider} store=${ store }> <${ ConnectedApp } key=${ Date.now() }> <//> <//>`,
    document.querySelector('section')
);

requestAnimationFrame(()=>{
    console.log(`🚀 tests running → any failures will be shown below`);
    requestAnimationFrame(()=>{ console.log(`🍻 tests done → any failures are shown above`); });

    const { updateTotal } = actions;
    let result = updateTotal({principal: 5000, years: 5, rate: 0.025}).payload;
    let expected = 5625;
    console.assert(Math.round(result.total) === expected, `expected total ${ result.total } to be ${ expected }`, result);

});
</script>
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

Everything's on the front-end You're using HTM to interpret the JSX by putting the JSX through a template literal. It's quite an interesting option, but for a professional project, I think compiling the JSX beforehand in a build process would be a better choice:

  • It's very easy to make syntactical typos in the template literal, and you have to run the script in order to see them. If something gets conditionally rendered, you may not see problems with your syntax until the component gets rendered, which may be uncommon; the potential bugs will be significantly harder to encounter and fix. In contrast, if you put the JSX inside a JSX file, and use a syntax-aware IDE like VSCode, you'll be able to spot such mistakes on sight and fix them immediately.
  • Transpiling the script ahead of time also makes things much easier if you decide to make the project larger. For example, if you have component A and notice that something isn't working right with it, you could have a standalone A.jsx file that you could go to and debug. (In contrast, if you have, say, 200 or 400 or 1000 lines of code in a single file without a build process, it gets more difficult to navigate around than it should be)
  • Having standalone .js / .jsx files also allows for linting, which I consider to be absolutely essential to enforcing code style and quality, and which can turn difficult-to-debug runtime errors into trivially fixable compile-time errors.
  • It also allows you to transpile ES2020+ syntax down to ES2015 or ES5 or whatever you prefer, which can make code easier to read and write while remaining compatible with as many browsers as possible.

It's just an option to consider. If you're not happy at the amount of setup it requires, you can use boilerplate like create-react-app so that it's nearly ready to go out of the box.

Avoid let and reassignment When variable names are permitted to be reassigned, it can make code more difficult to understand at a glance, when you always have to keep in mind "This variable was declared with let, so I need to be on the lookout for when it may be reassigned." (Linting rule: prefer-const). See this post on softwareengineering about the topic. There are a number of other places in the code where you're using let where const can be used instead.

In the reducer, rather than reassign store and then return it at the bottom of the switch, you can return it immediately. (You could also consider avoiding the switch here, since it requires a lot of unnecessary boilerplate).

// When an argument list requires multiple lines,
// I think it's easier to read to put each argument on a separate line like this:
function reducer(
    model={total: 0},
    action={type:'', payload: null}
) {
    switch(action.type){
        case 'TOTAL_COST':
            return { total: action.payload.total };
        default:
            return model;
    }
}

or, if the reducer won't be expanded in the future:

function reducer(
    model={total: 0},
    action={type:'', payload: null}
) {
    return action.type === 'TOTAL_COST'
        ? { total: action.payload.total };
        : model;
}

this binding and mount actions You do:

constructor(props){
    super(props);
    this.state = {
        principal: 1000,
        years: 7,
        rate: 0.025
    };
    this.submit = this.submit.bind(this);
    this.update = this.update.bind(this);
    this.submit(new CustomEvent('init'));
}

There are 2 improvements that can be made here: first, consider using class fields to define the methods instead of using .bind in the constructor, to be more concise and avoid boilerplate. Secondly, populating the component initially with this.submit(new CustomEvent('init'));, an event that is passed only so that calling e.preventDefault on it later doesn't throw is weird. How about using optional chaining in submit instead of passing a parameter?

class App extends React.Component{
    state = {
        principal: 1000,
        years: 7,
        rate: 0.025
    };
    constructor(props){
        super(props);
        this.submit();
    }
    submit = (e) => {
        e?.preventDefault();
        this.props.dispatch( actions.updateTotal(this.state) );
    }
    update = (e) => {

You could also consider using functional components instead; React recommends using them instead of class components for new code - they make code a bit easier to understand and maintain.

Update You have:

update(e){
    let { valueAsNumber, value, name } = e.target;
    const state = {...this.state};
    if(isNaN(valueAsNumber)){
        valueAsNumber = 0;
    }
    state[ name ] = valueAsNumber;
    this.props.dispatch( actions.updateTotal(state) );
    this.setState({[name]: valueAsNumber});
}

When using React, seeing something like

state[ name ] = valueAsNumber;

should look scary, because it looks very close to mutating state, and state should never be mutated in React - see here for lots of articles on the subject. While your current method works, it may well be worrying at a glance. Consider using a more functional approach, to construct the new state all at once, without mutation:

update = (e) => {
    const { valueAsNumber, value, name } = e.target
    const newState = {
      ...this.state,
      [name]: valueAsNumber || 0
    };
    this.props.dispatch( actions.updateTotal(newState) );
    this.setState(newState);
}
\$\endgroup\$
3
  • \$\begingroup\$ thanks @CertainPerformance The feedback related to what is unclear and clearer is useful. Much of the feedback echoes and refers specifically to community and framework norms without a reason. If you are able to explicitly and concisely articulate reasons for these comments that would be useful. I don't expect it however it's worth mentioning for those who aren't a part of the cultural React-Redux phenomenon. \$\endgroup\$
    – jimmont
    Commented Oct 10, 2020 at 4:33
  • \$\begingroup\$ I added a couple links, is there anything in particular that doesn't seem well-grounded enough? \$\endgroup\$
    – CertainPerformance
    Commented Oct 10, 2020 at 4:44
  • \$\begingroup\$ Nothing additional needed--thanks again. This is very helpful. It's worth noting that many of these assertions are simply preferences without substantive reason. To be clear, stating something is clearer is a reason, stating something is better without more detail or because someone else recommends is not a reason. These are logical fallacies until substantiated and are common methods of economizing and normalizing behavior in software development. There's no need to add more, this is simply a point of reflection. Thank you again, appreciate the time and articulation. It is useful. \$\endgroup\$
    – jimmont
    Commented Nov 16, 2020 at 6:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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