0

I have a long and confusing static method in one of my classes. It is full or error checking code and as a consequence is turning into unreadable spaghetti! It looks something like this:

void myMethod(int foo, int bar) 
{
    int y = functionCall(foo);
    if (!y)
    {
        int x = functionCall(bar);
        if (!x)
        {
            // lots of code with further nested ifs for error checking
            // it all starts to get a bit confusing
        }
        else
        {
            // error handling
        }
     }
     else
     {
         // error handling
     }
}

So that the code is readable and more modular (allowing me to more easily test/ maintain etc) I would like to break it down into some smaller functions. This is not about code reuse as the functions will only ever be called from this one place - it is purely about readability and making 100's of lines of complicated code more understandable for humans.

So my question is this.

If I am to do this will I lose efficiency as I am making unnecessary calls and so extra work for the processor?

If I make these smaller functions should I declare them inline to help the linker realise that they are only used by this one function and should be blown up in place?

Will the linker be able to manage this kind of optimization itself?

Finally if I am to declare it inline what is the correct way to do this?

Should I put the inline function declaration in the header file and the code body in the .cpp file? 

i.e.

in MyClass.hpp :

    inline static int myMethodPart1();

in MyClass.cpp

    int MyClass::myMethodPart1()
    { /* body */ }

Or should I perhaps not declare it in the header or ..... ?

Sam Redway
  • 7,605
  • 2
  • 27
  • 41
  • keyword `inline` has nothing to do with inlining code. and compiler are good to inline code. So just write your smaller functions (probably as static or in unnamed namespace). – Jarod42 Aug 23 '15 at 14:51
  • will this not have a negative impact on the codes performance making extra unnecessary function calls? – Sam Redway Aug 23 '15 at 15:07
  • 1
    Compiler should inline code with optimisation enabled, so no overhead. – Jarod42 Aug 23 '15 at 15:11
  • Inlining is a complicated beast. If you are defining a function in the header, marking it `inline` can help avoid issues with those definitions conflicting across cpp files. Some compilers do that implicitly, I suggest never depending on compiler behavior. However, the intended behavior of getting the compiler to actually inline the function where it is called is largely a lost cause. If anything, it's the definition in the header which shoves the compiler, not the `inline` keyword itself. I don't believe you are allowed to define an inline the way you are, period. –  Aug 23 '15 at 15:13
  • This might be useful for that bit: http://stackoverflow.com/questions/10103161/inline-keyword-vs-header-definition http://stackoverflow.com/questions/2082551/what-does-inline-mean there are also other questions on here regarding this topic if you're a brave soul. –  Aug 23 '15 at 15:14
  • The question is not so much "when to use the inline keyword" more - what is the most efficient way to break up my long function. From the answers I guess it is a case of just do it as a bunch of static methods which can then be called from the main method and let the compiler worry about optimising? – Sam Redway Aug 23 '15 at 15:16
  • Yes, I know. That topic is kind of tangential which is why I didn't say this is a duplicate of those or something. But I wanted to try to provide that information. I think your actual question might be too broad / a matter of opinion. I personally don't see a *need* to break up what you posted at all [maybe were it a loop], but I could see argument to chop out the two first-level bracketed sections. –  Aug 23 '15 at 15:19
  • The point is that the section contains hundreds of lines of code with nested loops and if statements that are hard to read, and hard to test. It is also multithreaded so I can't go through with a debugger so the section is a nightmare to work with. But I worry that breaking it into smaller functions is bad for performance. I wondered what the standard approach was in this situation. – Sam Redway Aug 23 '15 at 15:23
  • Well, that changes it. Based on what you posted it's quite hard to tell what's actually going on in those conditionals. I think I can post an actual answer addressing that. –  Aug 23 '15 at 15:26

1 Answers1

0

With regards to how to organize and divide the code, I would say you need to use good judgment. If you feel it is a problem big enough to post here, then addressing it is probably worth the effort. That said, I will try to address the components of your post individually.

The cost of function calls. Function calls are insanely cheap. The system essentially just dereferences a single pointer and it’s there. Similar already happens in loops, conditionals, and other forms of branching. Declaring:

While( x != 0 )
{
    Do stuff;
}

Will compile, at a low level, to effectively having “do stuff;” as a separate function called repeatedly. As such, the cost of splitting your function into multiple functions is low and possibly, if done cleanly and with a smart compiler, non-existent.

Regarding inlining. As I explained in the comments, the inline keyword does not mean (quite) what you think it means and what it suggests it means. Compilers have a tendency to ignore inline with regards to actually inlining the function, and at best take it as a suggestion. What inline does do is prevent multiple definitions of the function from becoming an error. This is important behavior if you define a function within a header, because that function definition will be compiled into every cpp's object file. If not declared inline, linking these objects into an executable can generate a multiple definition error. Some compilers implicitly inline functions defined in such a way, but you should never depend upon compiler-specific behavior.

Actually getting a function inlined is to an extent up to the good graces of the compiler. I have seen it stated, although I cannot now find where, that defining a function within the class declaration (in the header) is a fairly strong nod to the compiler to inline.

That said, as I noted before, inlining is not a particularly important matter. The cost of calling a function is insanely low, and really the only area one should be concerned about it is in functions called often - like getter and setter functions.

How to use inline. Having established inline doesn't inline, usually, your question about using it is mostly addressed as above. If you define a function within the class declaration, use the inline keyword to avoid possible linker errors. Otherwise, it's largely a meaningless keyword to most modern compilers so far as I am aware.

Where to put functions formed from splitting a single function. This is very much an opinion based question but there are two options I think that seem best:

First, you can make it a protected member of the class. If you do so, you should probably include a symbolic nod that this is not a general-purpose function - a leading underscore in the name is typically the symbol for "do not touch."

Alternatively, you can define the extra functions in the .cpp file, not within the class itself. For example [MyClass.cpp]:

void functionA()
{
    stuff;
}

void functionB()
{
    stuff;
}

void MyClass::myFunction()
{
    functionA();
    functionB();
}

This completely prevents these functions form being called outside this cpp file. It also prevents calls from child classes, which may or may not be desirable behavior. Use your discretion in choosing where to put them.

A final note. Be careful about how you divide up complicated functions, or you could end up with something worse than a single function. Moving things elsewhere might only serve to hide the fact the actual logic is messy. I personally find it much simpler to follow a single branching function than one that calls other functions. It is more difficult to read, especially for someone not familiar with the code, if calls are being made outside the function for potentially non-obvious reasons.

It might be beneficial to think how you could reorganize the code to be simpler, if possible, and keep it in one function - or divide it in such a way it would be reusable.