Return to Answer

• 1

• 939
• 1
• 6
• 14

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);