0

I am starting to be more and more attracted to writing long C++ algorithmic functions using successive scoped blocks, as follows:

void my_algorithm(const MyStruct1 &iparam1, MyStruct2 &oparam2)
{
    // First block
    MyStruct3 intermediate_var3;
    {
        double temporary_var;
        // Functional step 1.1
        // Functional step 1.2
        intermediate_var3 = ...
    }
    // Second block
    MyStruct4 intermediate_var4;
    {
        double temporary_var;
        // Functional step 2.1
        // Functional step 2.2
        intermediate_var4 = ...
    }
    // Final block
    {
        int temporary_var;
        oparam2 = ...
    }
}

I am starting to think it is a good way to clarify the function structure and to limit the scope of temporary variables (such as counters i, j, k etc). I saw that such scope blocks make sense in C functions to enable new declarations (see Why enclose blocks of C code in curly braces?).

In the context of C++, is this good or bad practice ?

Community
  • 1
  • 1
BConic
  • 8,750
  • 2
  • 29
  • 55
  • 3
    That is a smell that you should really extract these blocks as functions. If you think there may be a performance issue you can always inline the functions. Smart compilers usually do this by themselves anyway – Panagiotis Kanavos Jan 22 '14 at 08:44
  • Generally bad, as it's usually a sign that a function is doing too many disparate things. Use separate functions instead. – molbdnilo Jan 22 '14 at 08:45
  • You may be right... Still, as these blocks are part of a single _algorithm_, not just any C++ function, does it really make sense to separate them in different functions ? – BConic Jan 22 '14 at 08:53

1 Answers1

1

This is a clear sign, that you should extract this separate blocks into separate functions.

MyStruct3 DoSth3(params)
{
    double temporary_var;
    // Functional step 1.1
    // Functional step 1.2
    return ...
}

MyStruct4 DoSth4(params)
{
    double temporary_var;
    // Functional step 2.1
    // Functional step 2.2
    intermediate_var4 = ...
}

void my_algorithm(const MyStruct1 &iparam1, MyStruct2 &oparam2)
{
    // First block
    MyStruct3 intermediate_var3 = DoSth3(params);

    // Second block
    MyStruct4 intermediate_var4 = DoSth4(params);

    int temporary_var;
    oparam2 = ...
}

It may happen, that you'll be worried about DoSth3 and DoSth4 being public, as they should be private in the context of my_algorithm. In such case you can solve it in the following way:

class my_algorithm
{
private:
    static MyStruct3 DoSth3(params);
    static MyStruct4 DoSth4(params);

public:
    static void Perform(const MyStruct1 &iparam1, MyStruct2 &oparam2);
};
Spook
  • 25,318
  • 18
  • 90
  • 167
  • OK, I guess using a class and private internal functions is the most proper way to do it. Thanks! – BConic Jan 22 '14 at 08:57
  • 1
    +1 ; instead of the class approach, you can just as well keep the 'private' functions local to the cpp file where the algorithm is in though. I like this more as it achieves the same goal and you're not using a class as a glorified namespace.. – stijn Jan 22 '14 at 08:57
  • Yes, local functions are a nice idea too. I think that even is the most natural modification to the original way I was writing my function. – BConic Jan 22 '14 at 09:04