6
\$\begingroup\$

I have created a product configurator in VueJS and the whole functionality of it runs on one single function. I've put the word algoritm in the title in quotes because I don't really know if it is allowed that title. Anyhow, I have a feeling that the code I wrote is way to complicated and repeat a-lot of the same statements.

I honestly don't really know how to improve it to make it more efficient and readable. I read about something called DYI (Don't repeat yourself) which is actually a really good thought while programming. My thought is that I went too deep with the levels whilst it is not really necessary

How can I improve my code?

A little context: It regards a product configurator where a product has steps and each step has multiple options(Products). When clicked on a card the option should be added to the array. If the option is not in stock don't do anything at all and if the step allows multiple select. Append the option to the array anyway. If you read the comments I think it would make sense.

addToChosenOptions(step, option) {
    // Option is in stock
    if(option.stock === 1) {
        // Check if the step where the option belongs to, exists in the array of ChosenOptions
        if(this.chosenOptions.some(options => options[0].step.name === step.name)) {
            // The step allows multiple select
            if(step.allow_multiple) {
                // The option exists in the array of selected options
                if(this.optionExistInArray(option.name)){
                    // Loop through all the chosenOptions
                    this.chosenOptions.forEach(function(value, i) {
                        // The step equals the step given
                        if(value[0].step.name === step.name) {
                            // the options array length is 1 (Meaning its the last one in the array)
                            if (value[0].options.length === 1) {
                                // Delete the whole array instead of only the option
                                this.chosenOptions.splice(i, 1)
                            } else {
                                // The array's length is NOT 1 and thus only the options needs to be deleted
                                value[0].options.forEach(function(chOption, index) {
                                    // If the option equals the option given
                                    if(chOption.name === option.name) {
                                        // Delete the option from the array
                                        value[0].options.splice(index, 1);
                                    }
                                });
                            }
                        }
                    }, this);
                } else {
                    // If the option does not exists in the array
                    this.chosenOptions.forEach(function(value, i) {
                        // If the step name equals the given name
                        if(value[0].step.name === step.name) {
                            // Push the option to the right options array
                            value[0].options.push(option)
                        }
                    },this);
                }
            } else {
                // If the stap does NOT allow for multiple select
                this.chosenOptions.forEach(function(value, i) {
                    // If the step equals the given step
                    if(value[0].step.name === step.name) {
                        // Replace the current option with the new option
                        Vue.set(this.chosenOptions[i][0].options, 0, option)
                    }
                }, this);
            }
        } else {
            // If neither do exist at all in the array, Add the stap AND the option to the array
            this.chosenOptions.push([{step: step, options: [option]}])
        }
    }
},

Thank you

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

The original

Some comments on the code:

function addToChosenOptions(step, option) {
        -- if statement around the whole block. use the guard clause to return early
    // Option is in stock
    if(option.stock === 1) {
            -- too low level - extract it into a method
        // Check if the step where the option belongs to, exists in the array of ChosenOptions
        if(this.chosenOptions.some(options => options[0].step.name === step.name)) {
            // The step allows multiple select
            if(step.allow_multiple) {
                -- not clear why you are allowing multiple, but deleting. A bug?
                // The option exists in the array of selected options
                if(this.optionExistInArray(option.name)){
                    -- you are looping through just to modify one chosen option
                    // Loop through all the chosenOptions
                    this.chosenOptions.forEach(function(value, i) {
                        // The step equals the step given
                        if(value[0].step.name === step.name) {
                            // the options array length is 1 (Meaning its the last one in the array)
                            if (value[0].options.length === 1) {
                                    -- you are deleting inside of a for each loop - not good, because it will lead to bugs, because you skip the next element
                                // Delete the whole array instead of only the option
                                this.chosenOptions.splice(i, 1)
                            } else {
                                // The array's length is NOT 1 and thus only the options needs to be deleted
                                -- again looping through and removing values 
                                value[0].options.forEach(function(chOption, index) {
                                    // If the option equals the option given
                                    if(chOption.name === option.name) {
                                        // Delete the option from the array
                                        value[0].options.splice(index, 1);
                                    }
                                });
                            }
                        }
                    }, this);
                } else {
                        -- again looping through just to work with one element
                    // If the option does not exists in the array
                    this.chosenOptions.forEach(function(value, i) {
                        // If the step name equals the given name
                        if(value[0].step.name === step.name) {
                            // Push the option to the right options array
                            value[0].options.push(option)
                        }
                    },this);
                }
            } else {
                // If the stap does NOT allow for multiple select
                this.chosenOptions.forEach(function(value, i) {
                    // If the step equals the given step
                    if(value[0].step.name === step.name) {
                        // Replace the current option with the new option
                        Vue.set(this.chosenOptions[i][0].options, 0, option)
                    }
                }, this);
            }
        } else {
                -- why are chosen options a 2D array? you are always accessing the values as value[0].
            // If neither do exist at all in the array, Add the stap AND the option to the array
            this.chosenOptions.push([{step: step, options: [option]}])
        }
    }
},

The intermediate step

After my comments are addressed, this is what the code looks like:


hasStep(chosenOptions, stepName) {
  return chosenOptions.some(option => option.step.name === stepName)
}

getOptionByName(chosenOptions, name) {
    return this.chosenOptions.find(options => options.step.name === step.name)
}

deleteFromChosenOptions(chosenOptions, chosenOption) {
    return chosenOptions.filter(chOpt => chOpt === chosenOption)
}

excludeOptionsWithName(options, name) {
    return chosenOption.option.filter((chOption) => chOption.name !== option.name)
}

addToChosenOptions(step, option) {
  if(option.stock === 0) return

  if(this.hasStep(this.chosenOpitons, step.name)) {
    // The step allows multiple select
    if(step.allow_multiple) {
      // The option exists in the array of selected options
      if(this.optionExistInArray(option.name)){
        const chosenOption = getOptionByName(this.chosenOptions, step.name)
        // the options array length is 1 (Meaning its the last one in the array)
        if (chosenOption.options.length === 1) {
          this.chosenOptions = deleteFromChosenOptions(this.chosenOptions, chosenOption)
        } else {
          // The array's length is NOT 1 and thus only the options needs to be deleted
          chosenOption.options = excludeOptionsWithName(chosenOption.options, option.name)
        }
      } else {
        const chosenOption = getOptionByName(this.chosenOptions, step.name)
        chosenOption.options.push(option)
      }
    } else {
        const chosenOption = getOptionByName(this.chosenOptions, step.name)
            Vue.set(chosenOption.options, 0, option) // try if you can replace this with chosenOption.opitons = [option]
    }
  } else {
    // If neither do exist at all in the array, Add the stap AND the option to the array
    this.chosenOptions.push({step: step, options: [option]})
  }
}

Building to higher level

After some more refactoring and extraction of what you are doing into functions (notice, the comments are gone):


hasStep(chosenOptions, stepName) {
  return chosenOptions.some(option => option.step.name === stepName)
}

getOptionByName(chosenOptions, name) {
    return this.chosenOptions.find(options => options.step.name === step.name)
}

deleteFromChosenOptions(chosenOptions, chosenOption) {
    return chosenOptions.filter(chOpt => chOpt === chosenOption)
}

excludeOptionsWithName(options, name) {
    return chosenOption.option.filter((chOption) => chOption.name !== option.name)
}

deleteOptionFromStep(chosenOptions, optionName, stepName) {
  const chosenOption = getOptionByName(chosenOptions, stepName)

  if (chosenOption.options.length === 1) {
    this.chosenOptions = deleteFromChosenOptions(chosenOptions, chosenOption)
  } else {
    chosenOption.options = excludeOptionsWithName(chosenOption.options, optionName)
  }
}

addOptionToStep(chosenOptions, stepName, option) {
    const chosenOption = getOptionByName(chosenOptions, step.name)
  chosenOption.options.push(option)
}

overrideOptionAtStep(chosenOptions, stepName, option) {
    const chosenOption = getOptionByName(chosenOptions, step.name)
    Vue.set(chosenOption.options, 0, option)
}

addToChosenOptions(step, option) {
  if(option.stock === 0) return

  if(this.hasStep(this.chosenOpitons, step.name)) {
    if(step.allow_multiple) {
      if(this.optionExistInArray(option.name)){
        this.deleteOptionFromStep(this.chosenOptions, option.name, step.name)
      } else {
        this.addOptionToStep(this.chosenOptions, step.name, option)
      }
    } else {
        this.overrideOptionAtStep(this.chosenOptions, step.name, option)
    }
  } else {
    this.chosenOptions.push({step: step, options: [option]})
  }
}

The Final Result

After some unnesting (basically just negating the if condition to act as a guard clause):

addToChosenOptions(step, option) {
  if(option.stock === 0) return
  
  if(!this.hasStep(this.chosenOpitons, step.name)) {
    this.chosenOptions.push({step: step, options: [option]})
    return
  }
  
  if(!step.allow_multiple) {
    this.overrideOptionAtStep(this.chosenOptions, step.name, option)
    return
  }

// a potential bug - you might need to check if option does not exist at step
// stepHasOption(step.name, option.name)?
  if(!this.optionExistInArray(option.name)){
    this.addOptionToStep(this.chosenOptions, step.name, option)
    return
  }
  
  this.deleteOptionFromStep(this.chosenOptions, option.name, step.name)
}

Disclaimer: I have not been able to test this, so I cannot guarantee that I did the refactoring 100% perfectly. There might be some "this" missing in front of function calls and syntactic stuff like that. I tried my best to not let that happen. Hopefully you get the idea at least and refactor your code so it will work. Don't forget that the UI will break, because the format of chosenOptions changed -> before it was an array of lenght 1, now it is an object.

Basically the main idea when you have so much if statements is to think what your code does. Start from inside the nested if statements and slowly build higher and higher level concepts. In the end you can see that your code actually did 5 high level things. You could actually throw away the methods that I extracted and implement them again if you wanted (the ones that are called from addToChosenOption)

\$\endgroup\$

You must log in to answer this question.