[SOLVED] Nested IFs - a quick check would be appreciated

I have found a bit too many posts on the subject of nested IFs, so now I’m unreasonably worried, and I would appreciate if somebody could kindly check the following excerpt. As a newbie, I’ll have some challenges to face, and I’d like to avoid adding unnecessary trouble by misusing the IF syntax.

I’d just like to know whether the “if” nesting is correct. I don’t expect you to spend time on the program logic, explanations are provided just in case you’d like to understand what I am aiming at.

Many thanks, here is the piece of code

/*
LIGHT-TRACKING TURRET, HORIZONTAL AXIS CONTROL
(no hardware limit switches)

Before program start, the turret is manually initialized to an horizontal neutral
position and the current_heading variable is initialized to a conventional 
value named neutral_heading. Current_heading is increased by rotation to the right
and decreased by rotation to the left. Soft rotation limits (the lowest, 
left_limit and the highest, right_limit) are implemented.
The main loop constantly checks two photoresistors to rotate towards
the highest luminance (= the lowest resistance reading) until both
photoresistor readings are subequal (=the turret is headed towards the light source).
If the allowed delta between the two photoresistors is exceeded,
a stepper rotation in the direction of the lower resistance is needed. 
If the "soft limit switch" in the required direction is not reached, then
the stepper is rotated in the appropriate direction, else a tone is played.
*/

{

if(abs(res_left-res_right) > allowed_delta) // horizontal move needed
{
    if(res_left<res_right) 
    // left rotation required?
    {
        if(current_heading>left_limit) // check if left rotation limit is reached
        {
            //<... here the instructions to rotate stepper to the left ...>;
            current_heading--;
        }
        else
            play_note(1); // cannot move further to the left - buzz!
     }
     else 
     // right rotation required! 
     {
        if(current_heading<right_limit) // check if right rotation limit is reached
        {
            //<... here the instructions to rotate stepper to the right ...>;
            current_heading++;
        }
        else
            play_note(2); // cannot move further to the right - buzz another way!
     }
}

I'd just like to know whether the "if" nesting is correct.

If the compiler is happy, and the code produces the correct results, it is.

Otherwise, it isn't.

@PaulS's advice is very pragmatic.

I try to avoid nested IFs as much as I can.
One problem with them is that each IF introduces at least 2 options so nested 3 deep you have 8 options. Can you be sure that your code properly accounts for all of them?

One way would be to draw a table with all the possible options on it and check it against your code.

Another problem (which I consider to be very significant) is that nested IFs make it very difficult to figure out what the code is supposed to be doing when you try to understand it after a break of a few months.

I like to write code that reads like a story so you don't need to study the code to see WHAT it is doing - only to see HOW it does it.

...R

I thank both @PaulS and @Robin2 for their useful comments.
Yes, I'm aware that compilation includes a syntax/structure check.
And, in my case, I can't imagine anything else than nested ifs to manage my operational scenario.
I'm still very far from a compilable version of the code - by the time I'll compile the program, it will be much bigger, and Arduino isn't very vocal about errors... so a deep structural defect, such as an improper use of nested ifs, may not be easy to trace and solve.
I was just thinking that an ounce of prevention is worth a pound of cure, and I wished to do some preventive check.

Many thanks, and excuse me of my naive approach.

In my opinion it is much better to use a case statement than a lot of if elseif statements.

and Arduino isn’t very vocal about errors.

Have you switched to verbose mode.

To do this, hold down the shift key while clicking on the upload button in the toolbar.

You might consider putting this and its twin in functions of their own:

        if(current_heading>left_limit) // check if left rotation limit is reached
        {
            //<... here the instructions to rotate stepper to the left ...>;
            current_heading--;
        }
        else
            play_note(1); // cannot move further to the left - buzz!

Then you still have the same functionality, but the nested ifs have gone away (or hidden at least).

You may well find too, that that pair of functions can be refactored so that they simply call a function that does both left and right, depending on what parameters it is passed.

CesareBrizio:
And, in my case, I can’t imagine anything else than nested ifs to manage my operational scenario.

In my experience some IFs can be used to exclude stuff completely so that nesting isn’t needed. This is, perhaps, a trivial example

void getData() {
   if (Serial.available >= 5) {
      byte inbyte = Serial.read();
      if (inbyte == ........
   }
}

OR - less nesting with an inverted test

void getData() }
   if (Serial.available < 5) {
       return;
   }
   byte inbyte = Serial.read();
   if (inbyte == ........
}

Putting code into short functions can also make the logic clearer even if, at the end of the day, the nesting is the same

…R

Thank you everybody for your insightful suggestions!
@Grumpy_Mike - I wasn't aware of the Verbose Mode, thanks for the explanation!
@wildbill - I agree, calling small functions can simplify and clarify my code
@Robin2 - I'll think about the examples that you provided, again thank you

I tried to follow all the suggestions I received, and now I got rid of nested IFs completely.
I’m still waiting for the stepper motors, and some time will pass before I can compile and test the logic of my code, but thanks to your help it will be much easier to understand and maintain.
Again thank you to everybody!

/*
LIGHT-TRACKING TURRET, HORIZONTAL AXIS 
(AZIMUTH) CONTROL (no hardware limit switches)
----------------------------------------------
Before program start, the turret is manually initialized to an horizontal neutral
position and the current_heading variable is initialized to a conventional 
value named neutral_heading. Current_heading is increased by rotation to the right
and decreased by rotation to the left. Soft rotation limits (the lowest, 
left_limit and the highest, right_limit) are implemented.
The main loop constantly checks two photoresistors to rotate towards
the highest luminance (= the lowest resistance reading) until both
photoresistor readings are subequal (=the turret is headed towards the light source).
If the allowed delta between the two photoresistors is exceeded,
a stepper rotation in the direction of the lower resistance is needed. 
If the "soft limit switch" in the required direction is not reached, then
the stepper is rotated in the appropriate direction, else a tone is played.
*/

/*
Global variables for azimuth control
------------------------------------
Turret will allow a 180° rotation from 0° (leftmost)
to 180° (rightmost) position. It will be centered 
manually before program start. Tentatively, I assume 
that the stepper will be rotated in 4,5° steps, so that 
180° will require 40 steps (the real number of steps 
will be empirically determined depending from stepper 
features). Heading is expressed by step number, from step 0 
(leftmost position, 0°) to step 40 (rightmost position, 
180°). Thus, the initial neutral heading will correspond 
to step 20.
*/

int resLeft; // reading from left photoresistor
int resRight; // reading from right photoresistor
int allowedDelta = 10; // difference threshold to trigger movement
int currentHeading; // will be constantly updated

int leftLimit = 0; // leftmost step no.
int neutralHeading = 20; // mid range step no., tentatively set to 20
int rightLimit = 40; // leftmost step no., tentatively set to 40


void checkAzimuth() {

    if(abs(resLeft-resRight) <= allowedDelta) // horizontal rotation NOT needed
    {
        Serial.println("Azimuth correct - no rotation required");
        return;
    }

    if(resLeft<resRight) // left rotation required?
    {
        Serial.println("Attempting left rotation");
        rotateAzimuth("L");
    }
    else  // right rotation required! 
    {
        Serial.println("Attempting right rotation");
        rotateAzimuth("R");
    }
}

void rotateAzimuth(string direction) {
    switch (direction) {
        case "L":
            //Attempt rotation to the left
            if(currentHeading>leftLimit) // check if left rotation limit is reached
            {
                Serial.println("Performing left rotation");
                //<... here the instructions to rotate stepper to the left ...>;
                currentHeading--;
            }
            else
            {
                Serial.println("Left rotation limit reached!");
                play_note(1); // cannot move further to the left - buzz!
            }            
            break;
        case "R":
            //Attempt rotation to the right
            if(currentHeading<rightLimit) // check if left rotation limit is reached
            {
                Serial.println("Performing right rotation");
                //<... here the instructions to rotate stepper to the right ...>;
                currentHeading++;
            }
            else
            {
                Serial.println("Right rotation limit reached!");
                play_note(2); // cannot move further to the right - buzz!
            }
            break;
    }
}


void setup(){
    // among other things, setup() will initialize currentHeading
    currentHeading==neutralHeading;
}

void loop(){
    // among other things, loop() will constantly check the azimuth
    checkAzimuth();
}

You must replace "L" by 'L' and "R" by 'R' - string constants are pointers, the
result of "L" == "L" is undefined in general (its compiler specific).

'L' == 'L' is very well defined since character constants are just integers in disguise.

You are completely right, @MarkT!!!
I'm definitely not at ease with variable / constant types, it's about 25 years since I last made some C programming and I forgot the fundamentals. Very helpful indeed, thank you again!

Cesare

I'm still very far from a compilable version of the code - by the time I'll compile the program, it will be much bigger,

My advice would be to stop right now and create a version that compiles even if it calls functions that do nothing or perhaps return fixed values that will vary in the final version.

Do not continue without testing sections of the program until you have written it all or you will regret it.

Thank you for the very welcome advice, @UKHeliBob.
Yes, before trying to implement stepper control proper, I'll spend some time just to test the logic by "empty" functions, just using the Serial. println and some beep.