looking for advice on streamlining a function SOLVED.

Hello. I have a function that averages 3 values and knows how to do it depending on which values are greater than zero:

int8_t average(int8_t near, int8_t far, int8_t out) { //returns avg temp and knows how to avg temps depending on which sensors are plugged in.
  if (near < 0 && far < 0 && out < 0) { //if none are plugged in, return zero.
    return 0;
  } //end if.
  else if (near < 0 && far < 0) { //if two of the 3 are unplugged, return the one that isnt.
    return out;
  } //end else.
  else if (far < 0 && out < 0) {
    return near;
  } //end else.
  else if (near < 0 && out < 0) {
    return far;
  } //end else.
  else if (near < 0) { //if one is unplugged, return avg of other two.
    return (far + out) / 2;
  } //end else.
  else if (far < 0) {
    return (near + out) / 2;
  } //end else.
  else if (out < 0) {
    return (near + far) / 2;
  } //end else.
  else { //return avg of all 3.
    return (near + far + out) / 3;
  } //end else.
} //end average().

It seems like its doing the same tests over and over and probably redundantly. Any ideas on how to streamline it?

Thanks!

Try counting the number of 0 values. then subtract that from 3. your "average" is then just sub of values/(3-num of 0). BUT remember to avoid division by 0!

Mark

Thanks. Ill think about it. Only Chuck Norris can divide by zero.

The way you programmed it, is good.
I would to that in a different way, with a second function ‘average’.

  if (near >= 0 && far >= 0 && out >= 0)
  {
    // everything is valid, calculate the average.
    avg = average( near, far, out);
  }
  else
  {
    // something is wrong.
    ....           <-- all the if .. else go here.
  }

I didn’t try to think through what holmes4 wrote, but precalculations with afterwards correction should work.
Perhaps there a few variations for that.

If the temperature is 50 degrees, the function is wrong. (50 + 50 + 50) / 3 will not fit in an 8 bit signed.

how about something like this -

total=0;
count=3;

if (near<0) count--; else total+=near;
if (far<0) count--; else total+=far;
if (out<0) count--; else total+=out;

if (count) return total/count; else return 0;

If the temperature is 50 degrees, the function is wrong. (50 + 50 + 50) / 3 will not fit in an 8 bit signed.

Its worked fine so far and my average temp is 62. I think the compiler is smart enough to not assign the value until after its been divided.

Thanks for the advise, I'll think about it!

if (out<0) count--; else total+=out;

No ;before the else dude!

Mark

It would be interesting to know what assembly code the compiler will make of that. An 8-bit microcontroller that does a calculation in 16 bits with variables that are 8 bit ? I would not expect that. As far as I know that calculation is wrong with 62 degrees.

apologies vulture please disregard my suggestion as it contained an extra semicolon. best stick to your original code. =(

I’d call this somewhat streamlined. Plug some values into the three variables and give 'em a try.

#define near 0
#define far 1
#define out 2
int temps[3];

void setup() {
  Serial.begin(115200);
  temps[near] = 50;
  temps[far] = 40;
  temps[out] = 0;
  Serial.println(average());
}

int average() {
  int avg = 0;
  int count = 0;
  for (int i = 0; i<3; i++) {
    if (temps[i] > 0 ) {
      count++;
      avg = avg + temps[i];
    }
  }
  if (avg > 0) {
    avg = avg / count;
  }
  return avg;
}

void loop() {
}

Nice, ill try that!

But do you mean avg = avg / count;?

vulture2600: But do you mean avg = avg / count;?

Well,it does work better that way. :cold_sweat:

this seems to be working :

int8_t average(int8_t near, int8_t far, int8_t out) {
  uint8_t count = 0;
  uint16_t avg = 0;
  if (near > 0) {
   count ++;
   avg += near;
  }
  if (far > 0) {
    count ++;
    avg += far;
  }
  if (out > 0) {
    count ++;
    avg += out;
  }
  if (count != 0) {
    return avg / count;
  }
  else return 0;
} //end average().

thanks for the help, everyone!

Very nice code lar3ry, you should be a teacher. I like that code, since it can be extended for more values easily.

I would change this : if (avg > 0) { To this: if (count > 0) { Because that is closer to the actual funtionality.

And I would use "avg +=" and "avg /="

vulture2600, your resulting code is so easy now. I didn't know that it was possible. Great result. The compiler has to convert 8-bit variables to 16-bit integers and vice versa, that requires more code and is slower than using 16-bit integers. I learned that by trying to optimize my sketch by using 8-bit variables, but the mix of 16-bit and 8-bit made it slower.

holmes4:

if (out<0) count--; else total+=out;

No ;before the else dude!

Mark

If that code were written properly:

if(out < 0)
   count--;
else
   total += out;

Which ; do you think doesn’t belong?

if (i want to put an if...then..else statement on one line)
{
    i can;
    it does not mean the code is not written "properly";
    it is personal preference && pragmatic choice acquired through many years programming experience;
    i need to include a ";" before the else keyword, as i indeed did;
}
else
{
    i can make a trivial statement 4 lines long;
}

Note how I could have said that in one sentence but thought it would be more readable to format it properly :)

it is personal preference && pragmatic choice acquired through many years programming experience;

As long as you are the only one maintaining the code, do what you want. If you want other people to help, do it right.

if (out<0) count--; else total+=out;

As there are two semi colons, you have two statements. Might as well do it properly and keep it to a single statement per line, however that is also my preference.

total += (out < 0) ? ++count, 0 : out;

Paul are you seriously saying the code below would be hard to maintain???

if (out<0) count--; else total+=out;

I think you’re taking the concept of general programming good practice (which I would not disagree with) and then using it to argue the most trivial of real-life cases. Really quite unnecessary within the context of this thread, which if you remember was about simplifying an algorithm.

My comments were initially directed at holme4, who said that a semicolon was unnecessary. I disagreed.

I do not like having more that one statement on a line. You have 4 on one line. Yes, that makes it harder to maintain code. What if you have some issue with that block of code, and need to print a variable’s contents? It is, IMHO, a lot harder to add a print statement to:

if (out<0) count--; else total+=out;

then it is to add one to:

if(out < 0)
{
   count--;
}
else
{
   total += out;
}

While the latter does take more vertical space to display, the intent and structure is far easier to see, in my opinion.