sp92
sp92

Reputation: 987

How can I optimize my code in a way where only one method is required (if possible)?

I'm trying to optimize my code in a way where I wouldn't need 3 different methods (or however many I decide to include in the future) to safely store the users selection of a food type in the database whenever the user clicks the appropriate button.

I've tried numerous different ways but my code keeps breaking and I end up just chasing my own tail around. I feel like it's inefficient to optimize because each food type is different which means we would need different methods to store different foods in the database.

Side note: The modal works fine in terms of the user selection appearing one by one upon clicking on the food type desired. I want my code to be optimized in a way where in the future, I can add a delete feature if the user decides not to want that particular food anymore (but I can deal with that later).

(I'm a noob with react, forgive me please =D)

So is it possible to make my code way cleaner than it already is?

Here's my code:

import React, { Component } from 'react';
import { connect } from 'react-redux';
import Modal from 'react-modal';

import Aux from '../../../../hoc/Aux';
import FoodButton from '../FoodButtons/FoodButton';
import CheckoutButton from '../CheckoutButton/CheckoutButton';
import axios from '../../../../axios-foodChosen';

import { CLOSE_MODAL, OPEN_MODAL } from "../../../../store/action/NoNameAction";

class TacoTypes extends Component {
    state = {
        items: {
            cTacoClicked: false,
            cTaco: '',

            bTacoClicked: false,
            bTaco: '',

            cBurritoSelected: false,
            cBurrito: ''
        }
    }

    componentWillMount() {
        // for modal
        Modal.setAppElement('body');
    }

    chickenTaco() {
        // modal
        const cTacoSelected = "Chicken Taco";
        this.setState({cTacClicked: true, cTaco: cTacoSelected});

        // firebase
        let name = "Chicken Taco";
        axios.post('./chosen.json', { name })
            .then(response => {
                console.log("chicken taco check firebase");
            }).catch(error => {
            console.log(error);
        })
    }

    beefTaco() {
        // modal
        const bTacoSelected = "Beef Taco";
        this.setState({bTacoClicked: true, bTaco: bTacoSelected});

        // firebase
        let name = "Beef Taco";
        axios.post('./chosen.json', { name })
            .then(response => {
                console.log("beef taco check firebase");
            }).catch(error => {
            console.log(error);
        })

    }

    chickenBurrito() {
        // modal
        const cBurritoSelected = "Chicken Burrito";
        this.setState({cBurritoSelected: true, cBurrito: cBurritoSelected });

        // firebase
        let name = "Chicken Burrito";
        axios.post('./chosen.json', { name })
            .then(response => {
                console.log("chicken burrito check firebase");
            }).catch(error => {
            console.log(error);
        })

    }

    render() {
        return (
            <Aux>
                <FoodButton clicked={() => this.chickenTaco()} label={"Chicken Taco"}/>
                <FoodButton clicked={() => this.beefTaco()} label={"Beef Taco"}/>
                <FoodButton clicked={() => this.chickenBurrito()} label={"Chicken Burrito"}/>

                <CheckoutButton clicked={() => this.props.openModalRedux()}/>

                <Modal isOpen={this.props.isOpen}>
                    <p>
                        {Object.keys(this.state.items).map(key => (
                            <p key={key}>{this.state[key]}</p>
                        ))}
                    </p>
                    <button onClick={() => this.props.closeModalRedux()}>Close</button>
                </Modal>
            </Aux>
        );
    }
}

const mapStateToProps = state => {
    return {
        // props for modal
        isOpen: state.global.isModalOpen,
    }
};

const mapDispatchToProps = dispatch => {
    return {
        // Modal handlers
        openModalRedux: () => dispatch({type: OPEN_MODAL}),
        closeModalRedux: () => dispatch({type: CLOSE_MODAL})
    }
};

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

Upvotes: 0

Views: 68

Answers (1)

NonameSL
NonameSL

Reputation: 1475

I've narrowed it down to these main problems:

  • You're declaring constants in your state. It's bad practice and there's no reason for it. The state is supposed to include dynamic variables that change. You shouldn't include constants like food names in there.
  • Your code isn't DRY (Don't Repeat Yourself). You should find ways to generalize your code. For example, if your three methods do the same thing but with different foods, you could pass the food you want to do the action on as a variable and have one method that does the same.

Here's how your code could look like applying these principles (I didn't include all your code for comfort):

//Imports, blah blah blah
class TacoTypes extends Component {
    constructor(props) {
        //It's common practice to declare your state in the constructor.

        this.state = {
            selectedItems: [],
        };

        //Constant item names
        this.items = {
            chickenTaco: 'Chicken Taco',
            beefTaco: 'Beef Taco',
            chickenBurrito: 'Chicken Burrito',
        };
    }

    componentWillMount() {
        // for modal
        Modal.setAppElement('body');
    }
    selectFood(food) {
        this.setState(state => {
            //Dynamically setting state to avoid race conditions
            let selected = state.selectedItems;
            if(!selected.includes(food)) selected.push(food);
            return {selectedItems: selected};
        });

        //Do whatever with firebase here...
        //You can get the name using this.items[food]
    }

    render() {
        return (
            <Aux>
                {Object.keys(this.items).map(key => (
                    //Keeping our code DRY, we dynamically create the food buttons.
                    //React will throw a warning if we don't supply a unique key prop when dynamically creating variables using loops.
                    <FoodButton key={key} clicked={() => this.selectFood(key)} label={this.items[key]} />
                ))}

                <CheckoutButton clicked={() => this.props.openModalRedux()}/>

                <Modal isOpen={this.props.isOpen}>
                    <p>
                        {
                            //According to your comments I assume you want to display the items
                            //selected only, unlike what you originally coded:
                            this.state.selectedItems.map(key => (
                                <p key={key}>{this.items[key]}</p>
                            ))
                        ))}
                    </p>
                    <button onClick={() => this.props.closeModalRedux()}>Close</button>
                </Modal>
            </Aux>
        );
    }
}
//Include the rest of your code here

Notice the comments along the code as they explain my actions and will help you understand the rationale behind what I'm doing.

Upvotes: 1

Related Questions