0

N00b question,

Is there a better way for readability / elegance / DRY etc to check various sensor values for high/low record value? This code works, but looks like the long way round.

  void checkRecord() {

if ( temp.current > temp.high) {
  temp.high = temp.current;
}

if ( temp.current < temp.low) {
  temp.low = temp.current;
}

if ( pressure.current > pressure.high) {
  pressure.high = pressure.current;
}

if ( pressure.current < pressure.low) {
  pressure.low = pressure.current;
}
if ( humidity.current > humidity.high) {
  humidity.high = humidity.current;
}

if ( humidity.current < humidity.low) {
  humidity.low = humidity.current;
}

if ( rainfall.current > rainfall.high) {
  rainfall.high = rainfall.current;
}

if ( rainfall.current < rainfall.low) {
  rainfall.low = rainfall.current;
}
if ( altitude.current > altitude.high) {
  altitude.high = altitude.current;
}

if ( altitude.current < altitude.low) {
  altitude.low = altitude.current;
}
}

Thanks!

  • If it works and is clear, what else do you need? if statements execute quickly and I doubt you are calling the function quickly enough for execution time to matter. Alternatively you could consider storing the data in an array rather than each variable separately - could make export easier. – MichaelT Jan 23 at 21:22
  • nothing wrong with the code ..... formatting is incorrect .... the whole checkRecord() block should be indented ..... it is only a readability issue ..... you could single-line all the if statements .... if ( temp.current > temp.high) temp.high = temp.current; ..... then you could neatly line up everything in a column .... put all the equal signs under each other – jsotola Jan 24 at 3:25
6

This is a variation on the struct idea with an object-oriented flavor.

If you put the current, low and high values of any sensor inside a struct, you could also morph this struct into a class and give it an update() method that, in addition to updating the current value, keeps the low and high updated:

class LowHigh
{
public:
    LowHigh() : current(0), low(INFINITY), high(-INFINITY) {}
    void update(float value) {
        current = value;
        if (current < low)
            low = current;
        if (current > high)
            high = current;
    }
    float get_low() { return low; }
    float get_high() { return high; }
private:
    float current, low, high;
};

I am assuming float data here, you may have to adapt to whatever type is appropriate for your sensors.

You would use the class as this:

LowHigh temp, pressure, humidity, rainfall, altitude;

void loop()
{
    temp.update(temp_sensor.get_reading());
    pressure.update(pressure_sensor.get_reading());
    humidity.update(humidity_sensor.get_reading());
    // ...
}
  • 1
    I like this, it may require a bit of rework to my code, but a Sensor class is exactly how I should go about it to learn OOP and not think so procedural about things. Thanks for your example! I like the INFINITY for setting initial high/low, I had used large and small values out of range. – opt10n Jan 23 at 21:08
  • 1
    I edited your answer to make the argument of the update method a float instead of int. Good explanation. – Michel Keijzers Jan 23 at 22:19
  • 1
    Thanks @MichelKeijzers, that was an oversight of mine. – Edgar Bonet Jan 24 at 8:42
2

You could write something like:

temp.high = (temp.current > temp.high) ? temp.current : temp.high;
temp.low  = (temp.current < temp.low) ? temp.current : temp.low;
...

Most of the time there are min/max macros defined already like:

#define MAX(a, b)   ((a > b) ? a : b)
#define MIN(a, b)   ((a < b) ? a : b)

So you can then simply write:

temp.high = MAX(temp.current, temp.high);
temp.low  = MIN(temp.current, temp.low);
...

Which expands to the exact code as given above.

  • I achieves the goal of looking a bit more tidy... Thank you! – opt10n Jan 23 at 21:02
  • Jeff Wahaus, you can go one step further. Can you make a macro for that with float numbers? – Jot Jan 23 at 21:24
  • Unfortunately this way is slower, because every loop it updates the values of temp.high and temp.low. The original code updated that value only when conditions where true. – Filip Franik Jan 24 at 7:43
  • 1
    @FilipFranik actually, it is exactly as fast as the if-statements. The compiler seems to be able to optimize it. – Jot Jan 24 at 9:53
  • Yes, the optimizer sees 'a=a' and optimizes it out unless you declare 'a' volatile. – Jeff Wahaus Jan 24 at 13:06
0

If the temperature, pressure, humidity, rainfall, and altitude structs were elements of an array of structs, you could iterate over those elements setting each one's high and low values with only two handwritten 'if-then' statements. If you also used an enum to name the indices of the structs, then in other parts of your code you could address each of them by a descriptive name:

// Declare a data struct and define an array of them.
struct {
   int16_t current;
   int16_t high;
   int16_t low;
} data[5] = {0, 0, 0, 0, 0};

// Name the structs by index
enum kind {
   temperature = 0,
   pressure,
   humidity,
   rainfall,
   altitude
};


// Assign one kind of data:
data[temperature].current = getTermperature();


// Set maxes & mins for all of the kinds of data:
for( i = 0; i < sizeof(data)/sizeof(data[0])); ++i ){
   if ( data[i].current > data.high) {
      data[i].high = data[i].current;
   }

   if ( data[i].current < data[i].low) {
      data[i].low = data[i].current;
   }
}

Update:

I'm strugling with the data[5] {0,0...} part in struct.

I can't see your whole sketch but you probably have allocated 5 separate structs named "temp", "pressure", ... , etc. I'm suggesting you allocate an array of 5 structs (which I collectively called "data"), one for each of your individual ones. I included an initializer ( '= {0,0,0,0,0}') to set everything to zero before the start of the run.

all of enum kind{} and how it becomes data[pressure] etc.,

The enum is a quick way to make 5 symbols that stand for consecutive integers, in this case, the index into the array of each struct, using a descriptive name for that struct's data. So 'data[pressure]' is the same as data[1] (in this case) but is more understandable.

the sizeof(data)/sizeof(data[0]) in the loop

That expression gives the number of structs in the array (the total byte-size of the array divided by the byte-size of one struct). We could write '5' instead, but when you someday add another member or two (wind_direction, wind_speed, perhaps?), you'd have to remember to also fix your for() loop. By using this calculation, the compiler will do it for you, leaving you one less opportunity to make a mistake you'd then need to debug.

and finally I think altitude.high in the if statement is a paste typo? should be data[i].high?

Absolutely right! Fixed now.

Update:

For defining initial values can you just assign an initial value in the struct such as int16_t low = INFINITY; and so on instead of {0,0,0,0,0}?

Yes - Without knowing your data and application, I chose the simplest initializer. The full syntax to do what you want would look like this:

// Declare a data struct and define an array of them.
struct {
   int16_t current;
   int16_t high;
   int16_t low;
} data[5] = {
  {0, 0, INFINITY},
  {0, 0, INFINITY},
  {0, 0, INFINITY},
  {0, 0, INFINITY},
  {0, 0, INFINITY}
};

You don't need (and may not) specify each data type and name; the struct declaration already provides that information. The initializers are positional and each one must be compatible with the data-type it matches up to.

  • Thanks, I don't understand a few things because n00b. I'm strugling with the data[5] {0,0...} part in struct. all of enum kind{} and how it becomes data[pressure] etc., the sizeof(data)/sizeof(data[0]) in the loop and finally I think altitude.high in the if statement is a paste typo? should be data[i].high? – opt10n Jan 23 at 21:01
  • Thank you so much, the explanation of enum kind{}, and the expression in the loop was very helpful in understanding how it all fit together. For defining initial values can you just assign an initial value in the struct such as int16_t low = INFINITY; and so on instead of {0,0,0,0,0}? Since a default value of 0 for low will not work properly if the temp is above freezing. – opt10n Jan 25 at 15:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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