Is the usage of internal scope blocks within a function bad style?

Question

There are some (quite rare) cases where there is a risk of:

• reusing a variable which is not intended to be reused (see example 1),

• or using a variable instead of another, semantically close (see example 2).

Example 1:

var data = this.InitializeData();
if (this.IsConsistent(data, this.state))
{
 this.ETL.Process(data); // Alters original data in a way it couldn't be used any longer.
}
// ...
foreach (var flow in data.Flows)
{
 // This shouldn't happen: given that ETL possibly altered the contents of `data`, it is
 // not longer reliable to use `data.Flows`.
}

Example 2:

var userSettingsFile = SettingsFiles.LoadForUser();
var appSettingsFile = SettingsFiles.LoadForApp();
if (someCondition)
{
 userSettingsFile.Destroy();
}
userSettingsFile.ParseAndApply(); // There is a mistake here: `userSettingsFile` was maybe
 // destroyed. It's `appSettingsFile` which should have
 // been used instead.

This risk can be mitigated by introducing a scope:

Example 1:

// There is no `foreach`, `if` or anything like this before `{`.
{
 var data = this.InitializeData();
 if (this.IsConsistent(data, this.state))
 {
 this.ETL.Process(data);
 }
}
// ...
// A few lines later, we can't use `data.Flows`, because it doesn't exist in this scope.

Example 2:

{
 var userSettingsFile = SettingsFiles.LoadForUser();
 if (someCondition)
 {
 userSettingsFile.Destroy();
 }
}
{
 var appSettingsFile = SettingsFiles.LoadForApp();
 // `userSettingsFile` is out of scope. There is no risk to use it instead of
 // `appSettingsFile`.
}

Does it look wrong? Would you avoid such syntax? Is it difficult to understand by beginners?

• Dan Pichelman: Could you argue that each independent "scope block" should instead be its own method or function?
• JustinC: I would say "often" not as well implemented as it could be (but probably not a "design" problem). Sometimes by design, it is unavoidable (for instance exception/resource scoping).
• Jonny Dee: I appreciate code that uses scopes in such cases. You can safely throw out (temp) variables from your mind when the scope ends because you know(!) they won't be used later. You can easily use the editor's code folding feature and collapse such a block. If it is preceded with a comment then you'll only see the comment telling you what the collapsed block does. Furthermore, you know exactly where to continue reading code if you are not interested in a certain step -- just skip to the end of the scope. Without one, you'd have to read through until you (hopefully) found the last statement to skip.
• Jonny Dee: And NO, it doesn't always make sense to move such code into another function/method. You would need to pass required context to such a method, you would need to check arguments and such a function suggested reusability when it is likely so specialized that reuse is very unlikely. Should it be reused later then someone might change its logic to there use case and/or add corresponding if-else statements or additional method arguments serving as flags for distinguishing between them.

Answer 1

If your function is so long that you cannot recognize any unwanted side effects or illegal reuse of variables any more, then it is time to split it up in smaller functions - which makes an internal scope pointless.

To back this up by some personal experience: some years ago I inherited a C++ legacy project with ~150K lines of code, and it contained a few methods using exactly this technique. And guess what - all of those methods were too long. As we refactored most of that code, the methods became smaller and smaller, and I am pretty sure there are no remaining "internal scope" methods any more; they are simply not needed.

• Kris Van Bael: But why is it bad? Having many named functions is also confusing.
• Benjamin Gruenbaum: +1 Exactly, if you need a scope, likely you're performing a particular action that can be extracted to a function. Kris, it's not about creating lots and lots of functions, it's that when these cases occur (where one block's scope might conflict with another's) that usually a function should have been used. I see this in other languages (namely JavaScript, with IIFE's instead of plain' old functions) all the time too.
• Doc Brown: @KrisVanBael: "Having many named functions is also confusing" - if you use descriptive names, quite the opposite is true. Creating too large functions is the topmost reason how code becomes hard to maintain, and even harder to extent.
• JustinC: If there are so many functions that naming them distinctly becomes a problem, possibly some of them can be rolled up into a new type as they may have significant independence from their original function body. Some might be so closely related that they could be eliminated. Suddenly, there aren't quite so many functions.
• Kris Van Bael: Still not convinced. Often long functions are bad indeed. But saying that every need for scope requires a separate function is too black&white for me.

Answer 2

It is far easier for a beginner (and any programmer) to think of:

• not using a variable that is not relevant

than to think of:

• adding code structures aiming at preventing himself from not using a variable that is not relevant.

Moreover, from a reader's point of view, such blocks have no apparent role: removing them has no impact on the execution. The reader may scratch his head trying to guess what the coder wanted to achieve.

• Jonny Dee: Variables carry state which you need to keep track of while reading/understanding code. Less variables or variables with short-lived visibility result in less state. Less state means less possibilities for unwanted side effects. Adding more helpful structure actually improves readability, IMHO.
• mouviciel: @JonnyDee: You are correct. Actually I changed my mind on this since 2013.

Answer 3

Using a scope is technically right. But if you want to make your code more readable then you should extract that part in another method and give that a meaning full name. In this way you can give a scope of your variables and also give a name of your task.

Answer 4

I agree that reusing variable is more often than not a code smell. Probably such code should be refactored in smaller, self-contained, blocks.

There is one particular scenario, OTOH, in C#, when they tend to popup - that is when using switch-case constructs. Situation is very nicely explained in: C# Switch statement with/without curly brackets... what's the difference?