L'utilisation de blocs de portée internes dans un style de fonction est-elle mauvaise?

Dans certains cas (assez rares), il existe un risque de:

• réutiliser une variable qui n'est pas destinée à être réutilisée (voir exemple 1),

• ou en utilisant une variable au lieu d'une autre, proche sémantiquement (voir l'exemple 2).

Exemple 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`.
}

Exemple 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.

Ce risque peut être atténué en introduisant un périmètre:

Exemple 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.

Exemple 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`.
}

Cela vous semble-t-il faux? Souhaitez-vous éviter une telle syntaxe? Est-ce difficile à comprendre par les débutants?

Si votre fonction est si longue que vous ne pouvez plus reconnaître d'effets secondaires indésirables ou de réutilisation illégale de variables, il est temps de la diviser en fonctions plus petites - ce qui rend inutile une portée interne.

Pour confirmer cela par une expérience personnelle: il y a quelques années, j'ai hérité d'un projet hérité C ++ avec environ 150 000 lignes de code, et il contenait quelques méthodes utilisant exactement cette technique. Et devinez quoi - toutes ces méthodes étaient trop longues. Comme nous avons refactorisé la majeure partie de ce code, les méthodes sont devenues de plus en plus petites, et je suis sûr qu'il ne reste plus de méthodes de "portée interne"; ils ne sont tout simplement pas nécessaires.

Il est beaucoup plus facile pour un débutant (et tout programmeur) de penser à:

• ne pas utiliser une variable non pertinente

que de penser à:

• l'ajout de structures de code visant à éviter de ne pas utiliser une variable non pertinente.

De plus, du point de vue du lecteur, de tels blocs n'ont aucun rôle apparent: leur suppression n'a aucun impact sur l'exécution. Le lecteur peut se gratter la tête en essayant de deviner ce que le codeur voulait réaliser.

L'utilisation d'un oscilloscope est techniquement correcte. Mais si vous voulez rendre votre code plus lisible, vous devez extraire cette partie dans une autre méthode et lui donner un nom complet. De cette façon, vous pouvez donner une portée de vos variables et également donner un nom à votre tâche.

Je suis d'accord que la réutilisation de variable est le plus souvent une odeur de code. Ce code devrait probablement être refactorisé en blocs plus petits et autonomes.

Il y a un scénario particulier, OTOH, en C #, quand ils ont tendance à apparaître - c'est lors de l'utilisation de constructions de cas de commutation. La situation est très bien expliquée dans: Instruction C # Switch avec / sans accolades ... quelle est la différence?