React/Redux/Node/Express crud application with a React class component

Question

https://try.gitea.io/nadal/repository

Used this in a coding challenge after a job interview. I didn't get chosen, but I was wondering if it was because of the code quality in the project.

import React, { Component } from 'react';
import * as ExpenseActions from '../actions/ExpenseActions';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import { PropTypes } from 'prop-types';
import ExpenseTable from '../components/ExpenseTable';

export class ExpenseContainer extends Component {

    constructor(props) {
        super(props)
    }

    /**
     * Triggers the create action
     * @method
     *
     */
    createExpense = (expense) => {
        this.props.actions.CreateExpense(expense)
    }

    /**
     * Triggers the start edit action
     * @method
     *
     */
    startEditing = (id) => {
        this.props.actions.StartEditing(id);
    }

    /**
     * Triggers the cancel action
     * @method
     *
     */
    cancelEditing = (id) => {
        this.props.actions.CancelEditing(id);
    }

    /**
     * Triggers the edit action
     * @method
     *
     */
    editExpense = (expense) => {
        this.props.actions.UpdateExpense(expense);
    }

    /**
     * Triggers the delete action
     * @method
     *
     */
    deleteExpense = (expense) => {
        this.props.actions.DeleteExpense(expense);
    }

    render() {
        return (
            <div className="expense-container">
                <ExpenseTable
                    subTotal={this.props.expenses.subTotal}
                    total={this.props.expenses.total}
                    expenses={this.props.expenses.expenses}
                    createExpense={this.createExpense}
                    startEditing={this.startEditing}
                    cancelEditing={this.cancelEditing}
                    editExpense={this.editExpense}
                    deleteExpense = {this.deleteExpense}
                />
            </div>
        );
    }
}

ExpenseContainer.propTypes = {
    actions: PropTypes.object.isRequired,
    expenses: PropTypes.object.isRequired
}

function mapStateToProps(state, ownProps) {
    return {
        expenses: state.expenses
    }
}

function mapDispatchToProps(dispatch) {
    return {
        actions: bindActionCreators(ExpenseActions, dispatch)
    }
}

export default connect(mapStateToProps, mapDispatchToProps)(ExpenseContainer);

Aside the fact, I could have used a hook called useSelector and useDispatch. I am not sure how I could have made it better. The backend side, I am not too sure if there's anything wrong with it, because I am not too familiar with Node/Express. Try to be as nitpicky as possible.

Answer 1

Sorry you didn't get the position, but perhaps the interviewers/code reviewers were looking for something else or some very specific things.

You cannot, in fact, use useSelector or useDispatch, nor any other react hook as they are only compatible with functional components. Other than that this appears to be clean and readable code, although it does have some redundancy.

Suggestions

1. The constructor doesn't actually do anything, so it can be removed
2. All the class functions simply proxy props that are passed, so they, too, can be removed
3. This is debatable, but IMO you should explicitly map the actions to props that you will use, it makes it clearer what the component can do
4. The connect HOC's second parameter mapDispatchToProps automagically wraps each actionCreator with a call to dispatch as well
5. It appears this.props.expenses object shape is identical to the props ExpenseTable takes, so you can spread those in, more DRY

Nit-picky Suggestions

1. Try to be as precise/explicit when defining proptypes
2. Consistently use semi-colons
3. In react only Components are PascalCased, most other variables and objects should be normal camelCased
4. Destructure props to be more DRY, i.e. much less this.props.X, this.props.Y, this.props.Z. This one is debatable however as the argument against is that not destructuring them it is easy to see what is a prop versus state versus neither. I say you can just look to the top of the function where variables should normally be defined anyway to glean these details.

Component

import React, { Component } from 'react';
import { connect } from 'react-redux';
import { PropTypes } from 'prop-types';
import ExpenseTable from '../components/ExpenseTable';
import {
    CancelEditing,
    CreateExpense,
    DeleteExpense,
    StartEditing,
    UpdateExpense,
} from '../actions/ExpenseActions';

export class ExpenseContainer extends Component {
    render() {
        const {
            CancelEditing,
            CreateExpense,
            DeleteExpense,
            expenses,
            StartEditing,
            UpdateExpense,
        } = this.props;

        return (
            <div className="expense-container">
                <ExpenseTable
                    {...expenses}
                    createExpense={CreateExpense}
                    startEditing={StartEditing}
                    cancelEditing={CancelEditing}
                    editExpense={UpdateExpense}
                    deleteExpense={DeleteExpense}
                />
            </div>
        );
    }
}

ExpenseContainer.propTypes = {
    CancelEditing: PropTypes.func.isRequired,
    CreateExpense: PropTypes.func.isRequired,
    DeleteExpense: PropTypes.func.isRequired,
    StartEditing: PropTypes.func.isRequired,
    UpdateExpense: PropTypes.func.isRequired,
    expenses: PropTypes.shape({
      expenses: PropTypes.string.isRequired, // or number or whatever the specific type is
      subTotal: PropTypes.string.isRequired,
      total: PropTypes.string.isRequired,
    }).isRequired
};

function mapStateToProps(state) {
    return {
        expenses: state.expenses
    }
};

const mapDispatchToProps = {
    CancelEditing,
    CreateExpense,
    DeleteExpense,
    StartEditing,
    UpdateExpense,
};

export default connect(mapStateToProps, mapDispatchToProps)(ExpenseContainer);

A final suggestion would be to just convert ExpenseContainer to a functional component since it doesn't use any of the class-based lifecycle functions.

export const ExpenseContainer = ({
    CancelEditing,
    CreateExpense,
    DeleteExpense,
    expenses,
    StartEditing,
    UpdateExpense,
}) => (
    <div className="expense-container">
        <ExpenseTable
            {...expenses}
            createExpense={CreateExpense}
            startEditing={StartEditing}
            cancelEditing={CancelEditing}
            editExpense={UpdateExpense}
            deleteExpense = {DeleteExpense}
        />
    </div>
);

ExpenseContainer.propTypes = { ... };

function mapStateToProps(state) { ... };

const mapDispatchToProps = { ... };

export default connect(mapStateToProps, mapDispatchToProps)(ExpenseContainer);