1
\$\begingroup\$

I have the following code in JS (using last ES2020), the current code check if the type is 'asporto' or 'takeaway' and adding to current date hours from a setup fo type 'takeaway' or 'asporto' and if another setup called limiteOreTakeaway/Asporto is !== '00:00:00' I have to check if the current time is greater than the limiteOreTakeaway/Asporto, so I'm splitting the string limiteOreTakeaway/Asporto to Hours and Minutes and making my checks if the current date is > that limiteOreTakeaway/Asporto I'm adding to curDate the hours from setup + 24hours.

My code looks like this:

const curDate = new Date();
    if (this.carrello.tipo === 'asporto') {
      if (this.negozioSetup.limiteOreAsporto !== '00:00:00') {
        // controllo se l'orario limite è impostato (orariolimite != 00:00)
        const ore = parseInt(
          this.negozioSetup.limiteOreAsporto.split(':')[0],
          10
        );
        const minuti = parseInt(
          this.negozioSetup.limiteOreAsporto.split(':')[1],
          10
        );
        // controllo se l'ora di adesso ha superato l'orario limite
        if (curDate.getHours() > ore) {
          curDate.setHours(
            curDate.getHours() + this.negozioSetup.oreMinimeAsporto + 24
          );
         // se l'ora corrente è uguale controllo se i minuti limite hanno superato il limite
        }else if (curDate.getHours() === ore && curDate.getMinutes() > minuti) {
            // se l'orario corrente è maggiore di quello limite aggiungo 24 ore alla data corrente
            curDate.setHours(
              curDate.getHours() + this.negozioSetup.oreMinimeAsporto + 24
            );
        }
      } else {
        // orario limite disabilitato proseguo con l'aggiunta delle ore minime tra gli ordini
        curDate.setHours(
          curDate.getHours() + this.negozioSetup.oreMinimeAsporto
        );
      }
    } else if (this.carrello.tipo === 'takeaway') {
      if (this.negozioSetup.limiteOreTakeaway !== '00:00:00') {
        const ore = parseInt(
          this.negozioSetup.limiteOreTakeaway.split(':')[0],
          10
        );
        const minuti = parseInt(
          this.negozioSetup.limiteOreTakeaway.split(':')[1],
          10
        );
        if (curDate.getHours() > ore) {
          curDate.setHours(
            curDate.getHours() + this.negozioSetup.oreMinimeTakeaway + 24
          );
        }else if (curDate.getHours() === ore && curDate.getMinutes() > minuti) {
          curDate.setHours(
            curDate.getHours() + this.negozioSetup.oreMinimeTakeaway + 24
          );
        }
      } else {
        curDate.setHours(
          curDate.getHours() + this.negozioSetup.oreMinimeTakeaway
        );
      }
    }

I would like to simplify it. If it's possible, optimize it by using the last ES2020 available tools.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?, as well as How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Nov 16, 2020 at 15:44
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. For now, considering the answer has already followed your edit, nothing needs to be done. But please don't do it again. It can get messy really fast. \$\endgroup\$
    – Mast
    Nov 16, 2020 at 17:27

1 Answer 1

3
\$\begingroup\$

Use functions instead of repeating yourself The only difference between the tipo === 'asporto' branch and the tipo === 'takeaway' branch is the property name that gets passed into setHours: either .oreMinimeAsporto or .oreMinimeTakeaway. Create a function that takes that value as a parameter, then call it with either .oreMinimeAsporto or .oreMinimeTakeaway instead. That will about halve the size of your code.

const curDate = new Date();
const { tipo } = this.carrello;
if (tipo === 'asporto' || tipo === 'takeaway') {
    const casedProp = tipo[0].toUpperCase() + tipo.slice(1);
    changeDate(
        currDate,
        this.negozioSetup['oreMinime' + casedProp],
        this.negozioSetup['limiteOre' + casedProp],
    );
}

Split only once and destructure to grab the ore and minti values at once. You can also use Number instead of parseInt(str, 10), and use .map to apply the transformation to both elements at once:

const [ore, minuti] = this.negozioSetup.limiteOreAsporto.split(':').map(Number);

Call functions once rather than multiple times when possible - you don't need to call getHours every time, better to call it once and store the result in a variable.

const currHours = curDate.getHours();
const currHoursWithOffset = currHours + oreMinime;

Return early to avoid indentation hell - reading lots of {} blocks can make code more confusing to read than it needs to be. In the case of 00:00:00, consider running the one line that's needed and then returning, rather than having a large if/else.

The branches of currHours > ore and curDate.getHours() === ore && curDate.getMinutes() > minuti also contain the same logic inside the branches, so combine them into one if statement:

const changeDate = (date, oreMinime, limiteOre) => {
    const currHours = curDate.getHours();
    const currHoursWithOffset = currHours + oreMinime;
    if (limiteOre === '00:00:00') {
        curDate.setHours(currHoursWithOffset);
        return;
    }
    const [ore, minuti] = limiteOre.split(':').map(Number);
    if (currHours > ore || currHours === ore && curDate.getMinutes() > minuti) {
        curDate.setHours(
            currHours + oreMinime + 24
        );
    }
};
const curDate = new Date();
const { tipo } = this.carrello;
if (tipo === 'asporto' || tipo === 'takeaway') {
    const casedProp = tipo.slice(1).toUpperCase() + tipo.slice(1);
    changeDate(currDate, this.negozioSetup['oreMinime' + casedProp]);
}

You could put the values into variables first if you think the combined statement is difficult to read (I'll leave that to you, I don't understand the language) - eg

const cond1 = currHours > ore;
const cond2 = currHours === ore && curDate.getMinutes() > minuti;
if (cond1 || cond2) {
  // ...
}

Alternative to multiple separate properties On a broader scale, having to hard-code the correlation between this.carrello.tipo and the this.negozioSetup.oreMinime- property is a bit odd. If possible, consider rearranging your data so that the two oreMinime and limiteOre values are in an object instead of being two separate properties. Then you just need to use bracket notation to look up the value on the object:

ore: {
  asporto: { minime: <value>, takeaway: <value> },
  takeaway: { minime: <value>, takeaway: <value> },
}

Then you can:

changeDate(currDate, this.negozioSetup.ore[tipo]);

and change changeDate's parameters as needed.

\$\endgroup\$
2
  • \$\begingroup\$ There was a mistake in my code, the second ELSE uses limiteOreTakeaway instead of setting the same limiteOreAsporto \$\endgroup\$ Nov 16, 2020 at 16:59
  • 1
    \$\begingroup\$ Ok, see edit - you can do the same sort of thing as before, pass a parameter to DRY \$\endgroup\$ Nov 16, 2020 at 17:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.