React code to build shopping cart

Question

I have built a basic shopping cart simulator. All it does is add and remove items between two different basket (i.e. aisle vs basket).

While working on the problem I started to think about the approach and would love to hear what people think.

1. Avoid initializing the state empty state in the constructor. However, I had to initialize shoppingCart: [] in componentDidMount hook to avoid type error. What is the better and cleaner approach?
2. Is it good practice to keep the logic simple in handleXXXClicks, separation of concern one for handling items and one for shoppingCart?
3. I feel aisleList and cartList bloated and I could have written itemComponent which is avoidable duplicate code.
4. Is there any better way to handle the item in setState? I couldn't think of using prevState in my scenario.

Use setState without initializing state:

componentDidMount() {
 // assuming data comes from API fetch
 let items = ["banana", "eggs", "bread", "apple"];
 this.setState({
 items,
 shoppingCart: [] // had to set this empty to avoid typeerror 
 });
 }

Handle handleClick and setState:

handleCartClick = event => {
 let newList = this.state.shoppingCart.filter(item => {
 return item !== event.target.value;
 });
 this.setState({
 items: [...this.state.items, event.target.value],
 shoppingCart: [...newList]
 });
 };

Gist link

Demo link

Answer 1

Some Tips

• Avoid using constructor.
• Avoid using .bind(this)
• Destructuring will help you to make cleaner code.

Questions

1. Avoid initializing the state empty state in the constructor. However, I had to initialize shoppingCart: [] in componentDidMount hook to avoid type error. What is the better and cleaner approach?

You can have 2 aproaches to this

1. There is a way to avoid this, what you do is destruct the state in the variables you want and set then a default value of an array.

   state = {}
   render() {
    const {
    shoppingCart = []
    } = this.state
    ...
   }
   

   This way you don't need to check if shoppingCart is an array, and you only need to declare this.state as an object ( it can be just an empty object ).

   You can also do

   render() {
    const {
    shoppingCart = []
    } = this.state || {}
    ...
   }
   

   And now you don't even need state = {}

2. Because you are using arrow functions, you don't need .bind(this) and also you can avoid de constructor and just use

   state = {
    ...
   }
   

   This way you can declare shoppingCart and items and don't need to declare it in componentDidMount (wich is much better to understand) and now don't need to destruct the state.

   state = {
    shoppingCart = [],
    items = []
   }
   

   This way is good if the data you will get is always an array (or the type you specify) and another good reason to use this is that when other programmer look at the code, they will immediately know what is stored in that component state, but when you have too many things in the state, it can get confused.

This aproaches will solve some problems like using ternary to check if you have an state and then return the mapping or if you don't have, return an empty array.

render() {
 const {
 items = [],
 shoppingCart = []
 } = this.state
 let aisleList = items.map((item, index) => {
 return (
 <div key={index}>
 <span>{item}</span>
 <input
 type="checkbox"
 id={index}
 value={item}
 onClick={this.handleAileClick}
 />
 </div>
 );
 });
 let cartList = shoppingCart.map((item, index) => {
 return (
 <div key={index}>
 <span>{item}</span>
 <input
 type="checkbox"
 autoComplete="off"
 value={item}
 onClick={this.handleCartClick}
 />
 </div>
 );
 });
 ...
}

Looking at this, is much clean and easy to understand.

2. Is it good practice to keep the logic simple in handleXXXClicks, separation of concern one for handling items and one for shoppingCart?

You can do the way you want, but you need to see what you will do in the future, if both will always do the same, you can create one function for both and just send a different parameter, but if they will do some extra stuff, it's good to have then separated.

*(I'm not english native speaker and maybe I didn't understand exaclty what is this part about).

3. I feel aisleList and cartList bloated and I could have written itemComponent which is avoidable duplicate code.

Yes, they look so similar that you should create a component for both and pass the diference in props.

4. Is there any better way to handle the item in setState? I couldn't think of using prevState in my scenario.

Here is simple setState using a function with prevState. Note that the way you are doing it, you need to have event.persist(), for more info look here

handleCartClick = event => {
 event.persist();
 this.setState(prevState => {
 return {
 items: [...prevState.items, event.target.value],
 shoppingCart: prevState.shoppingCart.filter(item => {
 return item !== event.target.value;
 })
 };
 });
};

Overall

Your code looks ok, there is only one strange thing. You are using checkboxes and when you click, some get marked and other don't. I don't know if that was a test or you also wanted to solve the checkbox problem. Let me know and I can see what I can do.

I recommend you to start using some new things like I mentioned in the tips part, that help you alot and make your code cleaner.

Also have a look at hooks.
For me, using hooks makes alot of things easier, but also, the code looks cleaner.

I made a demo for you to test and see the new code. I changed some things.

• I changed handleCartClick but left handleAileClick the way it was before, so you can try useing prevState yourself.
• And also left the part of itemComponent for you.

The best way to learn, is doing it!

Hope I helped you!

• degr: why avoid using of contructor?
• Vencovsky: @degr You should think in a different way, Why should I use the constructor?
• degr: I should use constructor because it is common practice in all OOP languages where I can declare state of object
• Vencovsky: You would use contructor for things like .bind or declaring the state, but now you don't need to use .bind because arrow functions solve this and you can set the state outside this like a normal property of the class e.g. state = { anythingyouwant: something}. And also, now with hooks, using class component will desapear over time, and functional components can't have constructor.
• Vencovsky: I should use constructor because it is common practice in all OOP languages, Acctualy, you should use only necessary things, but now, you have better ways of doing the same thing, but in simpler way. I'm not saying you can't use contructor, but that is better to use other ways.