Est-ce une odeur de code de mettre un drapeau dans une boucle pour l'utiliser plus tard?

30

J'ai un morceau de code où j'itère une carte jusqu'à ce qu'une certaine condition soit remplie et ensuite j'utilise cette condition pour faire plus de choses.

Exemple:

Map<BigInteger, List<String>> map = handler.getMap();

if(map != null && !map.isEmpty())
{
    for (Map.Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        fillUpList();

        if(list.size() > limit)
        {
            limitFlag = true;
            break;
        }
    }
}
else
{
    logger.info("\n>>>>> \n\t 6.1 NO entries to iterate over (for given FC and target) \n");
}

if(!limitFlag) // Continue only if limitFlag is not set
{
    // Do something
}

Je sens que mettre un drapeau et ensuite l'utiliser pour faire plus de choses est une odeur de code.

Ai-je raison? Comment pourrais-je supprimer cela?

Siddharth Trikha
la source
10
Pourquoi pensez-vous que c'est une odeur de code? quel genre de problèmes spécifiques pouvez-vous prévoir en faisant cela qui ne se produirait pas sous une structure différente?
Ben Cottrell
13
@ gnasher729 Juste par curiosité, quel terme utiliseriez-vous à la place?
Ben Cottrell
11
-1, votre exemple n'a aucun sens. entryn'est utilisé nulle part à l'intérieur de la boucle de fonction, et nous ne pouvons que deviner ce qui listest. Est fillUpListcensé remplir list? Pourquoi ne l'obtient-il pas comme paramètre?
Doc Brown
13
Je reconsidérerais votre utilisation des espaces et des lignes vides.
Daniel Jour
11
Le code ne sent rien. «Odeur de code» est un terme inventé par les développeurs de logiciels qui veulent se taire quand ils voient du code qui ne répond pas à leurs normes élitistes.
Robert Harvey

Réponses:

70

Il n'y a rien de mal à utiliser une valeur booléenne pour son objectif: enregistrer une distinction binaire.

Si on me disait de refactoriser ce code, je mettrais probablement la boucle dans une méthode qui lui est propre afin que l'affectation + breakse transforme en a return; alors vous n'avez même pas besoin d'une variable, vous pouvez simplement dire

if(fill_list_from_map()) {
  ...
Kilian Foth
la source
6
En fait, l'odeur dans son code est la fonction longue qui doit être divisée en fonctions plus petites. Votre suggestion est la voie à suivre.
Bernhard Hiller
2
Une meilleure expression qui décrit la fonction utile de la première partie de ce code consiste à déterminer si la limite sera dépassée après avoir accumulé quelque chose à partir de ces éléments mappés. Nous pouvons également supposer en toute sécurité qu'il fillUpList()y a du code (que OP décide de ne pas partager) qui utilise réellement la valeur entryde l'itération; sans cette hypothèse, il semblerait que le corps de la boucle n'ait rien utilisé de l'itération de la boucle.
rwong
4
@Kilian: J'ai juste une préoccupation. Cette méthode remplira une liste et renverra un booléen qui représente que la taille de la liste dépasse ou non une limite, de sorte que le nom 'fill_list_from_map' ne précise pas que représente le booléen renvoyé (il n'a pas réussi à se remplir, un dépasse, etc.). Comme le booléen renvoyé est pour un cas spécial qui n'est pas évident d'après le nom de la fonction. Des commentaires? PS: nous pouvons également prendre en compte la séparation des requêtes de commande.
Siddharth Trikha
2
@SiddharthTrikha Vous avez raison, et j'avais exactement la même préoccupation lorsque j'ai suggéré cette ligne. Mais on ne sait pas quelle liste le code est censé remplir. Si c'est toujours la même liste, vous n'avez pas besoin du drapeau, vous pouvez simplement vérifier sa longueur par la suite. Si vous avez besoin de savoir si un remplissage individuel a dépassé la limite, vous devez transporter ces informations à l'extérieur d'une manière ou d'une autre, et IMO, le principe de séparation commande / requête n'est pas une raison suffisante pour rejeter la manière évidente: via le retour valeur.
Kilian Foth
6
Oncle Bob dit à la page 45 de Clean Code : "Les fonctions doivent soit faire quelque chose ou répondre à quelque chose, mais pas les deux. Soit votre fonction doit changer l'état d'un objet, soit elle doit renvoyer des informations sur cet objet. Les deux conduisent souvent à confusion."
CJ Dennis
25

Ce n'est pas nécessairement mauvais, et parfois c'est la meilleure solution. Mais définir des indicateurs comme celui-ci dans des blocs imbriqués peut rendre le code difficile à suivre.

Le problème est que vous avez des blocs pour délimiter les étendues, mais que vous avez ensuite des indicateurs qui communiquent entre les étendues, rompant l'isolement logique des blocs. Par exemple, le limitFlagsera faux si le mapest null, donc le code "faire quelque chose" sera exécuté si le mapest null. C'est peut-être ce que vous envisagez, mais ce pourrait être un bogue qui est facile à manquer, car les conditions de cet indicateur sont définies ailleurs, à l'intérieur d'une portée imbriquée. Si vous pouvez conserver les informations et la logique dans l'étendue la plus étroite possible, vous devriez essayer de le faire.

JacquesB
la source
2
C'est la raison pour laquelle j'ai senti que c'était une odeur de code, car les blocs ne sont pas complètement isolés et peuvent être difficiles à suivre plus tard. Donc je suppose que le code dans la réponse de @Kilian est le plus proche possible?
Siddharth Trikha
1
@SiddharthTrikha: C'est difficile à dire car je ne sais pas ce que le code est censé faire. Si vous voulez seulement vérifier si la carte contient au moins un élément dont la liste est supérieure à la limite, je pense que vous pouvez le faire avec une seule expression anyMatch.
JacquesB
2
@SiddharthTrikha: le problème de portée peut être facilement résolu en changeant le test initial en une clause de garde comme celle- if(map==null || map.isEmpty()) { logger.info(); return;}ci, cependant, ne fonctionnera que si le code que nous voyons est le corps complet d'une fonction, et la // Do somethingpartie n'est pas requise dans le cas où la carte est nul ou vide.
Doc Brown
14

Je déconseille de raisonner sur les «odeurs de code». C'est juste la façon la plus paresseuse possible de rationaliser vos propres préjugés. Au fil du temps, vous développerez de nombreux préjugés, et beaucoup d'entre eux seront raisonnables, mais beaucoup d'entre eux seront stupides.

Au lieu de cela, vous devriez avoir des raisons pratiques (c'est-à-dire non dogmatiques) de préférer une chose à une autre, et éviter de penser que vous devriez avoir la même réponse pour toutes les questions similaires.

Les "odeurs de code" sont pour quand vous ne pensez pas . Si vous pensez vraiment au code, faites-le bien!

Dans ce cas, la décision pourrait vraiment aller dans les deux sens en fonction du code environnant. Cela dépend vraiment de ce que vous pensez être la façon la plus claire de penser à ce que fait le code. (Le code "propre" est un code qui communique clairement ce qu'il fait aux autres développeurs et leur permet de vérifier facilement qu'il est correct)

Souvent, les gens écriront des méthodes structurées en phases, où le code déterminera d'abord ce qu'il doit savoir sur les données, puis agira en conséquence. Si la partie "déterminer" et la partie "agir sur elle" sont toutes les deux un peu compliquées, alors il peut être judicieux de le faire, et souvent "ce qu'il doit savoir" peut être transporté entre les phases dans les drapeaux booléens. Je préférerais vraiment que vous donniez un meilleur nom au drapeau. Quelque chose comme "largeEntryExists" rendrait le code beaucoup plus propre.

Si, en revanche, le code "// Do Something" est très simple, il peut être plus judicieux de le placer dans le ifbloc au lieu de définir un indicateur. Cela rapproche l'effet de la cause, et le lecteur n'a pas à analyser le reste du code pour s'assurer que l'indicateur conserve la valeur que vous définiriez.

Matt Timmermans
la source
5

Oui, c'est une odeur de code (cue downvotes de tous ceux qui le font).

L'essentiel pour moi est l'utilisation de la breakdéclaration. Si vous ne l'utilisez pas, vous parcourrez plus d'éléments que nécessaire, mais son utilisation donne deux points de sortie possibles de la boucle.

Ce n'est pas un problème majeur avec votre exemple, mais vous pouvez imaginer qu'à mesure que le conditionnel ou les conditions à l'intérieur de la boucle deviennent plus complexes ou que l'ordre de la liste initiale devient important, il est plus facile pour un bogue de se glisser dans le code.

Lorsque le code est aussi simple que votre exemple, il peut être réduit à une whileboucle ou une carte équivalente, une construction de filtre.

Lorsque le code est suffisamment complexe pour nécessiter des indicateurs et des ruptures, il sera sujet à des bugs.

Donc, comme pour toutes les odeurs de code: si vous voyez un drapeau, essayez de le remplacer par un while. Si vous ne le pouvez pas, ajoutez des tests unitaires supplémentaires.

Ewan
la source
+1 de moi. C'est une odeur de code à coup sûr et vous expliquez pourquoi et comment le gérer, bien.
David Arno
@Ewan: SO as with all code smells: If you see a flag, try to replace it with a whilePouvez-vous développer cela avec un exemple?
Siddharth Trikha
2
Avoir plusieurs points de sortie de la boucle peut compliquer la réflexion, mais dans ce cas, il faudrait le refactoriser pour que la condition de la boucle dépende de l'indicateur - cela signifierait le remplacer for (Map.Entry<BigInteger, List<String>> entry : map.entrySet())par for (Iterator<Map.Entry<BigInteger, List<String>>> iterator = map.entrySet().iterator(); iterator.hasNext() && !limitFlag; Map.Entry<BigInteger, List<String>> entry = iterator.next()). C'est un schéma assez rare pour que j'aie plus de mal à le comprendre qu'une pause relativement simple.
James_pic
@James_pic ma java est un peu rouillée, mais si nous utilisons des cartes, j'utiliserais un collecteur pour résumer le nombre d'éléments et les filtrer après la limite. Cependant, comme je le dis, l'exemple n'est "pas si mal" qu'une odeur de code est une règle générale qui vous avertit d'un problème potentiel. Pas une loi sacrée à laquelle vous devez toujours obéir
Ewan
1
Vous ne voulez pas dire "cue" plutôt que "queue"?
psmears du
0

Utilisez simplement un nom autre que limitFlag qui indique ce que vous vérifiez réellement. Et pourquoi enregistrez-vous quelque chose lorsque la carte est absente ou vide? limtFlag sera faux, tout ce dont vous vous souciez. La boucle est très bien si la carte est vide, donc pas besoin de vérifier cela.

gnasher729
la source
0

Définir une valeur booléenne pour transmettre des informations que vous aviez déjà est une mauvaise pratique à mon avis. S'il n'y a pas d'alternative facile, cela indique probablement un problème plus important comme une mauvaise encapsulation.

Vous devez déplacer la logique de la boucle for dans la méthode fillUpList pour la faire rompre si la limite est atteinte. Vérifiez ensuite la taille de la liste directement après.

Si cela casse votre code, pourquoi?

user294250
la source
0

Commençons par le cas général: l'utilisation d'un indicateur pour vérifier si un élément d'une collection remplit une certaine condition n'est pas rare. Mais le schéma que j'ai vu le plus souvent pour résoudre ce problème consiste à déplacer le chèque dans une méthode supplémentaire et à en revenir directement (comme Kilian Foth l'a décrit dans sa réponse ):

private <T> boolean checkCollection(Collection<T> collection)
{
    for (T element : collection)
        if (checkElement(element))
            return true;
    return false;
}

Depuis Java 8, il existe une manière plus concise d'utiliser Stream.anyMatch(…):

collection.stream().anyMatch(this::checkElement);

Dans votre cas, cela ressemblerait probablement à ceci (en supposant list == entry.getValue()dans votre question):

map.values().stream().anyMatch(list -> list.size() > limit);

Le problème dans votre exemple spécifique est l'appel supplémentaire à fillUpList(). La réponse dépend beaucoup de ce que cette méthode est censée faire.

Note latérale: En l'état, l'appel à fillUpList()n'a pas beaucoup de sens, car il ne dépend pas de l'élément que vous êtes en train d'itérer. Je suppose que c'est une conséquence de la suppression de votre code réel pour l'adapter au format de la question. Mais cela donne exactement un exemple artificiel difficile à interpréter et donc difficile à raisonner. Par conséquent , il est si important de fournir un minimal, complet et Vérifiable exemple .

Je suppose donc que le code réel transmet le courant entryà la méthode.

Mais il y a plus de questions à poser:

  • Les listes de la carte sont-elles vides avant d'atteindre ce code? Si oui, pourquoi existe-t-il déjà une carte et pas seulement la liste ou le jeu de BigIntegerclés? S'ils ne sont pas vides, pourquoi avez-vous besoin de remplir les listes? Lorsqu'il y a déjà des éléments dans la liste, n'est-ce pas une mise à jour ou un autre calcul dans ce cas?
  • Qu'est-ce qui fait qu'une liste dépasse la limite? S'agit-il d'une condition d'erreur ou devrait-il se produire souvent? Est-ce causé par une entrée invalide?
  • Avez-vous besoin des listes calculées jusqu'au point où vous atteignez une liste plus grande que la limite?
  • Que fait la partie " Faire quelque chose "?
  • Redémarrez-vous le remplissage après cette partie?

Ce ne sont que quelques questions qui me sont venues à l'esprit lorsque j'ai essayé de comprendre le fragment de code. Donc, à mon avis, c'est la véritable odeur de code : votre code ne communique pas clairement l'intention.

Cela peut signifier cela ("tout ou rien" et atteindre la limite indique une erreur):

/**
 * Computes the list of all foo strings for each passed number.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 * @return all foo strings for each passed number. Never {@code null}.
 * @throws InvalidArgumentException if any number produces a list that is too long.
 */
public Map<BigInteger, List<String>> computeFoos(Set<BigInteger> numbers)
        throws InvalidArgumentException
{
    if (numbers.isEmpty())
    {
        // Do you actually need to log this here?
        // The caller might know better what to do in this case...
        logger.info("Nothing to compute");
    }
    return numbers.stream().collect(Collectors.toMap(
            number -> number,
            number -> computeListForNumber(number)));
}

private List<String> computeListForNumber(BigInteger number)
        throws InvalidArgumentException
{
    // compute the list and throw an exception if the limit is exceeded.
}

Ou cela peut signifier cela ("mise à jour jusqu'au premier problème"):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @throws InvalidArgumentException if any new foo list would become too long.
 *             Some other lists may have already been updated.
 */
public void updateFoos(Map<BigInteger, List<String>> map)
        throws InvalidArgumentException
{
    map.replaceAll(this::computeUpdatedList);
}

private List<String> computeUpdatedList(
        BigInteger number, List<String> currentValues)
        throws InvalidArgumentException
{
    // compute the new list and throw an exception if the limit is exceeded.
}

Ou ceci ("mettre à jour toutes les listes mais conserver la liste d'origine si elle devient trop grande"):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * Lists that would become too large will not be updated.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @return {@code true} if all updates have been successful,
 *         {@code false} if one or more elements have been skipped
 *         because the foo list size limit has been reached.
 */
public boolean updateFoos(Map<BigInteger, List<String>> map)
{
    boolean allUpdatesSuccessful = true;
    for (Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        List<String> newList = computeListForNumber(entry.getKey());
        if (newList.size() > limit)
            allUpdatesSuccessful = false;
        else
            entry.setValue(newList);
    }
    return allUpdatesSuccessful;
}

private List<String> computeListForNumber(BigInteger number)
{
    // compute the new list
}

Ou même les suivants (en utilisant computeFoos(…)le premier exemple mais sans exception):

/**
 * Processes the passed numbers. An optimized algorithm will be used if any number
 * produces a foo list of a size that justifies the additional overhead.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 */
public void process(Collection<BigInteger> numbers)
{
    Map<BigInteger, List<String>> map = computeFoos(numbers);
    if (isLimitReached(map))
        processLarge(map);
    else
        processSmall(map);
}

private boolean isLimitReached(Map<BigInteger, List<String>> map)
{
    return map.values().stream().anyMatch(list -> list.size() > limit);
}

Ou cela pourrait signifier quelque chose de complètement différent… ;-)

siegi
la source