How to avoid code duplication in a function?

scswift:
Is one going to be harder to debug? I would say the answer is yes.

How so? In one case mode is function scope and the other its module scope. Do you have multiple "mode" variables in the module in question?

In which case I'm not sure what point you're trying to make here.

There are valid reasons to broaden the scope of data. From your description, the data cries out to be module scope.

Or, you can bite the bullet and make your code much more object-oriented.

Are you assuming my project is small? It is not.

You have an interesting definition of "not small". We are talking about an AVR processor. :wink:

Is it likely some other lib uses a global mode variable and modifies it? Probably not. Would the compiler throw an error even if it did? Maybe.

A problem easily avoided by putting "static" in front of mode.

Really, the reason I do this is not because I think I need to, but because it would be embarrassing to have other coders look at my code and see I've used globals everywhere. :slight_smile:

Limiting scope is always a good thing. But, limit it too much and you'll be unnecessarily pulling your hair out.

I'm clearly missing something here; why not pass the relevant variables by reference?

No, but a library I link in later might have a global variable called "mode" which conflicts with my own. That's one reason to avoid using globals.

There are valid reasons to broaden the scope of data. From your description, the data cries out to be module scope.

The device I'm programming is a light and sound effects board. My main program is skeleton code which will be used in every case. I then have a specific function, we'll call it statemachine(), which is different for every project. That code runs in the main loop, uses the timing code from the main loop, uses the inputs read in the main loop, and then does whatever it wants to do with that data.

My goal is to get the code to a state where the only function you need to change is that one statemachine() function, and maybe a few helper function to keep the code neat. But having a bunch of globals in the main program that have to be altered as well is a no-no. I don't want myself or the end user to have to alter the main program. That makes a mess of things.

Since this mode variable is specific to the project, I don't want it mixed in with the other global variables.

I guess the ideal way to handle this would be to have each project be it's own library so it can have its own globals, but I ran into issues with that. I don't remember what those issues were, but I don't have time right now to muck about with it and try to shove it all into a separate library again.

Anyway that's why I don't want to make the variable global.

You have an interesting definition of "not small". We are talking about an AVR processor. :wink:

I meant relative to your typical Arduino project that blinks a few LEDs. Also, I've got a 1284P in my board so I do have a little more program space and ram to work with than is typical. :slight_smile:

Is it likely some other lib uses a global mode variable and modifies it? Probably not. Would the compiler throw an error even if it did? Maybe.

A problem easily avoided by putting "static" in front of mode.

I don't understand. Static on a global variable?

wildbill:
I'm clearly missing something here; why not pass the relevant variables by reference?

I suppose I could use a function parameter, but that doesn't allow me to access the constants used to define the different modes. Also it's not a particularly pretty solution if I have several variables I need to access.

scswift:

pYro_65:

I also can't use an inline function, because although the code in those is supposed to just replace the function call, the compiler can decide not to do that which means trying to call variables local to the function holding my switch statement is a no-no.

You are also using GCC in the Arduino environment, so you can use GCC function attribute 'always inline':

I would prefer not to use compiler specific things. I'm trying to get into good coding practices in C, not GCC specifically.

Well use a C compiler, you are in C++ land my friend. Even without using attributes, I have managed to get every function that I need in lined to be so, maybe a reorganisation of how you declare your functions is in order, wildbill points out a major player. The only place you will have trouble is with template functions ( newer GCC does not have problem ).

Static on a global variable makes it visible to the compilation unit it is defined in, other units cannot see it, if they can see the declaration ( if in header ) then they get their own copy.

Problem: You want certain data to be available to more than one function, but you don't want to make that data global.

Solution 1: Declare a class, making both the data and the functions members of that class.

Solution 2: Wrap the data and functions in a namespace.

scswift:
This check is always the same. So I end up with duplicate code.

Back to the original question, why not pass arguments?

I can't think why you need to duplicate code like you describe. Either pass arguments, or use a function object if you need to retain state. Functions can recurse (even outside Lua) so I don't see what the big issue is there.

Classes, like dc42 suggests, are another way. Slightly different but related.

I don't know why none of the usual suspects have asked this yet: could you post the offending code?

The offending code is really long, and it wasn't really necessary to explain the problem. Anyway I've gotten lots of good suggestions, thanks guys. :slight_smile:

scswift:
Hey guys,

I've got a state machine in my code (a big switch statement), and in that state machine in several states I need to check to see if a switch has been flipped, and perform an action.. namely light an LED, play sound, and change a mod variable. This check is always the same. So I end up with duplicate code.

In other programming languages I've used, I'd just declare a function within a function, and that function would have access to all the parent function's variables, but I can't do that in C.

I'm not seeing the problem yet. The final paragraph is the only place that you've hinted the behaviour of the 'duplicate code' needs to be different in the different places it occurs. If it's actually the same behaviour, just stick it in a function. If it's similar behaviour and the only difference is the data involved then parameterise the parts of the behaviour that need to vary and pass the parameters in as function arguments. So far I don't see anything unusual about what you're trying to achieve. If I'm missing the point then perhaps you'd better post some sample code to demonstrate your current approach and see if any improvements can be suggested.

dc42:
Problem: You want certain data to be available to more than one function, but you don't want to make that data global.

Solution 1: Declare a class, making both the data and the functions members of that class.

+1

This is exactly what OOP is for

So any suggestions?

I don't see why you cannot use a function to group those tasks together.

To communicate with the function, you can use parameters, or global variables, or a "payload": a collection of dissimilar things in a struct. So before executing that function, you load the payload with the right parameters / pointers, and then execute the function (passing the payload to it).

dhenry:

So any suggestions?

I don't see why you cannot use a function to group those tasks together.

To communicate with the function, you can use parameters, or global variables, or a "payload": a collection of dissimilar things in a struct. So before executing that function, you load the payload with the right parameters / pointers, and then execute the function (passing the payload to it).

Maybe some example code will help explain:

          if (port::state[PORT_TOGGLE_MODE] == LOW) { // If mode switch is on...
            
            // Play sound here?
            
            led::set(16, 1); // Turn on slo-blo LED.
            mode = MODE_SLOBLO;
            
          } else {
            
            led::set(16, 0); // Turn off slo-blo LED.
            mode = MODE_FULLSTREAM;  
            
          }

This is the block of code I want to duplicate. If I were to just copy this into a function and pass the 'mode' variable, the constants PORT_TOGGLE_MODE, MODE_SLOBLO, and MODE_FULLSTREAM would no longer be accessible. So I would have to duplicate those in my function, or make them global. Neither being a very pretty solution.

Of course what some of the others have suggested, using a namespace, as I have done with led and port there, is probably the right answer. I'm not sure why some have suggested using a class though. When I wanted to know how to set up my leds I'd asked about using classes and I got yelled at because the leds are not objects, just elements in an array. :slight_smile: And since all this code is not an object which I would ever have more than one of, it doesn't seem to make sense to use a class for it either.

When I wanted to know how to set up my leds I'd asked about using classes and I got yelled at because the leds are not objects, just elements in an array. And since all this code is not an object which I would ever have more than one of, it doesn't seem to make sense to use a class for it either.

It makes enough sense to be useful.
A class can be an object or an interface, if you only have static members you can treat it like a namespace. The advantage being the protected and private areas.

scswift:
This is the block of code I want to duplicate. If I were to just copy this into a function and pass the 'mode' variable, the constants PORT_TOGGLE_MODE, MODE_SLOBLO, and MODE_FULLSTREAM would no longer be accessible. So I would have to duplicate those in my function, or make them global. Neither being a very pretty solution.

Static class members (of the appropriate class). Or globals passed as references to the objects upon construction.

Of course what some of the others have suggested, using a namespace, as I have done with led and port there, is probably the right answer. I'm not sure why some have suggested using a class though. When I wanted to know how to set up my leds I'd asked about using classes and I got yelled at because the leds are not objects, just elements in an array. :slight_smile:

Not by me :slight_smile:

And since all this code is not an object which I would ever have more than one of, it doesn't seem to make sense to use a class for it either.

Singleton.

The only difference (from a logical design standpoint) between (1) namespace (2) Class with all static members and (3) Class with static and non-static members, is that with 1 and 2 if you change your mind later and want "more than one of them", you have to edit a bunch of code, both "client" ( the code that is calling it) and "server" (the code that is being called).

IMO :slight_smile:

I come from mostly a large-scale-project environement, and such approaches are often not "worth it" in the uC-- especially hobbiest or prototype -- environment. But the concerns you raise about your project make these approaches more relevant.

Cheers,
John

ref
"GOF Design Patterns"
"Lakos Large-Scale C++ Software Design"

scswift:
This is the block of code I want to duplicate. If I were to just copy this into a function and pass the 'mode' variable, the constants PORT_TOGGLE_MODE, MODE_SLOBLO, and MODE_FULLSTREAM would no longer be accessible.

You've already got a class, have you? Or a namespace? Can't this extra "code I want to duplicate" be another function inside the class and have access to those constants?

Can you post enough code to demonstrate the whole problem? Like, the class/namespace, and two blocks of code that you want to duplicate (are duplicates of each other).

From what you've posted I don't see the problem.

No, the functon that code is inside of is neither in a class or namespace.

Can you post enough code to demonstrate the whole problem? Like, the class/namespace, and two blocks of code that you want to duplicate (are duplicates of each other).

From what you've posted I don't see the problem.

I'll try to explain it a little better.

I have my main program. I have my main loop in my main program. My main loop handles timing and input and puts that data in globals so functions can access it. And it calls a function, protonpack():

/* ---
Main loop - Executes continuously while the system is powered.
--- */

  void loop() {

    // Constants:
    // Statics: (These are initialized once and their value is maintained on each subsequent loop.)
    // Temporary variables:
       
    // Timing:
   
      lastTime = time;                           // Store start time of last update.
      time = millis();                           // Get current system time.
      timeDelta = time - lastTime;               // Calculate how long last update took, in milliseconds.
      timeDeltaSec = float(timeDelta) / 1000.0;  // Convert last update period into seconds.
      //Serial1.println(timeDelta);

  // Read all switch states:
    readIOPorts();
      
  // This is where all the magic happens:
    protonpack();
      
  // Update LEDs:
    updateLEDs(); 
 
  // Play any queued sound effects once the current sound effect ends:
    updateSound();
 
  // Calculate time left in this frame and delay if needed.
  //  timeDelta = millis()-time;
  //  if (timeDelta < 16) { delay(16-timeDelta); }
 
}

That function contains all the code that makes the board do all this:

And the contents of that function are basically a large switch statement that performs whatever actions are needed based on the current state.

But let's say I want to reconfigure the board to do this:

In that case, I would like to replace the call to protonpack() with a call to ghosttrap() and not have to mess with any of the code in my main program. And sticking a bunch of globals in my main program which are specific to protonpack() is a big no-no, because then I'm altering my main program and I, or whomever else is modifying the code, might accidentally delete something they shouldn't, or do something else to break everything.

I'm basically trying to keep the code that drives everything behind the scenes, and everything protonpack() does, completely separate from eachother.

And it seems like sticking it in a namespace or class might be the best solution, although I am not sure if the IDE will allow me to put that code in a tab by itself rather than sticking it in my main program where I don't want it.

scswift:
I'm basically trying to keep the code that drives everything behind the scenes, and everything protonpack() does, completely separate from eachother.

And it seems like sticking it in a namespace or class might be the best solution, although I am not sure if the IDE will allow me to put that code in a tab by itself rather than sticking it in my main program where I don't want it.

When defining a new class, typically you split it into a declaration, which goes in a .h file in one tab, and its member definitions, which go in a .cpp file in another tab. Then you #include the .h file in your main sketch.