3
\$\begingroup\$

I've started to fill my GitHub profile with small applications to educate and showcase my skills. However, I feel like I'm missing something essential, while creating these projects.

Could you take a look at this component, and advice me on how to refactor it? Its purpose is to get Coin Data from an API, and display the chart if the data is present :

import React from 'react';
import {connect} from 'react-redux';

import {formatCrypto} from '../../common/formating';

import ChartContainer from './chartContainer';

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

        this.state = {
            chartData: [],
            minmaxValues: {},
            isLinear: false,
        };
    };

    /***
     * @method getDerivedStateFromProps - defines if the chart should be updated, since useEffect
     * hook was giving a warning for async component rendering
     * @param props - nextProps of the app
     * @param state - nextState of the app
     * @return {null} - returns null when there is no updates for the current state
     */
    static getDerivedStateFromProps({coinsData}, state) {
        if (!coinsData) return null;

        let parsedChartData = [];

        if (coinsData) {
            parsedChartData = [{
                id: 'CoinMarketCap API response',
                data: [...coinsData.map((el) => {
                    return {
                        id: el.id,
                        name: el.name,
                        marketCap: '$' + formatCrypto(el.quote.USD.market_cap, 3),
                        volume: '$' + formatCrypto(el.quote.USD.volume_24h, 3),
                        priceChange: formatCrypto(Math.abs(el.quote.USD.percent_change_24h)) + '%',
                        x: el.quote.USD.market_cap,
                        y: el.quote.USD.volume_24h,
                        z: Math.abs(el.quote.USD.percent_change_24h)
                    }
                })]
            }];

        }

        return {chartData: parsedChartData};
    }

    swapChart = () => {
        this.setState((prevState) => ({isLinear: !prevState.isLinear}));
    };

    render() {
        const {chartData, isLinear} = this.state;

        return (
            <>
                <div className="d-flex justify-content-between">
                    <h2>Liquidity</h2>

                    <button
                        className={'btn btn-primary'}
                        type="button"
                        onClick={() => this.swapChart()}
                    >
                        Switch to {isLinear ? 'logarithmic' : 'linear'}
                    </button>
                </div>
                <ChartContainer chartData={chartData} isLinear={isLinear}/>
            </>
        )
    }
}

const mapStateToProps = (state) => {
    const {coinsData} = state.cmcListCallReducer;

    return {coinsData};
};
export default connect(mapStateToProps)(Liquidity); 

This component is a part of the largest app I've created so far. The app repo is here, in case you might be interested in a big picture. I will appreciate any kind of critics related to the project in general, since maybe my structure sucks more, than code styling.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to code review. You might also want to try to answer a few questions. \$\endgroup\$ – pacmaninbw Dec 27 '19 at 14:56
  • \$\begingroup\$ Sure thing, I would love to, but my skills aren't high enough yet at my opinion. :) \$\endgroup\$ – Animus Dec 27 '19 at 23:36
2
\$\begingroup\$

You're using redux for state management. The problem with mixing local state and redux state is that component render cycles are asynchronous, which oftentimes can cause finicky race conditions. If you're going to use redux, commit to it. If you aren't using any middlewares like redux-thunk, I highly suggest you do so. Dispatching actions and making api calls are ensured to be synchronous when using thunk actions

Your component is "responsible" for too much. It's a violation of the single responsibility principle, makes the component harder to reason, and is [far] more difficult to test. One of the main selling points of redux is the ability to tuck away how data is being passed. It lets you create a separation of concerns and will make your components much easier to reason and maintain. If I were in your position, I would create the following:

Make your api call inside a redux thunk action, and store the raw values in the store

Data calls and transfer are ensured to be synchronous. I would place the data in its entirety, just in case you need the raw data elsewhere

getCoinsData = () => async dispatch => {
    coinsData = await // api call
    const parsedCoinsData = coinsData.map((el) => { // Array#map already creates a new array. No need to spread it
        const {
            market_cap,
            volume_24h,
            percent_change_24h,
            percent_change_24h
        } = el.quote.USD
        return {
            id: el.id,
            name: el.name,
            marketCap: market_cap,
            volume24h: volume_24h,
            percentChange24h: percent_change_24h
        }
    })
    dispatch({
        type: SET_COINS_DATA,
        data: parsedCoinsData
    })
}

Create a selector to create the chart data

Creating a selector to parse/structure the data for your needs is an easy way to hide how the data is being created for your component to use and be able to hold all of the data in its unadulterated state in your redux store.

getChartData = state => {
    const { coinsData } = state.cmcListCallReducer;
    return [
        {
            id: 'CoinMarketCap API response',
            data: coinsData.map((el) => {
                const {
                    id,
                    name,
                    marketCap,
                    volume24h,
                    percentChange24h,
                } = el
                return {
                    id,
                    name,
                    marketCap: '$' + formatCrypto(marketCap, 3),
                    volume: '$' + formatCrypto(volume24h, 3),
                    priceChange: formatCrypto(Math.abs(percentChange24h)) + '%',
                    x: marketCap,
                    y: volume24h,
                    z: Math.abs(percentChange24h)
                }
            })
        }
    ]
}

And then your component would just look like this.

import React from 'react';
import { connect } from 'react-redux';
import ChartContainer from './chartContainer';

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

        this.state = {
            minmaxValues: {},
            isLinear: false,
        };
    };

    swapChart = () => {
        this.setState((prevState) => ({ isLinear: !prevState.isLinear }));
    };

    render() {
        const { isLinear } = this.state;
        const { chartData } = this.props
        return (
            <>
                <div className="d-flex justify-content-between">
                    <h2>Liquidity</h2>

                    <button
                        className={'btn btn-primary'}
                        type="button"
                        onClick={() => this.swapChart()}
                    >
                        Switch to {isLinear ? 'logarithmic' : 'linear'}
                    </button>
                </div>
                <ChartContainer chartData={chartData} isLinear={isLinear} />
            </>
        )
    }
}

const mapStateToProps = (state) => {
    return { 
        chartData: getChartData(state)
     };
};

export default connect(mapStateToProps)(Liquidity);
```
\$\endgroup\$
  • \$\begingroup\$ This is extremely elaborative, thank you a lot! But could you clarify 2 things shortly, just yes/no would be enough: 1) If I'm using redux-state I mustn't use state? So for my component, isLinear and minmaxValues from the state, should be taken from store instead. 2) The end result makes component almost for display only. Should I keep my components clean from any kind of computations? \$\endgroup\$ – Animus Dec 29 '19 at 21:20
  • \$\begingroup\$ 1. Depends on the context. In most cases, redux is the way to go when it comes to handing async data flow. 2. isLinear can definitely be kept in local state. It's very much encapsulated within the component. It has no interaction with any outside process. I don't know what minmaxValues is, but if it has any possible async behavior when interacting with your api call, then put it in your thunk action \$\endgroup\$ – Andrew Dec 30 '19 at 2:29
  • \$\begingroup\$ I see now, thank you for all the answers! \$\endgroup\$ – Animus Dec 30 '19 at 20:56

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.