Return to Answer

• global variables - any variable not declared with var (inside a function), let or const e.g. $shoppingCartContainer will be stored as a global variable - on window. In a small application that likely wouldn’t be an issue but it is a good habit to avoid those - especially when you get into a larger application and the same name is used to hold different data.

• variables starting with dollar symbol when using jQuery, that can be a sign that a variable represents a jQuery collection, so if $shoppingCartContainer is not a jQuery collection, somebody trying to extend your code might think it is a jQuery collection and use jQuery methods on it, which would lead to an error. See more in this post.

• default to using const for variables that don’t need to be reassigned - e.g. $quantity and then switch to let if it is determined that reassignment is necessary.

• setting length of new array to zero - when would the length of a new array be something other than zero?

  var cart = [];
  

   cart.length = 0;
  

• use spread operator - since ecmascript-6 features like arrow operators are used then instead of calling:

  Array.prototype.forEach.call(addToCart, function(element) {
  

  The collection can be put into an array with the spread operator:

   [...addToCart].forEach( function(element) {
  

• I see this line

  window.setTimeout(function() {}, 1000); // prevent flickering
  

  but I am not convinced that calling an empty function after 1 second will prevent flickering. How did you reach that conclusion?

• global variables - any variable not declared with var (inside a function), let or const e.g. $shoppingCartContainer will be stored as a global variable - on window. In a small application that likely wouldn’t be an issue but it is a good habit to avoid those - especially when you get into a larger application and the same name is used to hold different data.

• variables starting with dollar symbol when using jQuery, that can be a sign that a variable represents a jQuery collection, so if $shoppingCartContainer is not a jQuery collection, somebody trying to extend your code might think it is a jQuery collection and use jQuery methods on it, which would lead to an error. See more in this post.

• default to using const for variables that don’t need to be reassigned - e.g. $quantity and then switch to let if it is determined that reassignment is necessary.

• setting length of new array to zero - when would the length of a new array be something other than zero?

  var cart = [];
  

   cart.length = 0;
  

• use spread operator - since ecmascript-6 features like arrow operators are used then instead of calling:

  Array.prototype.forEach.call(addToCart, function(element) {
  

  The collection can be put into an array with the spread operator:

   [...addToCart].forEach( function(element) {
  

• I see this line

  window.setTimeout(function() {}, 1000); // prevent flickering
  

  but I am not convinced that calling an empty function after 1 second will prevent flickering. How did you reach that conclusion?

  EDIT: It might be better to wait for the DOMContentLoaded instead of the load event before starting to interact with the DOM. And as I mentioned above, the jQuery DOM ready function is available for this.

  If the flickering you are seeing is the shopping cart container, then add style="display: none" to that HTML element instead of waiting for the DOM to be loaded to hide it. Then it can still be displayed dynamically via JS. If that was in the CSS then it would make displaying it more challenging.

Why not register event handlers like the others:

Why not simplify the event handler registration to the jQuery syntax, like the code already does for other elements (e.g. with class name plus-btn, minus-btn). Instead of looping through all elements with the class name .AddToCard like the code does above, just use this:

Overall this code isn’t bad but could be simplified quite a bit The biggest thing I noticed is that jQuery is used for a few things but could be used a lot more - e.g. selecting DOM elements and manipulating them, adding event handlers, etc. There are some who believe jQuery isn't needed (see sites like youmightnotneedjquery.com. Some of the advice below comes after I read this article Stop Writing Slow Javascript - I know it is from a few years ago but the concepts are still relevant. It also touches on the usage of jQuery in todays web.

• functions doing more than necessary? I see the function CreateAddToCartEventListener() calls updateCartCount(). Is that really necessary when adding an event listener?

• scoping and DOM ready wrap JS in a DOM ready callback (I.e. $(function() {}) - formerly .ready()) or else an IIFE to limit scope of variables

• utilize jquery more As mentioned above, it can be used a lot more - e.g. instead of:

   $clearAll = document.getElementById("clearAll");
  

  Just select it with the jquery function:

   $clearAll = $("#clearAll");
  

Note that such a variable would then contain a jQuery collection and not a single DOM reference anymore, so with that one can call jquery methods on the collection like .hide() and .show()

Overall this code isn’t bad but could be simplified quite a bit. The biggest thing I noticed is that jQuery is used for a few things but could be used a lot more - e.g. selecting DOM elements and manipulating them, adding event handlers, etc. There are some who believe jQuery isn't needed (see sites like youmightnotneedjquery.com. Some of the advice below comes after I read this article Stop Writing Slow Javascript - I know it is from a few years ago but the concepts are still relevant. It also touches on the usage of jQuery in todays web.

• functions doing more than necessary? I see the function CreateAddToCartEventListener() calls updateCartCount(). Is that really necessary when adding an event listener?

• scoping and DOM ready wrap JS in a DOM ready callback (I.e. $(function() {}) - formerly .ready()) or else an IIFE to limit scope of variables

• utilize jQuery more As mentioned above, it can be used a lot more - e.g. instead of:

   $clearAll = document.getElementById("clearAll");
  

  Just select it with the jQuery function:

   $clearAll = $("#clearAll");
  

Note that such a variable would then contain a jQuery collection and not a single DOM reference anymore, so with that one can call jQuery methods on the collection like .hide() and .show()