My brain is fried from looking from looking at this for so long. Is there a more efficient way to do this code block. We have the ability to use es6 code too if that helps.

First it tries to set quantity to itemTemplateIDQty.value if itemTemplateQty is not valid and itemTemplateIDQty is valid. Then it basically checks the opposite and sets quantity to itemTemplateQty.value. I'm thinking since in that else block, I am simply checking the exact opposite, I shouldn't need to retype all that code again. Also I'm wondering if I can remove a lot of code by using something like quantity = itemTemplateIDQty.value || 1;.

Update
I'll try to sum up the code more clearly. The default value for quantity should be 1 if all the checks fail. First if there are no classes of itemTemplateQty found and 1+ classes of 'itemTemplateQty' + itemId found then set quantity to the value of the first class found of 'itemTemplateQty' + itemId. If that check fails, then check if no classes of 'itemTemplateQty' + itemId are found and 1+ classes of itemTemplateQty are found, set quantity to the value of the first class of itemTemplateQty.

let itemId = 5;

let isDynamicRecommendation = false;

let itemTemplateQty =

        document.getElementsByClassName('itemTemplateQty').length > 0 ? document.getElementsByClassName('itemTemplateQty') : null;

    let itemTemplateIDQty =

        document.getElementsByClassName('itemTemplateQty' + itemId).length > 0 ? document.getElementsByClassName('itemTemplateQty' + itemId) : null;

    let quantity = 1;

    if (!isDynamicRecommendation) {

      if ((itemTemplateQty === null || itemTemplateQty.value == null || Number.parseInt(itemTemplateQty.value) <= 0) &&

         (itemTemplateIDQty !== null && itemTemplateIDQty.value !== undefined && itemTemplateIDQty.value !== null && Number.parseInt(itemTemplateIDQty.value) > 0)) {

           quantity = itemTemplateIDQty.value;

      } else if ((itemTemplateIDQty === null || itemTemplateIDQty.value == null || Number.parseInt(itemTemplateIDQty.value) <= 0) &&

        (itemTemplateQty !== null && itemTemplateQty.value !== undefined && itemTemplateQty.value !== null && Number.parseInt(itemTemplateQty.value) > 0)) {

          quantity = itemTemplateQty.value;

      }

    }

console.log("quantity: ", quantity);

.getElementsByClassName() returns an HTMLCollection, not a single element. You can use bracket notation [n] to get an element at index n of the collection returned.

One if statement can be used to evalaute !itemData.isDynamicRecommendation. One if..else..if can be used to evaluate itemTemplateQty[0] && itemTemplateQty[0].value <= 0 or itemTemplateIDQty[0] && itemTemplateIDQty[0].value <= 0.

To convert the string .value to an integer you can use + operator preceding +itemTemplateQty[0].value.value or alternatively use .valueAsNumber.

let itemData = 5;

let isDynamicRecommendation = false;



let itemTemplateQty = document.getElementsByClassName('itemTemplateQty')[0];

let itemTemplateIDQty = document.getElementsByClassName('itemTemplateQty' + itemData)[0];

let quantity = 1;

if (!isDynamicRecommendation) {

  if (itemTemplateQty && itemTemplateQty.value <= 0) {

    quantity = +itemTemplateQty.value;

  } else if (itemTemplateIDQty && itemTemplateIDQty.value <= 0) {

    quantity = +itemTemplateIDQty.value;

  }

}



console.log({quantity});

Without the HTML it is hard to workout what is wanted.

General point

• Use const for variables that do not change.


• Use functions to do common tasks, they simplify the code by reducing its size.


• Don't test more that you need to. If a is true then you know its not false, you don't then test if its not false.


• The statement if (!isDynamicRecommendation) { is always true and not needed. (A reminder that at codeReview code must not be example code, we assume this is production code)


• Use querySelector if you are after just the first instance of an element. It returns null if nothing is found.


• The code given should be as a function.


• 
  

  Names ar poor and too verbose. We read words by their shape especially when they are more than half a dozen characters long. Long names means it's hard to spot errors. This is especially bad in JS as it does not care if you use an undefined variable names.

  
  
  
  
  
  • 
    

    itemTemplateQty and itemTemplateIDQty if you have a single line with these variables repeated 7 times. Can you spot the error in the following? itemTemplateQty, itemTemplateQty, itemTemplateQty, itemTemplateIDQty, itemTemplateIDQty, itemTemplate1DQty, itemTemplateIDQty

    
    
    

    You know its an item. the template part is irrelevant, the only important part is to differentiate the ID (BTW Id should have a lowercase d), qty and qtyId would be better.

    
    
    

    Now spot the same error qty, qty, qty, qtyId, qtyId, qty1d, qtyId

    
    
  
  

  • itemId??? Have no idea what this actually is?

  
  
  
  

Example

By using two functions we can greatly reduce the complexity of the code. Removing the overly verbose names also make things easier on the eyes, and helps prevent brain fry.

function getQuantity() {

    const query = id => document.querySelector(".itemTemplateQty" + id);

    const valid = item => item && !isNaN(item.value) && Number(item.value) > 0.5;

    const qty = query(""), qtyId = query("5");

    return Math.round(valid(qty) ? 

         (!valid(qtyId) ? qty.value : 1) : 

         ( valid(qtyId) ? qtyId.value : 1));

}