0

I've written a function that heavily relies on the If / Else statement but I feel it's a very beginner way to achieve functionality. Are there any better ways to achieve the same functionality?

Code:

void jumpThree() // Update from left then right
{
  if (effectCounter[0] < tiles) // Right, first tile
  {
    Serial << flag[6] << "\t";
    Serial << newRed[0] << " " << newGreen[0] << " " << newBlue[0] << "\t";

    if (flag[6] == 1)
    {
      oldRed[0]   = (tileBuffer[effectCounter[0]] & 0xff0000) >> 16;
      oldGreen[0] = (tileBuffer[effectCounter[0]] & 0x00ff00) >> 8;
      oldBlue[0]  = (tileBuffer[effectCounter[0]] & 0x0000ff);

      Serial << effectCounter[0] << "\t";
    }

    else if (flag[6] == 0)
    {
      oldRed[0]   = (tileBuffer[6 - effectCounter[0]] & 0xff0000) >> 16;
      oldGreen[0] = (tileBuffer[6 - effectCounter[0]] & 0x00ff00) >> 8;
      oldBlue[0]  = (tileBuffer[6 - effectCounter[0]] & 0x0000ff);

      Serial << (6 - effectCounter[0]) << "\t";
    }

    Serial << oldRed[0] << " " << oldGreen[0] << " " << oldBlue[0] << endl;


    if ((newRed[0] - oldRed[0] != 0) || (newGreen[0] - oldGreen[0] != 0) || (newBlue[0] - oldBlue[0] != 0))
    {
      // Red difference
      if (newRed[0] - oldRed[0] > 0) // Positive red change
      {
        oldRed[0] ++;
      }
      else if (newRed[0] - oldRed[0] < 0) // Negative red change
      {
        oldRed[0] --;
      }

      // Green difference
      if (newGreen[0] - oldGreen[0] > 0) // Positive green change
      {
        oldGreen[0] ++;
      }
      else if (newGreen[0] - oldGreen[0] < 0) // Negative green change
      {
        oldGreen[0] --;
      }

      // Blue difference
      if (newBlue[0] - oldBlue[0] > 0) // Positive blue change
      {
        oldBlue[0] ++;
      }
      else if (newBlue[0] - oldBlue[0] < 0) // Negative blue change
      {
        oldBlue[0] --;
      }

      if (flag[6] == 1)
      {
        tileBuffer[effectCounter[0]] = tile.Colour(oldRed[0], oldGreen[0], oldBlue[0]);
      }

      else if (flag[6] == 0)
      {
        tileBuffer[6 - effectCounter[0]] = tile.Colour(oldRed[0], oldGreen[0], oldBlue[0]);
      }

      // tileBuffer[effectCounter[0]] = tile.Colour(oldRed[0], oldGreen[0], oldBlue[0]);
    }

    else if (((newRed[0] - oldRed[0]) == 0) && ((newRed[0] - oldRed[0]) == 0) && ((newRed[0] - oldRed[0]) == 0))
    {
      effectCounter[0] ++;  // Go to next tile
      //flag[1] = false;
    }
  }

  else
  {
    flag[1] = false;
    effectCounter[0] = 0;

    if (flag[6] == 1) // Flip flop between left and right updates
    {
      flag[6] = 0;
    }

    else if (flag[6] == 0)
    {
      flag[6] = 1;
    }
  }
  delay(wait);
}

closed as off-topic by Enric Blanco, gre_gor, Code Gorilla, sa_leinad, KIIV May 19 '17 at 19:45

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "This question does not appear to be about Arduino, within the scope defined in the help center." – Enric Blanco, gre_gor, Code Gorilla, sa_leinad, KIIV
If this question can be reworded to fit the rules in the help center, please edit the question.

  • Hopefully oldX and newX are signed, otherwise you will never get into if(newRed[0] - oldRed[0] < 0) as it is always false if one of both is unsigned. – Kwasmich May 11 '17 at 13:52
  • They're not signed but it does work correctly, any idea why? – Matt May 11 '17 at 13:55
  • 2
    This question might belong over at Code Review. – Johnny Mopp May 11 '17 at 14:55
  • If-else is just fine. However, the function might become more readable if you split the code into multiple separate functions. – Gerben May 11 '17 at 16:18
1

For part of this you can use the switch case style
Which looks like

switch(flag[6]) {  
    case 0  //your code here 
             break;       // and exits the switch
    case 1 : cout << '2';  
             break;  
    default: //all other cases
}

There is also the ? syntax that can help

condition ? result_if_true : result_if_false

If I "refactor" this type of code (really we all get there from time to time) I try to go back to "what I want". From What I understand your code would look like (no code validation this is a educational example)

int bufferoffset=0;
int bufferMultiplyer=1;

if(flag[6] == 0)
{
   bufferoffset=6;
   bufferMultiplyer=-1;
}        
oldRed[0]   = (tileBuffer[bufferoffset+bufferMultiplyer*effectCounter[0]] & 0xff0000) >> 16;
oldGreen[0] = (tileBuffer[bufferoffset+bufferMultiplyer*effectCounter[0]] & 0x00ff00) >> 8;
oldBlue[0]  = (tileBuffer[bufferoffset+bufferMultiplyer*effectCounter[0]] & 0x0000ff);  

Serial << (bufferoffset+bufferMultiplyer*effectCounter[0]) << "\t";

Serial << oldRed[0] << " " << oldGreen[0] << " " << oldBlue[0] << endl;

int redDiff=newRed[0] - oldRed[0];
int greenDiff=newGreen[0] - oldGreen[0] ;
int blueDiff=newBlue[0] - oldBlue[0] ;
if(redDiff!= 0) 
{
  redDiff > 0?         oldRed[0] ++:   oldRed[0] --;
 }
if(greenDiff!= 0) 
{
  greenDiff> 0?         oldGreen[0] ++:   oldGreen[0] --;
 }    
if(blueDiff!= 0) 
{
  blueDiff> 0?         oldBlue[0] ++:   oldBlue[0] --;
 }        

    tileBuffer[bufferoffset+bufferMultiplyer*effectCounter[0]] = tile.Colour(oldRed[0], oldGreen[0], oldBlue[0]);
}

for more details on switch case see http://en.cppreference.com/w/cpp/language/switch for more detail on ? syntax https://stackoverflow.com/questions/795286/what-does-do-in-c

  • if(greenDiff!= 0) == if(greenDiff) – dandavis May 11 '17 at 22:15
  • I personally don't like if(greendiff) because greendiff is not a boolean. I consider that style. – jantje May 11 '17 at 22:30
0

you can shorten a lot of the vertical space with more compact conditionals, including ternaries to remove double conditionals where one result must follow from another.

For example, you can turn this:

  // Green difference
  if(newGreen[0] - oldGreen[0] > 0)  // Positive green change
  {
    oldGreen[0] ++;
  }
  else if(newGreen[0] - oldGreen[0] < 0) // Negative green change
  {
    oldGreen[0] --;
  }

into just this:

// Green difference
int gDiff = newGreen[0] - oldGreen[0];
if(gDiff) oldGreen[0]+= (gDiff > 0) ?  1 : -1;
0

No, if-else is really the simplest way of dealing with that.

If you wanted to abstract it a little since you have repeated code just with separate variables, you could make a little function

// Note: set the right parameter variable types here
// to match your unspecified variables.
static inline int testUpDown(int a, int b) {
    if (a > b) return 1;
    if (a < b) return -1;
    return 0;
}

// Inside your flag tests:
oldRed[0] = testUpDown(newRed[0], oldRed[0]);
oldGreen[0] = testUpDown(newGreen[0], oldGreen[0]);
oldBlue[0] = testUpDown(newBlue[0], oldBlue[0]);

If you wanted an inline conditional variant you could do it with this:

oldRed[0] = ((newRed[0]-oldRed[0]) ? ((newRed[0]-oldRed[0])/abs(newRed[0]-oldRed[0])) : 0);

But who can understand that? You could always macro it:

#define UPDOWN(A,B) (((A)-(B)) ? (((A)-(B)) / abs((A)-(B))) : 0

oldRed[0] = UPDOWN(newRed[0], oldRed[0]);
0

//when there are multiple if/if-else/nested if/nested if condition then obviously it creates complexity in the code. I think switch is best option to use in such conditions. plz check switch condition if it is feasible in your program. Moreover i will check it in detail if it still not works. Try.........

  • 1
    @Matt please elaborate the objective of your code? – Hafeez Ahmad Fii Zue May 11 '17 at 20:12

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