Replacing repeated lines of code with a function

Question

I work on embedded C and am trying to refactor code to improve readability and optimize ROM used in my project.

I have this 3 lines of code repeats many times in switch-cases to update a particular edit box on a particular screen name.

EbSetText and SetState are predefined graphics library functions.

sprintf(Screen1_Editbox1_Text,"%04d",GetHV(0));
EbSetText((EDITBOX *)GOLFindObject(ID_Screen1_Editbox1), Screen1_Editbox1_Text);
SetState(GOLFindObject(ID_TestProbe_HVEb01), EB_DRAW);

Some code repeats like this:

EbSetText((EDITBOX *)GOLFindObject(ID_Screen1_Editbox1), "Text to output");
SetState(GOLFindObject(ID_TestProbe_HVEb01), EB_DRAW);

The above pieces of code repeats 120 times for various text-boxes and strings/buffer inputs present.It hurts readability of my code mainly and i am nearing 90% of my ROM.

I am thinking of replacing it with a utility function to contain all above logic.

SetTextofEditBox(Screen1_Editbox1_Text,ID_Screen1_Editbox1);

Does replacing it with single utility function give any advantage here?

Answer 1

As unwind said: it might be possible that a function call turns out to be more expensive than what you have now. Of course, bulky functions and repeating the same code makes for ugly code, that is harder to maintain and as a result: more error prone.

I'd personally consider using an inline function, and let the compiler decide if the code should be executed locally, or a function call is indeed a better option.
Or, if you want to, you could simply use a macro, too.

inline is generally considered the better option, and looking at those tiny snippets of code you posted, I'd say you should probably go for inline functions here, too.
Remember that inline leaves it to the compiler to decide if the function is going to be inlined or not, so if -after some serious, and representative testing- you feel as though the function should always be inlined, and you are using gcc, you could use the GCC always_inline attribute:

__attribute__((always_inline)) void your_inline_func(void *x, const char *y){}

If your compiler can't be persuaded to always inline the function so easily, then you'll have to revert back to using a macro. Be advised: macro's are not type safe, and can have side-effects: thorough testing is a must.

Based on the 2 statements you posted, one way of making your code more "data-drive", as unwind suggested could be this:

const char *txt_argument;
//and for any other argument you might need, too:
EDITBOX *edit = NULL;
//possibly add SetState's  arguments
switch(foo)
{
    case bar:
        txt_argument = Screen1_Editbox1_Text;
        edit = (EDITBOX *)GOLFindObject(ID_Screen1_Editbox1);
        break;
    case zar:
        txt_argument = "Text to output";
        edit = (EDITBOX *)GOLFindObject(another_editbox);
        break;
}
EbSetText(edit, txt_argument);

That doesn't really reduce the amount of code you have by much, but it does make it easier to oversee what the switch is actually doing.

If you are looking into ways to (further) optimize this one big switch statement then you could look into nesting the switch cases. It could well be that this has little of no effect, but for some compilers, and in some cases, it can make a difference.

If you have 120 cases, try to see if you can't break them up into 2 or 3 groups: those that are very common, those that are likely, and those cases that occur rarely. The most common cases go in the main switch, then the default contains another switch that lists the less common cases, and the default there, in turn, is the switch that deals with the rarer cases.
Nesting is of course prone to messy code, if you like, you can replace the default: switch bit simply with a series of shorter switch-es, followed by a return...

Answer 2

Perhaps. It's hard to say for sure, it depends on whether the overhead of the extra function call will be greater than the overhead of doing the work locally.

You could try breaking it out and making it a bit more "data-driven", i.e. arrange the code so that the EbSetText() and SetState() functions are always called after the switch, but make their arguments into variables that are set properly by each case.