Lazarus

Programming => General => Topic started by: MaxCuriosus on July 17, 2020, 12:02:43 pm

Title: Eliminating Goto/Label
Post by: MaxCuriosus on July 17, 2020, 12:02:43 pm
In the code skeleton below I'd like to eliminate the gotos/labels while maintainig a reasonably equivalent clarity in the "flow chart" of the code. How could I do that?

Code: Pascal  [Select][+][-]
  1. Procedure Skeleton;
  2. Label
  3.   Reload, DispLoop, Lexit;
  4.  
  5. Begin
  6.   { Code #1 }
  7.   If Condition1 then
  8.     Begin
  9.       { Code #2 }
  10.       Goto Lexit;
  11.     end;
  12.   { Code #3 }
  13.  
  14. Reload:
  15.   { Code #4 }
  16.  
  17. DispLoop:
  18.   { Code #5 }
  19.  
  20.   If Condition2 then Begin Goto DispLoop; end;
  21.  
  22.   If Condition3 then
  23.  
  24.     Begin
  25.  
  26.       Case VK of
  27.         0:
  28.           Begin Goto Lexit; end;
  29.         1:
  30.           Begin
  31.             If Condition4 then Begin Goto DispLoop; end;
  32.             { Code #6 }
  33.             Goto Reload;
  34.           end;
  35.         2:
  36.           Begin
  37.             { Code #7 }
  38.             Goto Reload;
  39.           end;
  40.         3:
  41.           Begin
  42.             { Code #8 }        
  43.             If Condition5 then
  44.               Begin
  45.                 { Code #9 }
  46.                 Goto DispLoop;
  47.               end;
  48.             { Code #10 }
  49.             Goto Reload;
  50.           end;
  51.         4:
  52.           Begin
  53.             { Code #11 }
  54.             If Condition6 then Begin Goto DispLoop; end;
  55.             If Condition7
  56.               then
  57.                 Begin
  58.                   { Code #12 }
  59.                   Goto Reload;
  60.                 end
  61.               else
  62.                 Begin
  63.                   Goto DispLoop;
  64.                 end;
  65.           end;
  66.         5:
  67.           Begin
  68.             { Code #13 }
  69.             Goto DispLoop;  
  70.           end;
  71.  
  72.       end; {Case}
  73.  
  74.     end; {If Condition3}
  75.  
  76. Lexit:
  77.   { Code #14 }
  78. end;
  79.  
Title: Re: Eliminating Goto/Label
Post by: marcov on July 17, 2020, 12:18:07 pm
One to one substitutions in cases of multiple exit/break are hard. I did give it a try to solve it without additional state variables:

Code: Pascal  [Select][+][-]
  1.     Procedure Skeleton;
  2.     Label
  3.       Reload, DispLoop, Lexit;
  4.      
  5.     Begin
  6.      try
  7.       { Code #1 }
  8.       If Condition1 then
  9.         exit; // if lexit is always exit procedure.
  10.       { Code #3 }
  11.      
  12.     Reload:
  13.       { Code #4 }
  14.       while true do
  15.         begin
  16.      
  17.     DispLoop:
  18.       { Code #5 }
  19.       while true do
  20.         begin
  21.  
  22.       If Condition2 then
  23.           continue;
  24.      
  25.       If Condition3 then
  26.    
  27.         Begin
  28.      
  29.           Case VK of
  30.             0:
  31.               exit;
  32.             1:
  33.               Begin
  34.                 If Condition4 then continue;
  35.                 { Code #6 }
  36.                 break;
  37.               end;
  38.             2:
  39.               Begin
  40.                 { Code #7 }
  41.                 break;
  42.               end;
  43.             3:
  44.               Begin
  45.                 { Code #8 }        
  46.                 If Condition5 then
  47.                   Begin
  48.                     { Code #9 }
  49.                      continue;
  50.                   end;
  51.                 { Code #10 }
  52.                 break;
  53.               end;
  54.             4:
  55.               Begin
  56.                 { Code #11 }
  57.                 If Condition6 then continue;
  58.                 If Condition7
  59.                   then
  60.                     Begin
  61.                       { Code #12 }
  62.                       break;
  63.                     end
  64.                   else
  65.                     Begin
  66.                       continue;
  67.                     end;
  68.               end;
  69.             5:
  70.               Begin
  71.                 { Code #13 }
  72.                 continue;
  73.               end;
  74.      
  75.           end; {Case}
  76.      
  77.         end; {If Condition3}
  78.         break; // if we get here, there is no terminating goto, so assume no loop.
  79.                // it could be that that we never get here (there is always a break or continue), in that case, kill the continues, and this break
  80.        end; {disploop}
  81.        // we get here for "breaks", this, run reloadloop again.
  82.        end {reloadloop}
  83.       finally
  84.       { Code #14 }
  85.        end;
  86.     end;
  87.      
Title: Re: Eliminating Goto/Label
Post by: Warfley on July 17, 2020, 03:05:12 pm
Replacing goto simply for the replacements sake is generally a bad idea, goto is not bad because there is something inherently bad in using this keyword, but because it leads to poorly designed code.

Meaning a 1-1 substitution, while getting rid of goto, will not get rid of the design of the code, and therefore does not improve code quality.

The clearest example for this I can think of:
Code: Pascal  [Select][+][-]
  1. label foo;
  2. begin
  3.   ...
  4.   goto foo;
  5.   ...
  6. foo:
  7.   ...
can be replaced with:
Code: Pascal  [Select][+][-]
  1. type TGotoFooException = class;
  2. begin
  3.   try
  4.     ...
  5.     raise TGotoFooException.Create;
  6.     ...
  7.   except on e: TGotoFooException do;
  8.   end;
  9.   ...
I think it is pretty obvious that the second code, even though it does not contain goto is actually worse in every way.

So I am also not really fond of the way it was done by marcov, because the code does not really get more readable with these while true, continue-break approaches, because this is basically the same jumping around, just less obvious.

IMHO you should change the design of that function a bit to use the full extend of the tools available to you. These are:
1. Try-Finally to allow you to jump to a final code block that should be executed in the end, meaning together with exit() this can be used as "jump to end"
2. If blocks let you jump over blocks depending on a condition, which can basically express a jump down to
3. repeat until lets you jump back to the beginning of a block if a condition is true, which basically expresses a jump up to
4. While basically combines the features of if and repeat-until
5. Functions allow you to call code blocks by name independently of the current position in code.

Your function does basically the following:
1. if a certain condition is fulfilled, simply skip the whole function
2. execute some code (reload, disploop)
3. with the results of step 2 do other code and either decide if you are finished, need to start over at 2 or start over at 2 but skip the reloading.
4. on finish execute some final code (even if step 1 skip is done)

First the obvious the 4. point can be realized with a try-finally and the first point with the exit "function".
Code: Pascal  [Select][+][-]
  1. try
  2.   if Condition1 then
  3.     Exit;
  4.   // LOOPCode
  5. finally
  6.   Code14
  7. end;

So let's look into the loop code.
First and foremost it is always a good idea to put code in separate functions to have concise and readable functions (use nested functions to not fill up your scope with one-time use functions), so for all your Code #N comments I will simply assume that there is a function for it called CodeN

The next thing is, Reload and DispLoop belong together, if Reload is executed disploop will always be also executed. So Reload is optional and disploop is mandatory. There are two ways to implement this, explicitly skip Reload or explicitly execute Reload.
Because the number of Reloads is not so significantly different from the number of disploops without reloads, I think the best option would be to explicitely skip reload so we can simply do this function:
Code: Pascal  [Select][+][-]
  1. procedure DispLoop(reload: boolean);
  2. begin
  3.   if Reload then // (explicit) skip of reload if not requested
  4.     Code4; // reload section
  5.   Code5; // disploop section
  6. end;

Your loop then can be expressed by a repeat-until (as you want it to run at least once and multiple times if requested). Whether you want to execute reload or not needs to be saved in some form of state variable:
Code: Pascal  [Select][+][-]
  1. repeat
  2.   DispLoop(doReload); // function defined above
  3.   // Condition 2
  4.   if Condition 3
  5.     // case statement
  6. until Condition3 Or VK = 0;

The two loop breaking cases Condition 3 and VK = 0 are already covered by the loop condition, so if you read your code you will now see on a first glance what the stop condition is (which is much more complicated if it is a break or continue or goto buried somewhere deep in your code).

For condition2 you have two possibilities, using continue, which will directly jump to the beginning of the loop again, or to combine condition 3 and 2:
Code: Pascal  [Select][+][-]
  1. // first option:
  2. repeat
  3.   DispLoop(Reload); // function defined above
  4.   Reload = false;
  5.   if Condition2 then Continue;
  6.   if Condition 3
  7.     // case statement
  8. unti notl Condition3 Or VK = 0;
  9. // Second option
  10. repeat
  11.   DispLoop(Reload); // function defined above
  12.   if not Condition2 and Condition 3
  13.     // case statement
  14. until (not Condition2 and Condition3) Or VK = 0;
Personally I think the continue call is cleaner if you follow my advice and encapsulate the DispLoop and reload functionality into another function (and therefore the loop will be pretty slim). If you decide to pack everything in here and create a monster loop, I think that the second approach will be better because otherwise the continue will be buried in the code and not be visible that simple

So finally your case statement reduces to setting the reload variable
Code: Pascal  [Select][+][-]
  1. case VK of
  2.   1:
  3.   begin
  4.     Reload := not Condition4;
  5.     if Reload then
  6.       Code6;
  7.   end;
  8.   2:
  9.   begin
  10.     Reload := true;
  11.     Code7;
  12.   end;
  13.   3:
  14.   begin
  15.     Code8;
  16.     Reload := not Condition5;
  17.     if Reload then
  18.       Code10
  19.     else
  20.       Code9;
  21.   end;
  22.   4:
  23.   begin
  24.     Code11;
  25.     Reload := False;
  26.     if not Condition6 and Condition7 then
  27.     begin
  28.       Code12;
  29.       Reload := True;
  30.     end;
  31.   end;
  32.   5:
  33.   begin
  34.     Code13;
  35.     Reload := False;
  36.   end;
  37. end;
Of course how the conditions inside the if are built can be up to you (I like only setting reload an then branching over reload)

Putting it all together
Code: Pascal  [Select][+][-]
  1. var
  2.   Reload: Boolean = true;
  3. begin
  4.   try
  5.     if Condition1 then Exit;
  6.     repeat
  7.       DispLoop(Reload);
  8.       Reload := False;
  9.       If Condition2 then Continue;
  10.       if Condition3 then
  11.         case VK of
  12.         1:
  13.         begin
  14.           Reload := not Condition4;
  15.           if Reload then
  16.             Code6;
  17.         end;
  18.         2:
  19.         begin
  20.           Reload := true;
  21.           Code7;
  22.         end;
  23.         3:
  24.         begin
  25.           Code8;
  26.           Reload := not Condition5;
  27.           if Reload then
  28.             Code10
  29.           else
  30.             Code9;
  31.         end;
  32.         4:
  33.         begin
  34.           Code11;
  35.           Reload := False;
  36.           if not Condition6 and Condition7 then
  37.           begin
  38.             Code12;
  39.             Reload := True;
  40.           end;
  41.         end;
  42.         5:
  43.         begin
  44.           Code13;
  45.           Reload := False;
  46.         end;
  47.       end;
  48.     until not Condition3 or (VK=0);
  49.   finally
  50.     Code14;
  51.   end;
  52. end;
Title: Re: Eliminating Goto/Label
Post by: 440bx on July 17, 2020, 03:07:27 pm
In the code skeleton below I'd like to eliminate the gotos/labels while maintainig a reasonably equivalent clarity in the "flow chart" of the code. How could I do that?
The _correct_ way to do that is to rethink the structure of the program to find one that doesn't require producing spaghetti code.

Here is a "solution" that is _slightly_ less undesirable because it doesn't use any "goto"s but, in spite of not using any "goto"s, it is still a form of spaghetti code. 
Code: Pascal  [Select][+][-]
  1. program TestNoGoto;
  2.  
  3.   procedure LasagnaCode;
  4.   var
  5.     Condition1 : boolean = false;
  6.     Condition2 : boolean = false;
  7.     Condition3 : boolean = false;
  8.     Condition4 : boolean = false;
  9.     Condition5 : boolean = false;
  10.     Condition6 : boolean = false;
  11.  
  12.  
  13.     Reload     : boolean = false;    { pasta  layer                           }
  14.     DispLoop   : boolean = false;    { cheese layer                           }
  15.  
  16.     Scope      : integer = 1;        { used to create an inline scope         }
  17.  
  18.  
  19.   begin
  20.     { no inline scopes in Pascal, "synthesize" one                            }
  21.  
  22.     for Scope := Scope to Scope do   { scope to break out of                  }
  23.     begin
  24.       if Condition1 then
  25.       begin
  26.         break;                  { empty plate : no lasagna today              }
  27.       end;
  28.  
  29.       Reload := true;           { Reload loop control variable                }
  30.       while Reload do
  31.       begin
  32.  
  33.         DispLoop := true;       { DispLoop control variable                   }
  34.         while DispLoop do
  35.         begin
  36.  
  37.           if Condition2 then
  38.           begin
  39.             DispLoop := false; { end DispLoop and continue Reload loop        }
  40.  
  41.             break;             { out of DispLoop                              }
  42.           end;
  43.  
  44.           { NOTE: better than testing for Condition3 and nesting a "case"     }
  45.           {       statement in it, would be to test for "not condition3" and  }
  46.           {       simply exit, thus:                                          }
  47.           {                                                                   }
  48.           {       if not Condition3 then                                      }
  49.           {       begin                                                       }
  50.           {         DispLoop := false;                                        }
  51.           {         Reload   := false;                                        }
  52.           {                                                                   }
  53.           {         break;                                                    }
  54.           {       end;                                                        }
  55.  
  56.           if Condition3 then
  57.           begin
  58.             case Vk of
  59.               0 : begin
  60.                     Reload   := false;        { end pasta                     }
  61.                     DispLoop := false;        { end cheese                    }
  62.  
  63.                     break;
  64.                   end;
  65.  
  66.               1 : begin
  67.                     if Condition4 then
  68.                     begin
  69.                       continue;          { start new DispLoop iteration       }
  70.                     end;
  71.  
  72.                     DispLoop := false;   { start a new Reload iteration       }
  73.                     break;
  74.                   end;
  75.  
  76.               2 : begin
  77.                     DispLoop := false;   { force DispLoop to terminate        }
  78.  
  79.                     break;               { start a new Reload iteration       }
  80.                   end;
  81.  
  82.               3 : begin
  83.                     if Condition5 then
  84.                     begin
  85.                       continue;          { start new DispLoop iteration       }
  86.                     end;
  87.  
  88.                     DispLoop := false;   { start a new Reload iteration       }
  89.                     break;
  90.                   end;
  91.  
  92.               4 : begin
  93.                     if Condition6 then
  94.                     begin
  95.                       continue;          { start new DispLoop iteration       }
  96.                     end;
  97.  
  98.                     if Condition7 then
  99.                     begin
  100.                       DispLoop := false; { start a new Reload iteration       }
  101.                       break
  102.                     end;
  103.                   end;
  104.  
  105.               5 : begin
  106.                     continue;            { start new DispLoop iteration       }
  107.                   end;
  108.  
  109.               { it would be a good idea to have an "otherwise" for safety     }
  110.  
  111.               otherwise
  112.               begin
  113.                 { whatever should be done in this case                        }
  114.  
  115.                 ;          { do nothing - meditate over infinite lasagna      }
  116.               end;
  117.             end; { case }
  118.  
  119.           end; { Condition3 }
  120.  
  121.         end; { DispLoop }
  122.  
  123.       end; { Reload loop }
  124.  
  125.     end; { for scope }
  126.  
  127.     { do whatever here - surgeon general warning                              }
  128.  
  129.     writeln('Abuse of lasagna and spaghetti are not healthy');
  130.   end;
  131.  
  132.  
  133. begin
  134.   LasagnaCode();
  135. end.
  136.  
Neither the code above nor the code you showed is desirable in any way.  There is a better solution, the first step in finding it is to stop coding and rethink the structure of the program.

HTH.

Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 17, 2020, 04:15:36 pm
I'd echo in particular what @Warfley says about try/exit/finally and repeat/continue/break/until.

However I'd suggest that the bottom line is that "clever" code intended to conform to some bit of trendy dogma might actually be more difficult to follow than the alternative "naughty" form.

I'm fairly relaxed about goto, there are cases where it's quite simply the right tool for the job. Just don't over-use it.

MarkMLl
Title: Re: Eliminating Goto/Label
Post by: MaxCuriosus on July 18, 2020, 02:36:13 pm
marcov,
although you didn't use specifically a state variable, the combine use of "break" and "continue" acts as a kind of flip-flop flag or state variable.
All the other proposed solutions also use a form or another of implicit or explicit state variable. And contrary to what others seem to think your code has sufficient clarity, for me at least.
So you get the cigar.
Title: Re: Eliminating Goto/Label
Post by: MaxCuriosus on July 18, 2020, 02:38:12 pm
Warfley,
all your detailed points are well taken. I'd like to add the following:

1. Rather than use try-finally in relation to condition1, I'd prefer to use an additional procedure calling the "LoopCode" and add the "exit" code #14 in it. That
will lighten a little the main code.

2. With the exception of { Code #5 } all the other { code #x } are already reduced to 3-5 lines, one of which is a function with a return value used as a condition. So creating a new function to replace 3-5 lines doesn't add much to clarity.

3. While your procedure "DispLoop" and boolean "Reload" eliminate the explicit loops and reduce the lines of codes, that does not improve clarity w/r/t marcov's solution, in my opinion.
Title: Re: Eliminating Goto/Label
Post by: MaxCuriosus on July 18, 2020, 02:40:25 pm
440bx,
1. What's the point of using two state variables rather than just one, as Warfley has done?

2. What spaghetti code? The structure of the procedure is straigtforward, as Warfley has already summarized. Or in other words, setting aside the the initial condition and the exit code, it's simply:
 
a triage based on the value of a parameter;
the corresponding execution of code with conditions;
and the return to the beginning of the inner or outer loop.

The fact that the triage has to deal with many values of the parameter VK doesn't constitute "spaghetti" code, nor does the number of conditions used. As explained in my previous post many of those conditions are in fact the return value of a function.
Title: Re: Eliminating Goto/Label
Post by: MaxCuriosus on July 18, 2020, 02:41:58 pm
MarkMLI,
I concur, adding that "endless loops" are similar to "goto backwards".
I generally avoid the use of goto-label, but in this particular case I found it easier to use them, although the "Lexit" label can be easily removed, and will be.
Title: Re: Eliminating Goto/Label
Post by: Warfley on July 18, 2020, 03:46:02 pm
And contrary to what others seem to think your code has sufficient clarity, for me at least.
So you get the cigar.

As someone who without knowing anything about your code has read marcovs code before yours (when writing my post marcovs post was right below the editor so i looked there to check what your code was doing while translating it to my solution), I would suggest you, rather than using the loop based code, to stick with goto. Your goto code was understandable by just scrolling through once. With marcovs code, I had to scroll back a few times to remind myself on the loop structure used, because just encountering a break or continue doesn't tell you anything about the location you jump to without knowing the context.

The structure and code is essentially the same as the goto code, you just replaced named labels which by their name convey information about the jump, with anonymous break and continue statements, therefore having the same code with just less immediate information available.

I don't know why you want to remove the gotos, but the reason must be pretty good if you are willing to make your code less readable for that matter.
Title: Re: Eliminating Goto/Label
Post by: marcov on July 18, 2020, 04:05:55 pm
marcov,
although you didn't use specifically a state variable, the combine use of "break" and "continue" acts as a kind of flip-flop flag or state variable.
All the other proposed solutions also use a form or another of implicit or explicit state variable. And contrary to what others seem to think your code has sufficient clarity, for me at least.
So you get the cigar.

Thanks, but I don't smoke. Can we make it a virtual bottle of wine ? : _)

Anyway, I didn't really start out with an idea to avoid state variables. It was more an attempt to remove some gotos but stay close to the original structure, as a first step in a transformation. I immediately recognized that most gotos were either a continue or a break.

The rest were the less elegant things are what to put at the end of both loops, not knowing if we get there or not, and the .Lexit code to run (always) on exit, solved withy try..finally. I can live with the latter as solution (though it takes it from purely procedural into object pascal territory), no problem. The former is a bit more complicated, and can't be answered. Whatever decisions you make, comment it thoroughly, since these are not standard constructs.
Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 18, 2020, 04:10:29 pm
Noting @Warfly's comment about readability, even though labels are obviously predeclared I think it's worthwhile trying to have the definition point (i.e. where the label is actually inserted in the code) before the corresponding goto. So in short, going backwards a few statements is OKish, going forwards rather less so.

I was at a conference once where one of the papers argued that any explicit instruction to terminate a loop (break/continue in the case of Pascal) was tantamount to a goto, hence should be "considered harmful" and avoided in the interest of clarity. But I think that in this thread all of the clear solutions involve one or more of those instructions, and that the solutions with extra functions and/or state variables are inferior.

Another observation I'd make is that the "mathematically inclined" seem to enjoy introducing transcendentals into expressions simply to avoid an "if this is <1 then do this" type of decision. They then attempt to justify the practice by claiming that introducing a sin() or cos() is actually more efficient than the pipeline stall and potential cache miss that a straightforward control transfer might cause. A little knowledge is a dangerous thing...

MarkMLl
Title: Re: Eliminating Goto/Label
Post by: 440bx on July 18, 2020, 04:41:05 pm
440bx,
1. What's the point of using two state variables rather than just one, as Warfley has done?
The point is that it is much more powerful to do that way _and_, with just a little additional code, how the whole thing flows depending on those conditions can be made a lot easier to understand.  I'm not going to say that the code below is "good" because, it still leaves something to be desired but, I do believe it is a very significant improvement over your original code:
Code: Pascal  [Select][+][-]
  1. program TestNoGoto2;
  2.  
  3.   procedure LasagnaCode;
  4.   var
  5.     Condition1 : boolean = false;
  6.     Condition2 : boolean = false;
  7.     Condition3 : boolean = false;
  8.     Condition4 : boolean = false;
  9.     Condition5 : boolean = false;
  10.     Condition6 : boolean = false;
  11.  
  12.  
  13.     Reload     : boolean = false;    { pasta  layer                           }
  14.     DispLoop   : boolean = false;    { cheese layer                           }
  15.  
  16.     Scope      : integer = 1;        { used to create an inline scope         }
  17.  
  18.  
  19.     procedure ExitDispLoop;
  20.     begin
  21.       DispLoop := false;
  22.     end;
  23.  
  24.     procedure ExitDispLoopAndReloadLoop;
  25.     begin
  26.       DispLoop   := false;
  27.       ReloadLoop := false;
  28.     end;
  29.  
  30.  
  31.   begin
  32.     { no inline scopes in Pascal, "synthesize" one                            }
  33.  
  34.     for Scope := Scope to Scope do   { scope to break out of                  }
  35.     begin
  36.       if Condition1 then
  37.       begin
  38.         break;                  { empty plate : no lasagna today              }
  39.       end;
  40.  
  41.       Reload := true;           { Reload loop control variable                }
  42.       while Reload do
  43.       begin
  44.  
  45.         DispLoop := true;       { DispLoop control variable                   }
  46.         while DispLoop do
  47.         begin
  48.  
  49.           if Condition2 then
  50.           begin
  51.             ExitDispLoop;      { end DispLoop and continue Reload loop        }
  52.             break;
  53.           end;
  54.  
  55.           { NOTE: better than testing for Condition3 and nesting a "case"     }
  56.           {       statement in it, would be to test for "not condition3" and  }
  57.           {       simply exit, thus:                                          }
  58.           {                                                                   }
  59.           {       if not Condition3 then                                      }
  60.           {       begin                                                       }
  61.           {         ExitDispLoopAndReloadLoop();                              }
  62.           {                                                                   }
  63.           {         break;                                                    }
  64.           {       end;                                                        }
  65.  
  66.           if Condition3 then
  67.           begin
  68.             case Vk of
  69.               0 : begin
  70.                     ExitDispLoopAndReloadLoop();
  71.                     break;
  72.                   end;
  73.  
  74.               1 : begin
  75.                     if Condition4 then
  76.                     begin
  77.                       continue;          { start new DispLoop iteration       }
  78.                     end;
  79.  
  80.                     ExitDispLoop();
  81.                     break;               { start a new Reload iteration       }
  82.                   end;
  83.  
  84.               2 : begin
  85.                     ExitDispLoop();
  86.                     break;               { start a new Reload iteration       }
  87.                   end;
  88.  
  89.               3 : begin
  90.                     if Condition5 then
  91.                     begin
  92.                       continue;          { start new DispLoop iteration       }
  93.                     end;
  94.  
  95.                     ExitDispLoop();
  96.                     break;               { start a new Reload iteration       }
  97.                   end;
  98.  
  99.               4 : begin
  100.                     if Condition6 then
  101.                     begin
  102.                       continue;          { start new DispLoop iteration       }
  103.                     end;
  104.  
  105.                     if Condition7 then
  106.                     begin
  107.                       ExitDispLoop();
  108.                       break              { start a new Reload iteration       }
  109.                     end;
  110.                   end;
  111.  
  112.               5 : begin
  113.                     continue;            { start new DispLoop iteration       }
  114.                   end;
  115.  
  116.               { it would be a good idea to have an "otherwise" for safety     }
  117.  
  118.               otherwise
  119.               begin
  120.                 { whatever should be done in this case                        }
  121.  
  122.                 ;          { do nothing - meditate over infinite lasagna      }
  123.               end;
  124.             end; { case }
  125.  
  126.           end; { Condition3 }
  127.  
  128.           { should any instructions be executed when the DispLoop is broken   }
  129.           { out of but ReloadLoop will be executed again ?                    }
  130.  
  131.           { if ReloadLoop true/false then do whatever                         }
  132.  
  133.         end; { DispLoop }
  134.  
  135.       end; { Reload loop }
  136.  
  137.     end; { for scope }
  138.  
  139.     { do whatever here - surgeon general warning                              }
  140.  
  141.     writeln('Abuse of lasagna and spaghetti are not healthy');
  142.   end;
  143.  
  144.  
  145. begin
  146.   LasagnaCode();
  147. end.
  148.  
That way, a programmer that reads that can much more easily visualize what happens and when.  Something you don't get with "goto"(s).

2. What spaghetti code? The structure of the procedure is straigtforward, as Warfley has already summarized. Or in other words, setting aside the the initial condition and the exit code, it's simply:
Spaghetti code is any code that causes the programmer's attention to be jumping around in a piece of code.  "goto"s are not the only way to make execution jump around.  The answer to your question is: the code you posted is spaghetti code and the one I posted is dangerously close to spaghetti code (it's lasagna code, strands of cheese spaghetti between layers of pasta.)


The fact that the triage has to deal with many values of the parameter VK doesn't constitute "spaghetti" code, nor does the number of conditions used. As explained in my previous post many of those conditions are in fact the return value of a function.
As stated above, what makes it spaghetti code is that the programmer's attention has to jump all over the place to figure out how that code flows.  That's bad/niente/no good/trump.
Title: Re: Eliminating Goto/Label
Post by: egsuh on July 19, 2020, 06:11:58 am
Why not just write procedures  Reload, DispLoop and Lexit? You may define function/procedure within function/procedure.
Title: Re: Eliminating Goto/Label
Post by: process_1 on July 19, 2020, 09:02:55 am
@MaxCuriosus

Using goto in procedural language as it is Pascal is a mess, but also using too many break and continue make code as well quite messy.

I don't have much time right now to look in your original code in details, but this kind of code can be much clear, i.e. easier to control with additional Boolean variables instead too use too many break and continue loop control.

The tipical code of this type is when parsing text.
Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 19, 2020, 09:23:33 am
Why not just write procedures  Reload, DispLoop and Lexit? You may define function/procedure within function/procedure.

Because you can't do something like

Code: [Select]
while true do begin
...
  Lexit;
...
  Reload
end;

with code you write yourself, you need language-provided primitives. And it's the use/abuse of those primitives which is being discussed: see the copious code posted by various people.

MarkMLl
Title: Re: Eliminating Goto/Label
Post by: egsuh on July 19, 2020, 10:39:55 am
If you have definitive definion of finishing reloading-looping loop, something like following approach should do what you want... of course you have to think over the procedure carefully because each call of functions make stacks.


Code: Pascal  [Select][+][-]
  1. procedure Skeleton;
  2.  
  3. procedure Lexit;
  4. begin
  5.       { Code #14;  }
  6. end;
  7.  
  8. procedure Reload; forward;
  9.  
  10. procedure DisLoop;
  11. begin
  12.       { Code #5 }
  13.       If Condition2 then DispLoop;
  14.       If Condition3 then
  15.           Case VK of
  16.             0:  // Lexit;    ---> This may not be necessary. Lexit will be called at the end of this procedure.
  17.             1:   Begin
  18.                   If Condition4 then DispLoop
  19.                    else begin
  20.                       { Code #6 }
  21.                       Reload;
  22.                    end;
  23.              end;
  24.             2:  Begin
  25.                 { Code #7 }
  26.                 Reload;
  27.             end;
  28.             3:  Begin
  29.                 { Code #8 }        
  30.                 If Condition5 then  Begin
  31.                     { Code #9 }
  32.                      DispLoop;
  33.                  end
  34.                  else begin
  35.                      { Code #10 }
  36.                      Reload;
  37.                 end;
  38.             end;
  39.             4:   Begin
  40.                 { Code #11 }
  41.                 If Condition6 then DispLoop;
  42.                 If Condition7   then  Begin
  43.                       { Code #12 }
  44.                       Reload;
  45.                     end
  46.                    else DispLoop;
  47.               end;
  48.             5:  Begin
  49.                 { Code #13 }
  50.                 DispLoop;  
  51.              end;
  52.        end; {Case}
  53.  
  54.        Lexit;      
  55. end;
  56.  
  57. procedure  Reload;
  58. begin
  59.       { Code #4 };
  60.  
  61.       DisLoop;
  62. end;
  63.      
  64.  Begin    // of procedure Skeleton
  65.       { Code #1 }
  66.       If Condition1 then
  67.         Begin
  68.             { Code #2 }
  69.             Lexit;
  70.             Exit; // Not sure whether this is necessary here.
  71.         end;
  72.       { Code #3 }
  73.      
  74.       Reload;
  75. end;
Title: Re: Eliminating Goto/Label
Post by: kupferstecher on July 19, 2020, 12:07:49 pm
I'm fairly relaxed about goto, there are cases where it's quite simply the right tool for the job. Just don't over-use it.
I'm also relaxed about goto, but I didn't use it in the last several years, I never saw the necessity.

Even if goto would be the ideal solution in on specific case... the code often has to be changed afterwards which may destroy the perfection. And then rearranging such "optimised" code is quite an effort.

E.g. I also quite seldom use for-in-loops. Although I very like the concept and it makes the loop bounds very save, but it happened so often to me that later I needed a loop iterater in the same loop or I had to assign to the indexed variable which is not allowed in a for-in. And also not seldom I changed the loop again, so the for-in, again, would be the better option.
Title: Re: Eliminating Goto/Label
Post by: process_1 on July 19, 2020, 12:45:51 pm
@MaxCuriosus

What you basically do is to display data in the loop, ocassionally reload (whatever), then the simples (at least for me) is following approach:

Code: [Select]
procedure Skeleton;
  isReload: Boolean;

begin
  { Code #1 }
  if Condition1 then
  begin
    { Code #2 }
  end
  else
  begin

    { Code #3 }

    isReload := True;

    while True do
    begin

      // Reload:

      if isReload then
      begin
        { Code #4 }
        isReload := False;
      end;

      // DispLoop:

      repeat
        { Code #5 }
      until not Condition2;

      if Condition3 then
      begin

        case VK of
          0: break;

          1:
          begin
            if Condition4 then
              continue;

            { Code #6 }
            isReload := True;
          end;

          2:
          begin
            { Code #7 }
            isReload := True;
          end;

          3:
          begin
            { Code #8 }
            if Condition5 then
            begin
              { Code #9 }
              continue;
            end;

            { Code #10 }
            isReload := True;

          end;

          4:
          begin
            { Code #11 }
            if Condition6 then
              continue;

            if Condition7 then
            begin
              { Code #12 }
              isReload := True;
            end;

          end;

          5:
          begin
            { Code #13 }
          end;

        end; {Case}

      end; {If Condition3}

    end;

  end;

  //Lexit: ;
  { Code #14 }
end;

Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 19, 2020, 01:35:51 pm
Nobody denies that GOTO can be always be replaced, with varying degrees of inelegance.

The issue is whether it's worth it.

MarkMLl
Title: Re: Eliminating Goto/Label
Post by: process_1 on July 19, 2020, 01:45:53 pm
Well, the main point of this is that ASM style of progamming in higher languages need to be avoided by any cost. It is real headache for maintenence and clear sign for any employer what to do with such programmer.
Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 19, 2020, 03:00:00 pm
That is your opinion. It is not mine.

If a GOTO with strictly defined locality and adequate documentation is the most elegant and intuitive way of doing something, then I consider it to be acceptable.

I'm far more bothered about setjmp() etc. than I am about GOTO, on account of its implicit temptation to jump across block boundaries.

If the block in which a GOTO is syntactically valid is too big to make it obvious what's going on then that is obviously a problem in itself.

I am emphatically not saying that GOTO is acceptable in a context where it could be trivially replaced by repeat/until or some comparable construct. But I don't call introducing loads of single-use Boolean flags "trivial".

I'd also remind you that some hold the belief that since GOTO is "considered harmful" then BREAK, EXIT etc. should also be deprecated.

MarkMLl
Title: Re: Eliminating Goto/Label
Post by: howardpc on July 19, 2020, 03:34:03 pm
Often higher-level Break, Continue and Exit commands are implemented at assembly level by the compiler's code generator using gotos (jmp etc.).
So, like many generalisations, the proverbial wickedness of GoTo, and the need to avoid it, is merely hidden or disguised by high level language constructs, but not ultimately avoided.
GoTo is not intrinsically evil. But it is a construct that is so easily abused to write spaghetti code. Of course, spaghetti can also easily be produced without any gotos.
Title: Re: Eliminating Goto/Label
Post by: process_1 on July 19, 2020, 03:41:13 pm
@MarkMLl

Well, it is not my opinion it is general attitude in programming. Using LABELS and GOTO is considered bad practice in higher languages. These are stuff kids today first learn in school or university.

As a senior programmer (almost retired) which is practically grown on ASM, I do support such attitude that it should be avoided by any cost in higer language, while in ASM that is "natural".

Even more, nowadays compilers makes quite well optimization that such code "complication" to avoid usage of lablels and goto in higher languages, actually is minimum lost or even non existed.

Thus, no need to make source code more complicated than it really is. This may be acceptable in some cases, but certainly shouldn't be a practice.

One parallel. Toyota had an accident some 5 years ago or so with unpredictable car acceleration which was consequance of extensive use of recursion in the software (leading stack overflow, IIRC), which is even strictly forbiden by MISRA standards.

Avio industry also suffer from badly implemented software and "glitches" may cost several hundred lives...

Thus, some rules need to be followed as there is great chances that badly designed and implemented software cost even lives. Fine is we programming for fun, but in industry that is very serious matter.
Title: Re: Eliminating Goto/Label
Post by: devEric69 on July 19, 2020, 03:58:06 pm
It's understandable, and to their credit, that the MISRA standards are against a stack overflow.

Concerning myself, i really prefer to read in nested loops "If Not bAllRight Then goto NextThis;" rather than "If Not bAllRight Then Continue;".
Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 19, 2020, 04:03:40 pm
Well, it is not my opinion it is general attitude in programming. Using LABELS and GOTO is considered bad practice in higher languages. These are stuff kids today first learn in school or university.

They're also taught that C++ and Javascript are a good idea. And they used to be taught that APL was a good idea, with absolutely no consideration of the runtime resources required. And Lisp, Prolog and Smalltalk with no security in their runtime storage. I can play this game for hours... :-)

Quote
As a senior programmer (almost retired) which is practically grown on ASM, I do support such attitude that it should be avoided by any cost in higer language, while in ASM that is "natural".

As an engineer who /ought/ to be close to retirement, I'd argue that a pragmatic approach is to be preferred. And that it's very often appropriate to use block-structured constructs in assembler programming.

Quote
One parallel. Toyota had an accident some 5 years ago or so with unpredictable car acceleration which was consequance of extensive use of recursion in the software (leading stack overflow, IIRC), which is even strictly forbiden by MISRA standards.

Except where the events were due to out-of-place floor mats or provably-inept drivers. But I agree that wilfully ignoring documented standards is a very big problem... which is an argument for keeping documentation and standards simple and then enforcing them mercilessly (which would avoid debacles like "dieselgate" and the deaths due to an undocumented spring change in (Delco?) ignition switches).

And since you mention recursion I'd point out that a whole lot of people who use it probably don't do so knowingly, because they lack the background to understand exactly what the compiler and runtimes are doing on their behalf. I could go on...

Quote
Thus, some rules need to be followed as there is great chances that badly designed and implemented software cost even lives. Fine is we programming for fun, but in industry that is very serious matter.

But again, that argues: keep things simple and intelligible. So if you can do it in five statements plus comments pointing out why you're using a GOTO, it's preferable to using 15.

MarkMLl
Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 19, 2020, 04:05:13 pm
Concerning myself, i really prefer to read in nested loops "If Not bAllRight Then goto NextThis;" rather than "If Not bAllRight Then Continue;".

GRIN

MarkMLl
Title: Re: Eliminating Goto/Label
Post by: process_1 on July 19, 2020, 04:21:22 pm
It's understandable, and to their credit, that the MISRA standards are against a stack overflow.

You are probably not aware on what people are ready to do to save  few cents on hardware (ie maximizing profit) and try to "fix" issues in software, making things even worst. Human lives? Nah, they have bunch of lawyers to cover anything... These mentioned examples are self-explanatory.

And this is as well quite interesting,not far from future massive use:
https://www.youtube.com/watch?v=y3RIHnK0_NE

We are doomed, no doubt! ;)
Title: Re: Eliminating Goto/Label
Post by: Warfley on July 19, 2020, 04:27:09 pm
I do support such attitude that it should be avoided by any cost in higer language
This statement (i.e. avoid goto by any cost) implies that you are happy to replace:
Code: Pascal  [Select][+][-]
  1.     label foo;
  2.     begin
  3.       ...
  4.       goto foo;
  5.       ...
  6.     foo:
  7.       ...
  8.  
with this
Code: Pascal  [Select][+][-]
  1.     type TGotoFooException = class;
  2.     begin
  3.       try
  4.         ...
  5.         raise TGotoFooException.Create;
  6.         ...
  7.       except on e: TGotoFooException do;
  8.       end;
  9.       ...

And I personally don't know anyone who would bring forth that argument.

Goto is not intrinsicly bad, goto often leads to less readable code, but, if you replace goto with something that is even less readable, you just made your code worse, even though you got rid of goto.

Quote
One parallel. Toyota had an accident some 5 years ago or so with unpredictable car acceleration which was consequance of extensive use of recursion in the software (leading stack overflow, IIRC), which is even strictly forbiden by MISRA standards.
[...]
Thus, some rules need to be followed as there is great chances that badly designed and implemented software cost even lives. Fine is we programming for fun, but in industry that is very serious matter.
Yes, but most people don't write safty crucial software for the automobile industry. If a car crashes because of a software bug it results in a lot of damage, including human damage. This must be avoided even at very low risks. If the size calculator for an online shop crashes and the customer has to reload the page, it is annoying but if it doesn't happen to often no one bats an eye.

Very ofter a recursive solution is the easiest solution, and saying that you need to invest a lot more time to change this function for in an online shop software to be iterative because in the automobile industrie this could result in human casulties is a little bit far fetched.

And this might not even be the general case, look at functional languages like Haskell, Haskell can't be used without recursion, and in haskell there is no such thing as a stack overflow (because in haskell there is no such thing as a stack).

These rules are dependent on their context, and while they may be very sensible in the automobile or avionics industry, for your run of the middle webdev or app developer they are simply restricting you, without you gaining anything, because no one cares if your app crashes occasionally as long as most of the time it works fine.

Also, optimizing tail recursion to an iterative function is one of the baseline optimizations most compilers do. So simple recursion will actually be compiled to iterative code anyway
Title: Re: Eliminating Goto/Label
Post by: process_1 on July 19, 2020, 04:53:56 pm

This statement (i.e. avoid goto by any cost) implies that you are happy to replace:
...
with this
...

That is plain insinuation - that is your code! ;)
I would never wrote such code, especially not with raise. ;)

But certainly, GOTO and LABELS are just relicts from the ASM...

As I have wrote already, if we make software for fun - who cares? But developing good habbit in programming is crucial for eventual professional carriere, especially for safty critical products. Embedded programming is quite popular nowadays and MCUs are quite powerful and cheap, but they simply have significantly less resources than classical desktop computer. Thus, lazy programmers used to recursion everywhere have nothing to do in that world...

Programming used to be art long time ago, but nowadays...
Title: Re: Eliminating Goto/Label
Post by: process_1 on July 19, 2020, 05:23:38 pm
They're also taught that C++ and Javascript are a good idea. And they used to be taught that APL was a good idea, with absolutely no consideration of the runtime resources required. And Lisp, Prolog and Smalltalk with no security in their runtime storage. I can play this game for hours... :-)

Tell me more about... ;) Well, I practically lived in pioneers days...

Quote
I'd argue that a pragmatic approach is to be preferred.

That is truth. But anyway, using LABEL and GOTO with procedural languages?!
Title: Re: Eliminating Goto/Label
Post by: MarkMLl on July 19, 2020, 05:31:01 pm
That is truth. But anyway, using LABEL and GOTO with procedural languages?!

I can assure you that in my academic days I marked plenty of students down for it.

Incidentally, I'd suggest not looking at any of Wirth's early code unless you like jump tables in your ALGOL :-)

MarkMLl
TinyPortal © 2005-2018