Optimize and improve If Else Condition-CodePudding

In one old project, I found this condition ( l little bit rewrite it to work it here but logic was saved )

I looking for and I don't see the solution how it possible to do better. for use 'switch' this case also doesn't look good please correct me if I am wrong

So any ideas on how it can be written better? Thanks a lot

    const conditionOne = true
    const conditionTwo = true
    const conditionThree = false
    
    
if (conditionOne) {
    if (conditionTwo) {
        if (!conditionThree) {
            // dispatch(new Actions.SetFirstValue('someValue1');
            console.log('SetFirstValue  set somevalue 1')
        } else {
            console.log('SetSecondValue  set somevalue 2')
            // dispatch(new Actions.SetSecondValue('someValue2');
        }
    } else if (!conditionThree) {
        // dispatch(new Actions.SetThirdValue('someValue3');
        console.log('SetThirdValue  set somevalue 3')
    } else {
        // dispatch(new Actions.SetFourthValue('someValue4');
        console.log('SetFourthValue  set somevalue 4')
    }
}

if (!conditionOne) {
    if (conditionTwo) {
        if (!conditionThree) {
            console.log('SetFirstValue  set 'null' ')
            // dispatch(new Actions.SetFirstValue(null);
        } else {
            // dispatch(new Actions.SetSixValue('someValue6');
            console.log('SetSixValue  set somevalue 6')
        }
    } else if (!conditionThree) {
        console.log('SetThirdValue  set 'null' ')
        // dispatch(new Actions.SetThirdValue(null);
    } else {
        // dispatch(new Actions.SetEightValue('someValue8');
        console.log('SetEightValue  set somevalue 8')
    }
}

CodePudding user response：

It would be good to change the Actions class so that rather than spelling out the different numeric value setters in words, it has a method that accepts an argument instead. For example, change it so that this

new Actions.SetFirstValue('someValue1')
new Actions.SetSecondValue('someValue2')

can be

new Actions().setValue(0, 'someValue1')
new Actions().setValue(1, 'someValue2')

using the 0-indexed collection convention. That'll simplify things a lot.

After that - notice that all of your conditions are mutually exclusive, you can create an array of conditions with their associated values to set. Find the first truthy condition, and then you can call the method with the needed parameters.

const valuesToSetByCondition = [
  [c1 && c2 && !c3, 0, 'someValue1'], // this is the data for the branch: dispatch(new Actions.SetFirstValue('someValue1');
  [c1 && c2, 1, 'someValue2'] // this is the data for the branch: dispatch(new Actions.SetSecondValue('someValue2');
  [c1 && !c3, 2, 'someValue3'],
  [c1, 3, 'someValue4'],

  [c2 && !c3, 0, null],
  [c2 && c3, 5, 'someValue6'],
  [!c3, 2, null],
  [true, 7, 'someValue8']
];
const actionToUse = actionsByCondition.find(condition => condition[0]);
dispatch(new Actions().setValue(actionToUse[1], actionToUse[2]));

You can either omit parts of the conditions by taking into consideration above conditions - like what I did above (eg, c1 && !c3 means that the next check can be c1, rather than c1 && c3) - or you can be more explicit and include every branch in every array element. It'll be a bit more WET but might be easier to understand, and would allow the array to be in any order.

[
  [c1 && c2 && !c3, 0, 'someValue1'],
  [c1 && c2 && c3, 1, 'someValue2']
  [c1 && !c2 && !c3, 2, 'someValue3'],
  [c1 && !c2 && c3, 3, 'someValue4'],
  // etc

CodePudding user response：

This could in fact work fine as a switch statement:

const conditionOne = true
const conditionTwo = true
const conditionThree = false

dispatch(() => {
    switch (`${conditionOne} ${conditionTwo} ${conditionThree}`) {
        case 'true true false':   return new Actions.SetFirstValue('someValue1');
        case 'true true true':    return new Actions.SetSecondValue('someValue2');
        case 'true false false':  return new Actions.SetThirdValue('someValue3');
        case 'true false true':   return new Actions.SetFourthValue('someValue4');
        case 'false true false':  return new Actions.SetFirstValue(null);
        case 'false true true':   return new Actions.SetSixValue('someValue6');
        case 'false false false': return new Actions.SetThirdValue(null);
        case 'false false true':  return new Actions.SetEightValue('someValue8');
    }
})());

I've kept the order for now, but would recommend to sort so that you have a proper truth table:

        case 'true true true':    return new Actions.SetSecondValue('someValue2');
        case 'true true false':   return new Actions.SetFirstValue('someValue1');
        case 'true false true':   return new Actions.SetFourthValue('someValue4');
        case 'true false false':  return new Actions.SetThirdValue('someValue3');
        case 'false true true':   return new Actions.SetSixValue('someValue6');
        case 'false true false':  return new Actions.SetFirstValue(null);
        case 'false false true':  return new Actions.SetEightValue('someValue8');
        case 'false false false': return new Actions.SetThirdValue(null);

For better readability of the cases, you can also use (conditionOne ? ' one' : '!one') etc in the identifier, with whatever the real condition denotes.

If the actions are cheap to create and have no side effects, you might even consider really putting them into a table, and indexing them with bitwise operations:

const conditionOne = true
const conditionTwo = true
const conditionThree = false

const table = [
    new Actions.SetThirdValue(null),
    new Actions.SetEightValue('someValue8'),
    new Actions.SetFirstValue(null),
    new Actions.SetSixValue('someValue6'),
    new Actions.SetThirdValue('someValue3'),
    new Actions.SetFourthValue('someValue4'),
    new Actions.SetFirstValue('someValue1'),
    new Actions.SetSecondValue('someValue2'),
];
dispatch(table[conditionOne << 2 | conditionTwo << 1 | conditionThree << 0]);

This is shorter, but less readable, and a bit confusing if your binary is not strong or if the matrix doesn't fit the real use case.

If you like none of these approaches, your nested if cascade is also ok, except you should not invert conditionThree - just flip the conditional branches - and you should use consistent nesting with else blocks:

if (conditionOne) {
    if (conditionTwo) {
        if (conditionThree) {
            dispatch(new Actions.SetSecondValue('someValue2'));
        } else {
            dispatch(new Actions.SetFirstValue('someValue1'));        
        }
    } else {
        if (conditionThree) {
            dispatch(new Actions.SetFourthValue('someValue4'));
        } else {
            dispatch(new Actions.SetThirdValue('someValue3'));
        }
    }
} else {
    if (conditionTwo) {
        if (conditionThree) {
            dispatch(new Actions.SetSixValue('someValue6'));
        } else {
            dispatch(new Actions.SetFirstValue(null));
        }
    } else {
        if (conditionThree) {
            dispatch(new Actions.SetEightValue('someValue8'));
        } else {
            dispatch(new Actions.SetThirdValue(null));
        }
    }
}

CodePudding user response：

Try this flow if seems easier to you:

    const conditionOne = true
    const conditionTwo = true
    const conditionThree = false

if (conditionOne && conditionTwo && !conditionThree) {
    console.log('SetFirstValue  set somevalue 1')
}else if(conditionOne && conditionTwo && conditionThree){
    console.log('SetSecondValue  set somevalue 2')
}else if(conditionOne && !conditionTwo && !conditionThree){
    console.log('SetThirdValue  set somevalue 3')
}else if(conditionOne && !conditionTwo && !conditionThree){
    console.log('SetThirdValue  set somevalue 3')
}

CodePudding user response：

Ok, you said maybe a switch statement, so I decided to try to work out a binary system, where each condition is represented by a digit of one or zero. That way it's easy to test the state of the conditions with one linear statement, and no need for nesting.

I think your SetXValue numbering is off, but just in case it's not, I reworked the logic tree to set the same values for the same conditions as your example.

Given:

1s place is firstCondition
10s place is secondCondition
100s place is thirdCondition

// this would evaluate to true, false, false, going from one to three
let conditions = "001"

switch(conditions) {
  case "011":
    console.log('SetFirstValue')
    break;
  case "111":
    console.log('SetSecondValue')
    break;
  case "001":
    console.log('SetThirdValue')
    break;
  case "101":
    console.log('SetFourthValue')
    break;
  case "010":
    console.log('SetFirstValue')
    break;
  case "110":
    console.log('SetSixthValue')
    break;
  case "000":
    console.log('SetThirdValue')
    break;
  case "100":
    console.log('SetEightValue')
    break;
  default:
    break;
}

And updating your conditions could involve some binary math to update the number string. Or you could just slice and dice the string as you see fit.

CodePudding user response：

There are eight combinations of conditions, these can be keys in an object that holds the code. Here, finding the logic to run is better than linear time...

function doIf(conditionOne, conditionTwo, conditionThree) {
  const whatToDo = {
   ttf : () => console.log('one, two, but not three');
   ttt : () => console.log('one, two and three');
   ftt : () => console.log('not one, but two and three');
   //...
  };

  const boolToA = condition => condition ? 't':'f';
  const key = boolToA(conditionOne)   boolToA(conditionTwo)   boolToA(conditionThree);
  return whatToDo[key]();
}

Instead of anonymous blocks, the code can also be function refs, as in...

function whenOneAndTwoButNotThree() {
   console.log('one, two, but not three');
}

function doIf(conditionOne, conditionTwo, conditionThree) {
  const whatToDo = {
    ttf : whenOneAndTwoButNotThree;
    //...

CodePudding user response：

To make it cleaner, extract logic into separate functions. For example,

if (!conditionThree) {
   // dispatch(new Actions.SetFirstValue('someValue1');
   console.log('SetFirstValue  set somevalue 1')
} else {
   console.log('SetSecondValue  set somevalue 2')
   // dispatch(new Actions.SetSecondValue('someValue2');
}

can be extracted into a function, passing conditionThree in as a parameter. The same can be done with the rest of the if-else statements.

CodePudding user response：

By building a truth table!

const t = {}

// populate with arrays
// null is 0
for (let i = 0; i < 9; i  ) {
  if (i != 7 && i != 5) t[i] = []
}

// test all cases
for (let a = 0; a < 2; a  ) {
  for (let b = 0; b < 2; b  ) {
    for (let c = 0; c < 2; c  ) {

      if (a) {
        if (b) {
          if (!c) {
            t[1].push(`${a}${b}${c}`)
          } else {
            t[2].push(`${a}${b}${c}`)
          }
        } else if (!c) {
          t[3].push(`${a}${b}${c}`)
        } else {
          t[4].push(`${a}${b}${c}`)
        }
      }

      if (!a) {
        if (b) {
          if (!c) {
            t[0].push(`${a}${b}${c}`)
          } else {
            t[6].push(`${a}${b}${c}`)
          }
        } else if (!c) {
          t[3].push(`${a}${b}${c}`)
        } else {
          t[8].push(`${a}${b}${c}`)
        }
      }
    }
  }
}

console.log(t)

Outputs

{
  '0': [ '010' ],
  '1': [ '110' ],
  '2': [ '111' ],
  '3': [ '000', '100' ],
  '4': [ '101' ],
  '6': [ '011' ],
  '8': [ '001' ]
}

Looking at '0': [ '010' ] you can see that the someValue is equal to null (since 0 represents null for convenience in the loop) only when conditionOne is false, conditionTwo is true and conditionThree is false.

So you would type the following in the code:

if (!conditionOne && conditionTwo && !conditionThree) {
    console.log('SetFirstValue  set 'null' ')
    // dispatch(new Actions.SetFirstValue(null);
}

And you can repeat for remaining cases. The benefit is that this will mechanically group cases by the values that they output which looks clean. You can also use the generated text to build a binary tree for fast lookup.