13

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?

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
Arseni Mourzenko
  • 134,780
  • 31
  • 343
  • 513
  • 7
    Could you argue that each independent "scope block" should instead be its own method or function? – Dan Pichelman Jun 06 '13 at 19:58
  • 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). – JustinC Jun 07 '13 at 07:47
  • 1
    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 May 17 '22 at 07:49
  • 1
    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. – Jonny Dee May 17 '22 at 07:56

4 Answers4

34

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.

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • 6
    But why is it bad? Having many named functions is also confusing. – Kris Van Bael Jun 07 '13 at 05:22
  • 1
    +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. – Benjamin Gruenbaum Jun 07 '13 at 05:24
  • 10
    @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. – Doc Brown Jun 07 '13 at 05:44
  • 2
    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. – JustinC Jun 07 '13 at 07:44
  • 4
    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. – Kris Van Bael Jun 08 '13 at 13:29
  • @KrisVanBael: this is a topic I guess I cannot convince you here in a few lines of comments. For me, it based on 3 decades of programming experience. And I am still not going as far as Bob Martin in his "Clean Code" book. – Doc Brown Jun 08 '13 at 21:45
  • @DocBrown: How do you handle variables which should e.g. maintain running counts and totals throughout a sequence of operations? If a method is supposed to do a bunch of operations and then provide statistics afterward, passing statistics-related variables as `ref` parameters seems icky, but creating a new heap object for such purposes doesn't seem good either. Pascal's nested functions would be perfect here, except that they're only available in Pascal. – supercat May 06 '14 at 20:32
  • 1
    @supercat: this is case dependent, but having a common running count for a sequence of operations happens mostly when those (smaller) operations are part of a bigger operation, which could be grouped together to a class. If that's the case, the variable for the running count or total is a good candidate for a member variable of the class. Of course, if that is really the best solution may depend on things like the intended threading model and the business case itself. – Doc Brown May 06 '14 at 21:10
  • 1
    @DocBrown: For the use case I'm thinking of, the method in question cannot legitimately be run multiple times on an object, which would imply that the variables could be class fields, but that really feels wrong. Perhaps if they represented lifetime statistics, and there were a "statistic snapshot" class with a "subtract" method, that might be okay for some things, but some statistics may really only be meaningful in very limited contexts (e.g. what was the average time required for the remote system to respond to an `Oink` that immediately followed a `Quack`). – supercat May 06 '14 at 21:35
  • 1
    @DocBrown: Perhaps this is a broader question that just how to subdivide methods, but what's the best way to set things up so that one can gather statistics about oddly-specific details of a process performed by a method if nature of the required statistics won't become apparent until code is tested and performance in some parts is found lacking? – supercat May 06 '14 at 21:40
  • @supercat: honestly, these comments are the wrong place to discuss this, and this is. I suggest you provide an example and ask a new question about this here on PSE. – Doc Brown May 07 '14 at 07:44
  • I would have agreed if you asked 10 years ago. But these days, I'm really avoiding both OO design generally and "boilerplate code". Extensive use of LINQ, lambdas, ForEach() instead of foreach(), limiting "scope" as much as possible (hence needing this syntax) etc. You end up with some long methods, but I usually immediately turn on code folding. It's just a click to expand and take a peek at what's going on, then collapse and ignore it. Is that more or less the new modern style? I'm sort of making this up as I go and not sure the best style... – Brian Birtle May 12 '23 at 08:06
  • @BrianBirtle: I did not "ask", I answered the question, did you mean that? And yes, as you can see, my answer is almost 10 years old. I still think the gist of my answer is still true today as it was in 2013. Where I agree is that today there are probably more cases where small anonymous function blocks make sense, maybe as some functor in an expression which requires one. But I am still under the impression the OP had other cases in mind, where they were just trying to avoid figuring out a good name for something which would better be a named function. – Doc Brown May 12 '23 at 14:57
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.

mouviciel
  • 15,473
  • 1
  • 37
  • 64
  • 1
    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. – Jonny Dee May 17 '22 at 08:29
  • 1
    @JonnyDee: You are correct. Actually I changed my mind on this since 2013. – mouviciel May 17 '22 at 08:45
2

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.

Arnab
  • 377
  • 1
  • 2
  • 10
0

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?