1
\$\begingroup\$

I have been working on a beginner project, learning React and Typescript. The project is an expense manager application and I have tried splitting most of my applications into simple short components. For the EditExpensePage component I have connected it to the Redux store for the app, in order to be able to access the state and to dispatch actions.

I would like to get some feedback on the code I've written, and ways I could improve it.

The Expense interface :

export interface Expense {
    id: string;
    description: string;
    note?: string;
    amount: number;
    createdAt: number;
}

The EditExpensePage :

import * as React from "react";
import { RouteComponentProps } from "react-router";
import * as Redux from 'redux';
import {withRouter} from 'react-router';
import {connect} from 'react-redux';
import { Expense } from "../types/Expense";
import { AppState } from "../Store/configureStore";
import { compose } from "redux";
import ExpenseForm from "./ExpenseForm";
import { AppActions } from "../types/Actions";
import { addExpense, removeExpense } from "../Actions/expenses";

type PathParamsType = {
  id: string;
};
//component own props
type PropsType = RouteComponentProps<PathParamsType> & {
  id: string;
};

interface StateProps {
    expenses: Expense[]
}

interface DispatchProps{
    addExpense : (expense : Expense) => any,
    removeExpense : (expenseId : string) => any
}

type Props = PathParamsType & PropsType & StateProps & DispatchProps;

function GetSelectedExpense(id : string, expenses : Expense[]){
    for (var i=0 ; i<expenses.length; i++){
        if (expenses[i].id === id)
        {
            return expenses[i];
        }
    }
}

const EditExpensePage: React.FunctionComponent<Props> = (props): any => {
    let selectedExpense = GetSelectedExpense(props.match.params.id, props.expenses)
    return (
    <ExpenseForm description={selectedExpense?.description} amount={selectedExpense?.amount} onSubmit={(expense) => {

        for (let i = 0; i< props.expenses.length ; i++){
            if (props.expenses[i] === selectedExpense)
            {
                props.removeExpense(selectedExpense.id);
                props.addExpense(expense);
            }
        }

    }}/>
 )
};

const mapStateToProps = (state : AppState) :StateProps => {
    return({
        expenses : state.expenses
    })
}

const mapDispatchToProps = (dispatch : Redux.Dispatch<AppActions>) : DispatchProps => {
    return {
        addExpense : (expense : Expense) => dispatch(addExpense(expense)),
        removeExpense : (id : string) => dispatch(removeExpense(id))
    }
}

export default compose(
    withRouter,
    connect(mapStateToProps, mapDispatchToProps))(EditExpensePage);
```
\$\endgroup\$
1
\$\begingroup\$

I've noticed a few things, which I'm going to elaborate in the following.

Interfaces

First off, your interface Expense. It looks good so far, what I'd like to see is something like a structure in it, let it be alphabetically sorted or sort it optionals first or both, something like:

export interface Expense {
    amount: number;
    createdAt: number;
    description: string;
    id: string;
    note?: string;
}

It increases readability and allows others to quickly parse all existing keys (it's a bit nitpicking here, but it can have a huge effect).

Next up:

type PathParamsType = {
  id: string;
};
//component own props
type PropsType = RouteComponentProps<PathParamsType> & {
  id: string;
};

interface StateProps {
    expenses: Expense[]
}

interface DispatchProps{
    addExpense : (expense : Expense) => any,
    removeExpense : (expenseId : string) => any
}

type Props = PathParamsType & PropsType & StateProps & DispatchProps;

My two pain points here are, PathParamsType as well as DispatchProps. PathParamsType is not speaking for it self, try to give variables or interfaces a very consice name. Also I thought, you accidentially declared { id: string; } twice. There is some kind of repetition that could have been avoided. Please consider the following:

interface WithId {
    id: string;
}

/* Maybe name ComponentProps to something more speaking. */
type ComponentProps = RouteComponentProps<WithId> & WithId;

Also, you are declaring a props type below, why didn't you merge all in the first place. Something along the lines like: type Props = RouteComponentProps<WithId> & WithId /* & so on ... */;.

!!Side note!! Avoid stuff like: type Foo = { bar: string; foo: number; } we have interfaces for that.

Your function typing seems also a bit odd to me.

interface DispatchProps{
    addExpense : (expense : Expense) => any,
    removeExpense : (expenseId : string) => any
}

any seems in appropriate here. If you don't return a value from a function use void. If that function never returns (e.g. you quit the application inside that function), use the keyword never. But please don't use any.

To sum the interface section up, your code from above could have looked a bit like this:

interface WithId {
    id: string;
}

interface StateProps {
    expenses: Expense[];
}

interface DispatchProps{
    addExpense : (expense : Expense) => void;
    removeExpense : (expenseId : string) => void;
}

type Props = RouteComponentProps<WithId> & WithId & StateProps & DispatchProps;

React Component

Let's get to the component itself.

const EditExpensePage: React.FunctionComponent<Props> = (props): any => {
 /* ... */
}

The same as in the DispatchProps interface, a function does not return any. You actually broke the type system here. The following is fully compliant to your function/ variable typing.

const IamBroken: React.FunctionComponent<Props> = (props): any => 5;

This works as IamBroken should be a functional component but the function itself returns any and any is 5 (hopefully that explanation helps, at least a bit). What you would have wanted instead is:

const IamNotBroken = (props): React.FunctionComponent<Props> => 5; /* TypeScript: ERROR!!!! */

To apply the above pattern to your component (two solutions are available):

/* Variant 1 */
const EditExpensePage = (props: Props) => {
 /* ... */
}

/* Variant 2 */
const EditExpensePage: React.FunctionComponent<Props> = (props) => {
 /* ... */
}

Both solutions are totally fine, if you ask me though, I prefer the first one, it's sleek and if needed the compiler is able to infere the React.FunctionComponent part.

Component internals, functional programming

Inside your component I've noticed a few things as well. let selectedExpense = GetSelectedExpense(props.match.params.id, props.expenses). From what I have seen, that you never re-assign selectedExpense. Therefore it's better to keep such variables as constants: const selectedExpense.

I see what you want to achieve with the GetSelectedExpense function. But there are better way's to achieve this in TypeScript/JavaScript. Have a look at Arrays over at MDN. There are a few helpful methods for overall array handling. What you are looking for is something like find. (Side Note: There is a powerful concept that is called destructuring, and it helps you to avoid to write props. over and over again).

const { expenses, match } = props; /* Concept of destructuring, see my side note above */
const selectedExpense: Expense | undefined = expenses.find(expense => expense.id === match.params.id);

The onSubmit function could be improved in the following way. First, you want to avoid inline array functions as props (many linters are warning about possible performance issues). While that seems to be true, it also doesn't add to readability as well. Have a look at the following (I'm making use of the useCallback hook):

const { expenses, selectedExpense, removeExpense, addExpense, match } = props;
const selectedExpense: Expense | undefined = expenses.find(expense => expense.id === match.params.id);

const onSubmit = useCallback((expense) => {
    expenses.filter(exp => exp.id === selectedExpense).map(exp => removeExpense(exp.id));
    addExpense(expense);        
}, [removeExpense, addExpense, selectedExpense]);

return (<ExpenseForm description={selectedExpense?.description} amount={selectedExpense?.amount} onSubmit={onSubmit}/>);

Last but not least, your compose thingy at the end. It seems wrong and should be like this (because compose is coming from redux which shouldn't be abused to chain higher order components):

export default withRouter(connect(mapStateToProps, mapDispatchToProps)(EditExpensePage));
\$\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.