Variables temporaires vs exigences de longueur de ligne

J'ai lu le refactoring de Martin Fowler . Il est généralement excellent, mais l'une des recommandations de Fowler semble causer un petit problème.

Fowler vous recommande de remplacer les variables temporaires par une requête, donc au lieu de cela:

double getPrice() {
    final int basePrice = _quantity * _itemPrice;
    final double discountFactor;
    if (basePrice > 1000) discountFactor = 0.95;
    else discountFactor = 0.98;
    return basePrice * discountFactor;
}

vous sortez dans une méthode d'aide:

double basePrice() {
    return _quantity * _itemPrice;
}

double getPrice() {
    final double discountFactor;
    if (basePrice() > 1000) discountFactor = 0.95;
    else discountFactor = 0.98;
    return basePrice() * discountFactor;
}

En général, je suis d'accord, sauf qu'une des raisons pour lesquelles j'utilise des variables temporaires est lorsqu'une ligne est trop longue. Par exemple:

$host = 'https://api.twilio.com';
$uri = "$host/2010-04-01/Accounts/$accountSid/Usage/Records/AllTime";
$response = Api::makeRequest($uri);

Si j'essayais d'inclure cela, la ligne dépasserait 80 caractères.

Alternativement, je me retrouve avec des chaînes de code, elles-mêmes pas beaucoup plus faciles à lire:

$params = MustacheOptions::build(self::flattenParams($bagcheck->getParams()));

Quelles sont les stratégies pour concilier les deux?

Comment
1. Des restrictions de longueur de ligne existent pour que vous puissiez voir + comprendre plus de code. Ils sont toujours valables.
2. Mettez l'accent sur le jugement sur la convention aveugle .
3. Évitez les variables temporaires à moins d'optimiser les performances .
4. Évitez d'utiliser une indentation profonde pour l'alignement dans les instructions multilignes.
5. Divisez les longues déclarations en plusieurs lignes le long des limites de l'idée :

// prefer this
var distance = Math.Sqrt(
    Math.Pow(point2.GetX() - point1.GetX(), 2) + // x's
    Math.Pow(point2.GetY() - point1.GetY(), 2)   // y's
);

// over this
var distance = Math.Sqrt(Math.Pow(point2.GetX() -
    point1.GetX(), 2) + Math.Pow(point2.GetY() -
    point1.GetY(), 2)); // not even sure if I typed that correctly.

Raisonnement
La principale source de mes problèmes (de débogage) avec les variables temporaires est qu'elles ont tendance à être mutables. À savoir, je suppose qu'elles sont une valeur lorsque j'écris le code, mais si la fonction est complexe, un autre morceau de code change d'état à mi-parcours. (Ou l'inverse, où l'état de la variable reste le même mais le résultat de la requête a changé).

Pensez à vous en tenir aux requêtes, sauf si vous optimisez les performances . Cela conserve toute logique que vous avez utilisée pour calculer cette valeur en un seul endroit.

Les exemples que vous avez donnés (Java et ... PHP?) Permettent tous deux des instructions sur plusieurs lignes. Si les lignes deviennent longues, brisez-les. La source jquery pousse cela à l'extrême. (La première instruction va jusqu'à la ligne 69!) Non pas que je sois nécessairement d'accord, mais il existe d'autres façons de rendre votre code lisible que d'utiliser des variables temporaires.

Quelques exemples
1. Guide de style PEP 8 pour python (pas le plus bel exemple)
2. Paul M Jones sur le guide de style Pear (milieu de l'argument route)
3. Longueur de ligne Oracle + conventions d'habillage (stratagèmes utiles pour conserver jusqu'à 80 caractères)
4. Pratiques MDN Java (met l'accent sur le jugement du programmeur plutôt que sur la convention)

Je pense que le meilleur argument pour utiliser des méthodes d'assistance au lieu de variables temporaires est la lisibilité humaine. Si vous, en tant qu'humain, avez plus de mal à lire la chaîne de méthode d'assistance que la varialbe temporaire, je ne vois aucune raison pour que vous les extrayiez.

(Corrigez-moi si j'ai tort, s'il-vous plait)

Je ne pense pas que vous ayez besoin de suivre strictement les directives de 80 caractères ou que la variable temporaire locale soit extraite. Mais les longues lignes et les temps locaux devraient être étudiés pour de meilleures façons d'exprimer la même idée. Fondamentalement, ils indiquent qu'une fonction ou une ligne donnée est trop compliquée et que nous devons la décomposer. Mais nous devons être prudents, car diviser une tâche en mauvais morceaux ne fait que compliquer la situation. Je dois donc décomposer les choses en composants simples et réutilisables.

Permettez-moi de regarder les exemples que vous avez publiés.

$host = 'https://api.twilio.com';
$uri = "$host/2010-04-01/Accounts/$accountSid/Usage/Records/AllTime";
$response = Api::makeRequest($uri);

Mon observation est que tous les appels d'api twilio commenceront par "https://api.twilio.com/2010-04-1/", et donc il y a une fonction réutilisable très évidente à avoir:

$uri = twilioURL("Accounts/$accountSid/Usage/Records/AllTime")

En fait, je pense que la seule raison de générer une URL est de faire la demande, alors je ferais:

$response = TwilioApi::makeRequest("Accounts/$accountSid/Usage/Records/AllTime")

En fait, la plupart des URL commencent en fait par "Accounts / $ accountSid", donc je vais probablement extraire cela aussi:

$response = TwilioApi::makeAccountRequest($accountSid, "Usage/Records/AllTime")

Et si nous faisons de l'API Twilio un objet qui contient le numéro de compte, nous pourrions faire quelque chose comme:

$response = $twilio->makeAccountRequest("Usage/Records/AllTime")

L'utilisation d'un objet $ twilio a l'avantage de faciliter les tests unitaires. Je peux donner à l'objet un objet $ twilio différent qui ne rappelle pas réellement à twilio, ce qui sera plus rapide et ne fera pas de choses étranges à twilio.

Regardons l'autre

$params = MustacheOptions::build(self::flattenParams($bagcheck->getParams()));

Ici, je pense à l'un ou l'autre:

$params = MustacheOptions::buildFromParams($bagcheck->getParams());

ou

$params = MustacheOptions::build($bagcheck->getFlatParams());

ou

$params = MustacheOptions::build(flatParams($backCheck));

En fonction de l'idiome le plus réutilisable.

En fait, je ne suis pas d'accord avec l'éminent M. Fowler à ce sujet dans le cas général.

L'avantage d'extraire une méthode à partir de code anciennement en ligne est la réutilisation du code; le code de la méthode est désormais séparé de son utilisation initiale et peut désormais être utilisé à d'autres endroits du code sans être copié et collé (ce qui nécessiterait des modifications à plusieurs endroits si la logique générale du code copié devait changer) .

Cependant, une valeur conceptuelle égale et souvent supérieure est la «réutilisation de la valeur». M. Fowler appelle ces méthodes extraites pour remplacer les variables temporaires "requêtes". Eh bien, quoi de plus efficace; interroger une base de données à chaque fois que vous avez besoin d'une valeur particulière, ou interroger une fois et stocker le résultat (en supposant que la valeur est suffisamment statique pour que vous ne vous attendiez pas à ce qu'elle change)?

Pour presque tous les calculs au-delà du calcul relativement trivial de votre exemple, dans la plupart des langues, il est moins coûteux de stocker le résultat d'un calcul que de continuer à le calculer. Par conséquent, la recommandation générale de recalculer à la demande est fallacieuse; cela coûte plus de temps aux développeurs et plus de temps CPU, et économise une quantité insignifiante de mémoire, qui dans la plupart des systèmes modernes est la ressource la moins chère de ces trois.

Maintenant, la méthode d'assistance, en conjonction avec un autre code, pourrait être rendue "paresseuse". Il initialiserait une variable lors de sa première exécution. Tous les autres appels renverraient cette variable jusqu'à ce que la méthode soit explicitement invitée à recalculer. Il peut s'agir d'un paramètre de la méthode ou d'un indicateur défini par un autre code qui modifie toute valeur dont dépend le calcul de cette méthode:

double? _basePrice; //not sure if Java has C#'s "nullable" concept
double basePrice(bool forceCalc)
{
   if(forceCalc || !_basePrice.HasValue)
      return _basePrice = _quantity * _itemPrice;
   return _basePrice.Value;
}

Maintenant, pour ce calcul trivial, c'est encore plus de travail effectué que enregistré, et donc je recommande généralement de s'en tenir à la variable temp; cependant, pour les calculs plus complexes que vous voudriez généralement éviter d'exécuter plusieurs fois et dont vous avez besoin à plusieurs endroits dans le code, c'est la façon dont vous le feriez.

Les méthodes d'assistance ont leur place, mais vous devez faire attention à assurer la cohérence des données et l'augmentation inutile de la portée des variables.

Par exemple, votre propre exemple cite:

double getPrice() {
    final double discountFactor;
    if (basePrice() > 1000) discountFactor = 0.95;      <--- first call
    else discountFactor = 0.98;
    return basePrice() * discountFactor;                <--- second call
}

Il est clair que les deux _quantityet _itemPricesont des variables globales (ou au moins le niveau de la classe) et donc il y a un potentiel pour eux d'être modifié en dehors degetPrice()

Par conséquent, il est possible que le premier appel basePrice()renvoie une valeur différente du deuxième appel!

Par conséquent, je suggérerais que les fonctions d'aide peuvent être utiles pour isoler des mathématiques complexes, mais en remplacement des variables locales, vous devez être prudent.

Vous devez également éviter la réduction ad absurde - le calcul doit-il discountFactorêtre ramené à une méthode? Votre exemple devient donc:

double getPrice()
{
    final double basePrice      = calculateBasePrice();
    final double discountFactor = calculateDiscount( basePrice );

    return basePrice * discountFactor;
}

Le partitionnement au-delà d'un certain niveau rend le code moins lisible.

S'il vous arrive de travailler dans un langage avec des paramètres nommés (ObjectiveC, Python, Ruby, etc.), les variables temporaires sont moins utiles.

Toutefois, dans votre exemple basePrice, l'exécution de la requête peut prendre un certain temps et vous souhaiterez peut-être stocker le résultat dans une variable temporaire pour une utilisation future.

Comme vous, cependant, j'utilise des variables temporaires pour des raisons de clarté et de longueur de ligne.

J'ai également vu des programmeurs faire ce qui suit en PHP. C'est intéressant et idéal pour le débogage, mais c'est un peu étrange.

$rs = DB::query( $query = "SELECT * FROM table" );
if (DEBUG) echo $query;
// do something with $rs

La justification de cette recommandation est que vous voulez pouvoir utiliser le même précalcul ailleurs dans votre application. Voir Remplacer Temp par une requête dans le catalogue de modèles de refactoring:

La nouvelle méthode peut ensuite être utilisée dans d'autres méthodes

    double basePrice = _quantity * _itemPrice;
    if (basePrice > 1000)
        return basePrice * 0.95;
    else
        return basePrice * 0.98;

           

    if (basePrice() > 1000)
        return basePrice() * 0.95;
    else
        return basePrice() * 0.98;
...
double basePrice() {
    return _quantity * _itemPrice;
}

Ainsi, dans votre exemple d'hôte et d'URI, je n'appliquerais cette recommandation que si je prévois de réutiliser la même URI ou définition d'hôte.

Si c'est le cas, en raison de l'espace de noms, je ne définirai pas une méthode globale uri () ou host (), mais un nom avec plus d'informations, comme twilio_host () ou archive_request_uri ().

Ensuite, pour le problème de longueur de ligne, je vois plusieurs options:

• Créez une variable locale, comme uri = archive_request_uri().

Justification: Dans la méthode actuelle, vous voulez que l'URI soit celui mentionné. La définition de l'URI est toujours factorisée.

• Définissez une méthode locale, comme uri() { return archive_request_uri() }

Si vous utilisez souvent la recommandation de Fowler, vous saurez que la méthode uri () est le même modèle.

Si, en raison du choix de la langue, vous devez accéder à la méthode locale avec un `` self '', je recommanderais la première solution, pour une expressivité accrue (en Python, je définirais la fonction uri à l'intérieur de la méthode actuelle).