In general, you should try to reduce the scope of your variables. Uncle Bob's Clean Code rule for this is "declare variables close to their usage" and CppCheck typically gives a diagnostic such as "the scope of X could be reduced". Specifically, the Old C practice of declaring all variables at the top usually leads to violations of this rule, especially for large functions.
So as a style rule, I would prefer to avoid "declaration sections" at the top of the function where everything is declared. The only good reason to use that style nowadays is if you need to compile the code on certain legacy compilers that support only C89. Since C99, even plain vanilla C allows declaring the variable anywhere in the function, just like C++ and Java.
In your example, you declare and initialize a, b, c and d at the top of the function, so the scope of those variables is basically the entire function. However, your actual use of each of those is only in a very small portion of your function. If you had limited the scope of those, then your *BUG* line would become a compiler error. Scope can be limited in a large function by adding extra braces:
float foo()
{
float output = get_default();
{
float a = calculate_a();
if (a > some_threshold) {
output += a;
module_variable = calculate_c();
return output;
}
}
{
float b = calculate_b();
if (b < other_threshold) {
output += a; // ERROR.
module_variable = calculate_d();
return output;
}
}
// ... Imagine lots of other code that initializes and uses c, d, e, f, etc.
assert(0); // Should not reach this if I'm using early returns consistently.
}
If you're using C++17 or later, you could use an initializer inside the if, which allows to simplify things a bit:
// Since C++17.
float foo()
{
float output = get_default();
if (float a = calculate_a(); a > some_threshold) {
output += a;
module_variable = calculate_c();
return output;
}
if (float b = calculate_b(); b < other_threshold) {
output += a; // ERROR.
module_variable = calculate_d();
return output;
}
// ... Imagine lots of other code that initializes and uses c, d, e, f, etc.
assert(0);
}
This C++17 feature may look a bit strange at first, but notice it uses the same rule as an initializer inside a C++ and C99 for statement, which also tends to reduce scope of variables and should also be used where possible for the same reason.
c & d are both saved as debug outputs regardless of the conditional statement, which is why they are always calculated
If you always need to calculate everything, and want to avoid recalculating if necessary, and if you are using C++11, then a fancier alternative to the naked braces is to replace them with capturing lambdas that are immediately invoked. Arguably the [...](){...}(); notation is a bit noisy, but it allows some nice compiler diagnostics for a large function:
// Since C++11.
float foo()
{
float output = get_default();
float a = calculate_a();
float b = calculate_b();
float c = calculate_c();
float d = calculate_d();
// ... More calculate statements.
[&a, &c, &output]() {
if (a > some_threshold) {
output += a;
module_variable = c;
}
}();
[&b, &d, &output]() {
if (b < other_threshold) {
output += a; // ERROR.
module_variable = d;
}
}();
// ...
return output;
}
The specific diagnostic from GCC is:
main.cpp:32:27: error: 'a' is not captured
32 | output += a; // ERROR.
|
Which means that you either need to update the capture list for that section, or that you didn't mean a, but something else instead.
However, there is a caveat with GCC (at least the version I am using), when using immediately invoked lambda expressions. It's conceivable that you might accidentally forget the () at the end of the lambda expression, e.g. by typing just }; at the end instead of }();:
[&a, &c, &output]() {
if (a > some_threshold) {
output += a;
module_variable = c;
}
}; // OOPS.
That's syntatically valid, but unfortunately it just declares the code to be executed without actually executing it (the code inside the lambda is dead code). Unfornately GCC did not give a warning for that. Anyway, I see that Clang correctly gives a warning about this (expression result unused, -Wunued-value). It's possible that newer versions of GCC eventually do so as well, if the newest version does not already (I don't update my tools so often, so maybe the newest versions of GCC are better).
Finally, the lambda expressions make early returns impossible, so the sense of the short-circuiting behaviour in the "else if ... else if ..." of the original code is lost. If you need that behaviour, you could consider this:
...
bool done = false;
// ...
[&a, &c, &output, &done]() {
if (...) {
...
done = true;
}
}();
if (done) return output;
// ...
