Go Down

Topic: Whats up with GOTO command? (Read 9 times) previous topic - next topic

AznCaleb

Hey I'm an arduino newb and I was wondering why the use of GOTO command is discouraged in C and similar languages. In my opinion it makes the code much more modular and easier to edit specific sections of code. Is there a better way or a reason why the use of goto is discouraged? if there is, is there a way I can enhance this code?
Code: [Select]

//---------------------------------------int PIN definitions------------------------------------
// THere are 7 LEDs and 3 pushbutton switches, the pushbuttons have harware debouncing circuits so LOW registers when the button is pressed
int SW1 = 11;                          // push-buttons
int SW2 = 12;
int SW3 = 10;

int timer = 75;                        // LED timer
int pins[] = { 2, 3, 4, 5, 6, 7, 8 };  // array of the LED pins
int num_pins = 7;                      // the number of LEDs
long randomNumber;                     // random number generator                    

//-----------------------------------------Setup------------------------------------------------
void setup()
{
 int i;                            
 
 pinMode (SW1 , INPUT);               // I/O definitions
 pinMode (SW2 , INPUT);
 pinMode (SW3 , INPUT);
 
 for (i = 0; i < num_pins; i++)       // the array elements are numbered from 0 to num_pins - 1
   pinMode(pins[i], OUTPUT);          // set each pin as an output for LEDs    
 
}
//-----------------------------------------Main Loop------------------------------------------------    
void loop()
{
 // reads 3 pushbuttons for cases then goes to subroutines
 // HIGH = button pressed = 0 , LOW = button not pressed = X

Buttons:
{
 if (digitalRead (SW1) == HIGH && digitalRead (SW2) == HIGH && digitalRead (SW3) == HIGH)    //-----0----0----0------    
 goto LED_1;
 
 if (digitalRead (SW1) == LOW && digitalRead (SW2) == HIGH && digitalRead (SW3) == HIGH)     //-----X----0----0------    
 goto LED_2;
 
 if (digitalRead (SW1) == HIGH && digitalRead (SW2) == LOW && digitalRead (SW3) == HIGH)     //-----0----X----0------    
 goto LED_3;

 if (digitalRead (SW1) == HIGH && digitalRead (SW2) == HIGH && digitalRead (SW3) == LOW)     //-----0----0----X------    
 goto LED_4;
 
 if (digitalRead (SW1) == LOW && digitalRead (SW2) == LOW && digitalRead (SW3) == HIGH)      //-----X----X----0------    
 goto LED_5;
 
 if (digitalRead (SW1) == LOW && digitalRead (SW2) == HIGH && digitalRead (SW3) == LOW)      //-----X----0----X------    
 goto LED_6;

 if (digitalRead (SW1) == HIGH && digitalRead (SW2) == LOW && digitalRead (SW3) == LOW)      //-----0----X----X------    
 goto LED_7;
 
 if (digitalRead (SW1) == LOW && digitalRead (SW2) == LOW && digitalRead (SW3) == LOW)       //-----X----X----X------    
 goto LED_8;
}  

//-----------------------------------------Subroutines-------------------------------------------------
// These are the LED "subroutines" using the goto statements to come and go to the main loop.

//-----------------------------------------                                                        
LED_1:
 {                                                // Random Lighting
   int i;
   for (i = 0; i < num_pins; i++)
   {  randomNumber = random(0, 2);
    if (randomNumber == 0)
      digitalWrite (pins[i], LOW);  
      delay(timer);
    if (randomNumber == 1)
      digitalWrite (pins[i], HIGH);    
      delay(timer);
   }
  for (i = num_pins - 1; i >= 0; i--)
   {  randomNumber = random(0, 2);
    if (randomNumber == 0)
      digitalWrite (pins[i], LOW);  
      delay(timer);
    if (randomNumber == 1)
      digitalWrite (pins[i], HIGH);
       delay(timer);    
   }
goto Buttons;
 }
//-----------------------------------------  
LED_2:                                      // Move to the Left
 {
    int i;
    for (i = 0; i < num_pins; i++)
   {                                                
     digitalWrite(pins[i] && pins[i+1], LOW);      
     delay(timer);                                
     digitalWrite(pins[i], HIGH);                  
   }
     for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i], LOW);
     delay(timer);
     digitalWrite(pins[i], HIGH);    
   }
   
goto Buttons;
 }
//-----------------------------------------  
LED_3:                                     // Cylon lighting
 {
  int i;
  for (i = 0; i < num_pins; i++)
   {                                  
     digitalWrite(pins[i], LOW);      
     delay(timer);                  
     digitalWrite(pins[i], HIGH);    
   }
   for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i], LOW);
     delay(timer);
     digitalWrite(pins[i], HIGH);    
   }
goto Buttons;  
 }
//-----------------------------------------
LED_4:                                  // Move to the Right
 {
    int i;
    for (i = 0; i < num_pins; i++)
   {                                    
     digitalWrite(pins[i] , LOW);      
     delay(timer);                    
     digitalWrite(pins[i], HIGH);      
   }
     for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i] && pins[i+1], LOW);
     delay(timer);
     digitalWrite(pins[i], HIGH);    
   }

goto Buttons;  
 }
//-----------------------------------------
LED_5:                                  // Fill to the Left
 {
    int i;
    for (i = 0; i < num_pins; i++)
   {                                  
     digitalWrite(pins[i], LOW);      
     delay(timer);                    

   }
     for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i], HIGH);  
     delay(timer);  
   }
 goto Buttons;
 }
//-----------------------------------------
LED_6:                                  // Pocketed Fill
 {
   int i;

   for (i = 0; i < num_pins; i++)
   {                                  
     digitalWrite(pins[i++] , LOW);      
     delay(timer);          
           delay(timer/2);          
   }
 
   for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i--] , HIGH);
     delay(timer);        
           delay(timer/2);          
   }

   for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i--] , LOW);
     delay(timer);        
           delay(timer/2);        
   }
     
   for (i = 0; i < num_pins; i++)
   {                                
     digitalWrite(pins[i++] , HIGH);      
     delay(timer);          
           delay(timer/2);        
   }
goto Buttons;
 }
//-----------------------------------------
LED_7:  
 {                                      // Fill to the Right
    int i;
    for (i = 0; i < num_pins; i++)
   {                                  
     digitalWrite(pins[i], HIGH);    
     delay(timer);                    

   }
     for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i], LOW);  
     delay(timer);  
 }  
goto Buttons;  
 }  
//-----------------------------------------
LED_8:                                   // Bouncing Bar
 {
   int i;
   for (i = 0; i < num_pins; i++)
   {                                  
     digitalWrite(pins[i], LOW);      
     delay(timer);                    
   }  
   for (i = 0; i < num_pins; i++)
   {                                  
     digitalWrite(pins[i], HIGH);      
     delay(timer);  
   }
  for (i = num_pins - 1; i >= 0; i--)  
   {
     digitalWrite(pins[i], LOW);
     delay(timer);
   }
   for (i = num_pins - 1; i >= 0; i--)
   {
     digitalWrite(pins[i], HIGH);  
     delay(timer);        
  }  
goto Buttons;
 }

//-----------------------------------------
}

AlphaBeta

Quote
Hey I'm an arduino newb and I was wondering why the use of GOTO command is discouraged in C and similar languages.

Because it ruins readability and executionflow.

Functions return exectuion to where it was called. goto breaks the flow, and just jumps straight to a label.

Quote
In my opinion it makes the code much more modular and easier to edit specific sections of code.

This is even more 'true' if you use functions.


Quote
Is there a better way or a reason why the use of goto is discouraged? if there is, is there a way I can enhance this code?

The better way is using functions, and I suggest you convert to using them.

:)

RuggedCircuits

Easily replaced with functions:
Code: [Select]
while (1) {
if (digitalRead (SW1) == HIGH && digitalRead (SW2) == HIGH && digitalRead (SW3) == HIGH) {
  led_1();
}
if (digitalRead (SW1) == LOW && digitalRead (SW2) == HIGH && digitalRead (SW3) == HIGH) {
  led_2();
}

  /* etc. */
}


Or even cooler:
Code: [Select]
typedef void (*ledfunc)(void);
static ledfunc funcs[] = {
 led_8, // 000 = LOW LOW LOW
 led_5, // 001 = LOW LOW HIGH
 // etc.
 led_1  // 111 = HIGH HIGH HIGH
};

void loop()
{
 unsigned char pb;

 while (1) {
   pb = ((digitalRead(SW1)==HIGH) ? (1<<2) : 0) |
       ((digitalRead(SW2)==HIGH) ? (1<<1) : 0) |
       ((digitalRead(SW3)==HIGH) ? (1<<0) : 0);
   (*(funcs[pb]))();
  }
}

retrolefty

#3
May 06, 2009, 07:36 am Last Edit: May 06, 2009, 07:37 am by retrolefty Reason: 1
When several of the newer 'structured' high level languages (C, Pascal, etc) were being popularized in the 70s and 80s, the GOTO was given as the poster child of evil coding practices. In short programs it's not really a big (let alone evil) problem, but can you imagine having to read and understand a 12 page program that someone else wrote with lots of GOTO statements? The results of lots of GOTO statements in a large program lead some to call it "spaghetti code" and held as an example of the evil needing to be rid of.

Today even modern BASIC compilers utilize structured program features such that you have no need to use GOTOs at all. However the same can't be said for assembly language where jump-to-a-label is the equivalent of a GOTO and is not really possible to not utilize it in that language.

Lefty

Robert Lee

Code reuse is another good reason not to use GOTO.

By using GOTO and including the LED_1 code as part of the Loop routine, you can't reuse the LED_1 code from another place in the application. That means that in order to do the same work from another routine you would have to rewrite the same code again (or copy / paste it).

Not only does this consume more room on the device, but it leads to doing the work twice, and potentially having to work on the same code in multiple places. This becomes especially painful if you find that there was a small mistake in the LED_1 code, and now you have it copied in 6 places that need to be fixed.

This may seem trivial in such a small app, but it becomes far more important as an app grows.

Ray Andrews

GOTO is evil, plain and simple  >:(

Seriously though you can always write code better without it. You should see your LED sequences as separate functions, not just locations further down in the program. Anything you 'need' to GOTO should probably be a function.

This could be simplified more, but it integrates with what you've already written if you change the goto labels to proper functions

Code: [Select]

void loop()
{
 int buttonval = 0;
 
 if (digitalRead(SW1) == LOW) buttonval += 1;
 if (digitalRead(SW2) == LOW) buttonval += 2;
 if (digitalRead(SW3) == LOW) buttonval += 4;
 buttonval += 1;

 switch (buttonval)
   {
   case 1 : LED_1();  break;
   case 2 : LED_2();  break;
   case 3 : LED_3();  break;
   case 4 : LED_4();  break;
   case 5 : LED_5();  break;
   case 6 : LED_6();  break;
   case 7 : LED_7();  break;
   case 8 : LED_8();  break;
   }
}

AznCaleb

Wow that makes way more sense, I was reading up on functions and how they are used but I guess it didn't really make sense to me, I tried to use them in this program too but I messed up with the syntax so I just used goto. Thanks for all the help!

crimony

Quote
GOTO is evil, plain and simple


Not true.

Quote
Seriously though you can always write code better without it.


Also not true, and it depends on what you mean by "better". If by "better" you mean any of "more readable", "faster" or "more efficient" then there are many cases where *not* using goto will result in "worse" code.

There's a good discussion on this in the lkml (Linux kernel mailing list) archive from 2003. http://kerneltrap.org/node/553/2131

In this particular case, goto shouldn't be used, and it's initial use reflects the O.P.'s inexperience and that's fine, but making unilateral statements about a language feature that is useful in a small subset of problems is unhelpful. A better rule is "don't use goto unless you have exhausted all other language features and goto still looks better"

Ray Andrews

Quote
there are many cases where *not* using goto will result in "worse" code


Just for the sake of discussion (not trying to argue or anything) can you provide an example or two.  This is one of those topics that people love to fight about, but I just like to see how other people think.

RuggedCircuits

This is pretty much the only time I think it should be used (i.e., as a replacement for the structured exception handling in languages like C++/Java/Python/etc.):
Code: [Select]

int trysomething(void)
{
 char *buf = 0;
 success = 0;

 if (do_something() == 0) {
       print_error(1);
       goto fail;
 }

 buf = (char *)malloc(100);

 if (do_something_else() == 0) {
       print_error(2);
       goto fail;
 }

  ...

 success = 1;

fail:
 if (buf) free(buf);
 return success;
}

Ray Andrews

#10
May 08, 2009, 04:49 am Last Edit: May 08, 2009, 04:52 am by SteelToad Reason: 1
My C is still a little rusty but I would use something like this.  Putting the potentially dangerous code in trysomething_internal and using trysomething as a wrapper to report errors and freeing allocated memory.

Code: [Select]

int trysomething(void)
 {
 char *buf = 0;
 int success = 1;
 int error = trysomething_internal(buf);

 if (error != 0)
   {
   success = 0;
   print_error(error);
   }
 if (buf) free(buf);
 return success;
 }
 
int trysomething_internal(char *buf)
 {
 if (do_something == 0) return 1;
 buf = (char *)malloc(100);
 if (do_something_else() == 0) return 2;
 return 0;
 }

RuggedCircuits

Sure, you never *have* to use goto and you will always be able to find a way around it. But I would argue that in this case (pseudo exception handling), the single-function top-to-bottom approach with goto is easier to read. No passing buf back and forth, no return codes to remember (and get wrong), etc.

mem

#12
May 08, 2009, 06:19 am Last Edit: May 08, 2009, 06:22 am by mem Reason: 1
I wonder if the discussion about the  use of goto for structured exception handling is out of place in a thread on Arduino syntax.

Arduino users coming across this thread may interpret some of the posts here as endorsing the use of goto in Arduino code. I would expect that all the above posters would agree that anyone seeking advice about syntax would be best advised to stay well away from goto.

retrolefty

Well I would argue that if there is a function, feature, or structure built into C, then there is very likely a time or place where it might very well be the best method available.

Misapplying or abusing a tool is not the fault of the tool nor does it justify throwing the tool away. It's the person using the tool that determines if it's the proper use of the tool. GOTO is simply a tool to use effectively or not, based on the skill of the programmer using it.

I would think that the world of computer programing would be one of the last places that would preach dogma (GOTO == evil, never use). But then again we all know that Windows is evil also  ;)

Lefty

mem

#14
May 08, 2009, 08:20 am Last Edit: May 08, 2009, 08:21 am by mem Reason: 1
Lefty, the point is not about dogma, its that this forum is for Arduino (which, according the the home page is intended for artists, designers, hobbyists). IMO, discussions on arcane usage of C does not serve the arduino user looking for help and advice on syntax, and seems more appropriate for threads in technical forums like avrfreaks.

Go Up