Two solutions, which is best practice

Hey everyone, I have a simple counter element that can be increased and decreased, this is the way I chose to do it.

let numberOfItems = document.querySelector('.number-of-items')
let decreaseItem = document.querySelector('.decrease-item');
let increaseItem = document.querySelector('.increase-item');

decreaseItem.addEventListener('click', subtractItem)
increaseItem.addEventListener('click', addItem)

let counter = 0;

function addItem(){
    counter += 1;
    numberOfItems.innerHTML = counter;
}

function subtractItem(){
    if (counter === 0){
        alert("You don't have this item in your cart.")
    }
    else {
        counter -= 1;
        numberOfItems.innerHTML = counter;
    }
}

let numberOfItems = document.querySelector('.number-of-items')
let decreaseItem = document.querySelector('.decrease-item');
let increaseItem = document.querySelector('.increase-item');

decreaseItem.addEventListener('click', subtractItem)
increaseItem.addEventListener('click', addItem)

let counter = 0;

function addItem(){
    counter += 1;
    numberOfItems.innerHTML = counter;
}

function subtractItem(){
    if (counter === 0){
        alert("You don't have this item in your cart.")
    }
    else {
        counter -= 1;
        numberOfItems.innerHTML = counter;
    }
}

15 Replies

NazCodelandOP•3y ago

I seen this solution online, I am wondering, which is the better way of doing it

NazCodelandOP•3y ago

Joao•3y ago

I find your solution to be better, selecting the elements that are going to be modified and store them as global variables rather than running the selector each time. Also the onclick handler is out of fashion in favor of addEventListener, mainly because you can provide additional options to it (not in this case though). While I'm at it I would suggest not to use alert to display a notification, but instead use a separate function or make use of a library for that purpose. Also, the function subtractItem should be responsible for exactly that. Whether the item in question is on the cart or not should be validated before hand. I assume there's more to it than the example you are showing here?

NazCodelandOP•3y ago

As for my code, that was all I had so far. But, I see what you mean about making subtractItem do only what it's designed to do. I'm thinking, I would have to make another function that checks for the logic and from that function call the subtractItem function otherwise, return a msg saying you don't have the item in cart

MarkBoots•3y ago

you could combine the add/substract in one something like this

decreaseItem.addEventListener('click', ()=> { adjustItem(-1) })
increaseItem.addEventListener('click', ()=> { adjustItem(1) })

let counter = 0;
function adjustItem(amount){
  counter = Math.max(counter + amount, 0); // won't go below 0
  if(counter > 0){
     numberOfItems.innerHTML = counter;
  } else {
    alert("You don't have this item in your cart.")
}

decreaseItem.addEventListener('click', ()=> { adjustItem(-1) })
increaseItem.addEventListener('click', ()=> { adjustItem(1) })

let counter = 0;
function adjustItem(amount){
  counter = Math.max(counter + amount, 0); // won't go below 0
  if(counter > 0){
     numberOfItems.innerHTML = counter;
  } else {
    alert("You don't have this item in your cart.")
}

NazCodelandOP•3y ago

I was thinking about that too, that's neat. Is the Math a built-in module or library?

MarkBoots•3y ago

its in JS

NazCodelandOP•3y ago

ok thank you @joao6246 and @MarkBoots, I think I can combine both of your suggestions into a really nice compact modularized code, sounds fancy but I'll share in a bit

let items = 0;

function checkCart(){
    if (items == 0){
        alert("you don't have this item in your cart.");
    }
    else return true;
}

decreaseItem.addEventListener('click', () => {
    if (checkCart()){
        adjustItem(-1)
    }})

increaseItem.addEventListener('click', () => {adjustItem(1)})


function adjustItem(amount){
    items += amount;
    numberOfItems.innerHTML = items;
}

let items = 0;

function checkCart(){
    if (items == 0){
        alert("you don't have this item in your cart.");
    }
    else return true;
}

decreaseItem.addEventListener('click', () => {
    if (checkCart()){
        adjustItem(-1)
    }})

increaseItem.addEventListener('click', () => {adjustItem(1)})


function adjustItem(amount){
    items += amount;
    numberOfItems.innerHTML = items;
}

I like parts of this and also don't like some parts hmm I don't like how, checkCart() is above the eventListners, for some reason, I think it makes more sense if they are below it and together with the other function Another thing I don't like is, that I am checking for logic within the arrow function of the decreaseItem eventListner in terms of code layout/looks, I think this makes it most readable: one sec

MarkBoots•3y ago

how about this? https://codepen.io/MarkBoots/pen/MWGEXrY?editors=1010

Mark Boots

CodePen

MWGEXrY

...

NazCodelandOP•3y ago

yeah that makes sense and the code order is readable, I like that I tried doing something like this but it didn't work

decreaseItem.addEventListener('click', () => {adjustItem(-1)})
increaseItem.addEventListener('click', () => {adjustItem(1)})

let items = 0;

function adjustItem(amount){
    if (items == 0 && amount == -1){
        alert("you don't have this item in your cart.");
    }
    else {
        numberOfItems.innerHTML = items;
    }
}

decreaseItem.addEventListener('click', () => {adjustItem(-1)})
increaseItem.addEventListener('click', () => {adjustItem(1)})

let items = 0;

function adjustItem(amount){
    if (items == 0 && amount == -1){
        alert("you don't have this item in your cart.");
    }
    else {
        numberOfItems.innerHTML = items;
    }
}

the only time it would give that alert message is if the items == 0 and someone is trying to decrease I thought that would work but no beuno

MarkBoots•3y ago

you didnt do items += amount. put it in your else before the innerhtml

NazCodelandOP•3y ago

oh nice! I missed that, good catch

decreaseItem.addEventListener('click', () => {adjustItem(-1)})
increaseItem.addEventListener('click', () => {adjustItem(1)})

let items = 0;

function checkCart(amount){
    if (items == 0 && amount == -1){
        alert("you don't have this item in your cart.");
    }
    else return true;
}
function adjustItem(amount){
    if (checkCart(amount)) {
        items += amount;
        numberOfItems.innerHTML = items;
    }
}

decreaseItem.addEventListener('click', () => {adjustItem(-1)})
increaseItem.addEventListener('click', () => {adjustItem(1)})

let items = 0;

function checkCart(amount){
    if (items == 0 && amount == -1){
        alert("you don't have this item in your cart.");
    }
    else return true;
}
function adjustItem(amount){
    if (checkCart(amount)) {
        items += amount;
        numberOfItems.innerHTML = items;
    }
}

something like that seems clean...ish if checkCart has the job of checking to see if the cart is empty or not and returning true or false based on that If I am trying to make my code 'modular', If the cart is empty I would have to make another function that get's called to alert that it's empty something like this,

decreaseItem.addEventListener('click', () => {checkCart(-1)})
increaseItem.addEventListener('click', () => {checkCart(1)})

let items = 0;

function checkCart(amount){
    if (items == 0 && amount == -1){
        //call function that lets user know that this item is not in cart;
    }
    else adjustItem(amount);
}
function adjustItem(amount){
        items += amount;
        numberOfItems.innerHTML = items;
}

decreaseItem.addEventListener('click', () => {checkCart(-1)})
increaseItem.addEventListener('click', () => {checkCart(1)})

let items = 0;

function checkCart(amount){
    if (items == 0 && amount == -1){
        //call function that lets user know that this item is not in cart;
    }
    else adjustItem(amount);
}
function adjustItem(amount){
        items += amount;
        numberOfItems.innerHTML = items;
}

is this too much separation or if this is what they mean by modularizing, would this be a good solution ? @MarkBoots @joao6246

Joao•3y ago

That looks good, I think if we take things to the next level we start seeing more practical uses for these two functions. Let's assume that the items variable is no longer a simple number, but an array of objects in the cart. Actually let's call it cart, and I'll add a unique id for each item.

const cart = [
  {
    itemId: 'AAA123',
    name: 'Boots',
    price: 29.99,
    qty: 1,
  },
  {
    itemId: 'BBB456',
    name: 'Jacket',
    price: 30.00,
    qty: 0
  }
];

const cart = [
  {
    itemId: 'AAA123',
    name: 'Boots',
    price: 29.99,
    qty: 1,
  },
  {
    itemId: 'BBB456',
    name: 'Jacket',
    price: 30.00,
    qty: 0
  }
];

Each object has a name, price and quantity (how many times it appears in the cart). Now, you can no longer simply increase/decrease some variable by one, you have to: 1) Find the right item. 2) Increase/decrease the qty property on that item. 2.a) If item is not found in the cart, create the item and add it to the array with qty 1. 2.b) If item qty drops to 0, remove it from the array. Here's a baseline you can start with, with some hints. The weird comments are for documentation. If you put them on VSCode and hover over the function name it'll popup with the description. I've put a link to the array methods that you can use to find, add and remove items from an array.

/**
 * Searches the cart for the item provided, using the item's id
 * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findIndex
 * @param itemId An item's id to look for in the cart
 * @returns the index of the item in the cart array, or -1 if item is not found
 * @example isItemInCart('XYZ591'); // returns -1
 */
 function isItemInCart(itemId) {
  return cart.findIndex(i => i.itemId === itemId);
}

/**
 * Adds a new item to the cart. If the item is already in the cart, increase the qty instead.
 * @param item an object with an id, name and price properties.
 * @example addItem({ itemId: 'CCC789', name: 'Hat', price: 9.99 });
 */
function addItem(item) {
  const itemIdx = isItemInCart(item);

  if (itemIdx >= 0) {
    // increase item qty by 1
  } else {
    // add a new item with a qty of 1
    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push
  }
}

/**
 * Reduces an item's quantityby one. If it drops to 0, removes the item from the cart array.
 * @param itemId An item's id to look for in the cart and reduce its qty.
 * @example removeItem('AAA123');
 */
function removeItem(item) {
  const itemIdx = isItemInCart(item);

  if (itemIdx >= 0) {
    // reduce item qty by 1

    // if item qty goes down to 0, remove it
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice

  } else {
    alert(`Item ${item.name} is not in the cart!`);
  }
}

/**
 * Searches the cart for the item provided, using the item's id
 * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findIndex
 * @param itemId An item's id to look for in the cart
 * @returns the index of the item in the cart array, or -1 if item is not found
 * @example isItemInCart('XYZ591'); // returns -1
 */
 function isItemInCart(itemId) {
  return cart.findIndex(i => i.itemId === itemId);
}

/**
 * Adds a new item to the cart. If the item is already in the cart, increase the qty instead.
 * @param item an object with an id, name and price properties.
 * @example addItem({ itemId: 'CCC789', name: 'Hat', price: 9.99 });
 */
function addItem(item) {
  const itemIdx = isItemInCart(item);

  if (itemIdx >= 0) {
    // increase item qty by 1
  } else {
    // add a new item with a qty of 1
    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push
  }
}

/**
 * Reduces an item's quantityby one. If it drops to 0, removes the item from the cart array.
 * @param itemId An item's id to look for in the cart and reduce its qty.
 * @example removeItem('AAA123');
 */
function removeItem(item) {
  const itemIdx = isItemInCart(item);

  if (itemIdx >= 0) {
    // reduce item qty by 1

    // if item qty goes down to 0, remove it
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice

  } else {
    alert(`Item ${item.name} is not in the cart!`);
  }
}

NazCodelandOP•3y ago

@joao6246 sorry for the late reply, been MIA. Have some time now and I went wrote everything down as you provided, I did not know we can add description to code, that is awesome, thank you for showing me that. Thank you for going out of your way for the help and the additional links for me to look into. I will look into them and msg you back, thank you so much!

Unknown User•3y ago

Message Not Public

Gaming

Programming

Two solutions, which is best practice