Czy ustawianie flagi w pętli w celu użycia jej później jest zapachem kodu?

Mam fragment kodu, w którym iteruję mapę, dopóki jakiś warunek nie jest spełniony, a następnie użyję tego warunku, aby zrobić więcej rzeczy.

Przykład:

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
}

Czuję, że ustawiam flagę, a następnie używanie tego do robienia więcej rzeczy to zapach kodu.

Czy mam rację? Jak mogę to usunąć?

Nie ma nic złego w używaniu wartości boolowskiej zgodnie z jej przeznaczeniem: zapisaniem binarnego rozróżnienia.

Gdyby kazano mi zrefaktoryzować ten kod, prawdopodobnie umieściłbym pętlę w swojej własnej metodzie, aby przypisanie + breakzmieniło się w return; wtedy nie potrzebujesz nawet zmiennej, możesz po prostu powiedzieć

if(fill_list_from_map()) {
  ...

To niekoniecznie jest złe, a czasem jest to najlepsze rozwiązanie. Ale ustawienie takich flag w zagnieżdżonych blokach może utrudnić śledzenie kodu.

Problem polega na tym, że masz bloki do rozgraniczenia zakresów, ale potem masz flagi, które komunikują się między zakresami, przerywając logiczną izolację bloków. Na przykład, limitFlagfałsz będzie, jeśli mapjest null, więc kod „zrób coś” zostanie wykonany, jeśli mapjest null. To może być to, co zamierzasz, ale może to być błąd, którego łatwo przeoczyć, ponieważ warunki dla tej flagi są zdefiniowane gdzie indziej, w zagnieżdżonym zakresie. Jeśli potrafisz zachować informacje i logikę w jak najściślejszym zakresie, powinieneś spróbować to zrobić.

Odradzałbym rozumowanie na temat „zapachów kodu”. To tylko najbardziej leniwy możliwy sposób zracjonalizowania własnych uprzedzeń. Z czasem rozwiniesz wiele uprzedzeń i wiele z nich będzie rozsądnych, ale wiele z nich będzie głupich.

Zamiast tego powinieneś mieć praktyczne (tj. Nie dogmatyczne) powody preferowania jednej rzeczy nad drugą i unikaj myślenia, że ​​powinieneś mieć taką samą odpowiedź na wszystkie podobne pytania.

„Zapachy kodowe” są, gdy nie myślisz. Jeśli naprawdę chcesz pomyśleć o kodzie, zrób to dobrze!

W takim przypadku decyzja może naprawdę pójść w obie strony, w zależności od otaczającego kodu. To naprawdę zależy od tego, co według ciebie jest najwyraźniejszym sposobem myślenia o tym, co robi kod. („czysty” kod to kod, który wyraźnie komunikuje, co robi innym programistom i ułatwia im sprawdzenie, czy jest poprawny)

Wiele razy ludzie piszą metody podzielone na fazy, w których kod najpierw określa, co powinien wiedzieć o danych, a następnie działa na nich. Jeśli zarówno część „określająca”, jak i część „działaj na nią” są nieco skomplikowane, może to mieć sens, a często „to, co musi wiedzieć” może być przenoszone między fazami w flagach boolowskich. Naprawdę wolałbym, żebyś nadał flagę lepszą nazwę. Coś w rodzaju „largeEntryExists” sprawiłoby, że kod byłby znacznie czystszy.

Z drugiej strony, jeśli kod „// Zrób coś” jest bardzo prosty, sensowniej jest umieścić go w ifbloku zamiast ustawiać flagę. To przybliża efekt do przyczyny, a czytnik nie musi skanować reszty kodu, aby upewnić się, że flaga zachowuje ustawioną wartość.

Tak, jest to zapach kodu (zwracaj uwagę wszystkich, którzy to robią).

Najważniejsze dla mnie jest użycie tego breakoświadczenia. Jeśli go nie użyjesz, będziesz iterował więcej elementów niż jest to wymagane, ale użycie go daje dwa możliwe punkty wyjścia z pętli.

Nie jest to poważny problem z twoim przykładem, ale możesz sobie wyobrazić, że gdy warunkowe lub warunkowe wewnątrz pętli stają się bardziej złożone lub kolejność początkowej listy staje się ważna, łatwiej jest błędowi wkraść się do kodu.

Gdy kod jest tak prosty jak twój przykład, można go zredukować do whilepętli lub równoważnej mapy, konstruuj filtry.

Gdy kod jest na tyle skomplikowany, że wymaga flag i przerw, będzie podatny na błędy.

Tak jak w przypadku wszystkich zapachów kodu: jeśli widzisz flagę, spróbuj zastąpić ją flagą while. Jeśli nie możesz, dodaj dodatkowe testy jednostkowe.

Wystarczy użyć nazwy innej niż limitFlag, która mówi, co faktycznie sprawdzasz. I dlaczego logujesz coś, gdy mapa jest nieobecna lub pusta? limtFlag będzie fałszywy, wszystko na czym ci zależy. Pętla jest w porządku, jeśli mapa jest pusta, więc nie trzeba jej sprawdzać.

Moim zdaniem ustawienie wartości logicznej w celu przekazania informacji, które już posiadałeś, jest złą praktyką. Jeśli nie ma łatwej alternatywy, oznacza to prawdopodobnie większy problem, na przykład słabą enkapsulację.

Powinieneś przenieść logikę pętli for do metody fillUpList, aby złamała się, jeśli limit zostanie osiągnięty. Następnie sprawdź bezpośrednio rozmiar listy.

Jeśli to zepsuje Twój kod, dlaczego?

Po pierwsze ogólny przypadek: użycie flagi do sprawdzenia, czy jakiś element kolekcji spełnia określony warunek, nie jest rzadkością. Ale wzór, który najczęściej widziałem w celu rozwiązania tego problemu, to przesunięcie czeku w dodatkową metodę i bezpośredni powrót z niego (jak opisał Kilian Foth w swojej odpowiedzi ):

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

Od wersji Java 8 istnieje bardziej zwięzły sposób, używając Stream.anyMatch(…):

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

W twoim przypadku prawdopodobnie wyglądałoby to tak (zakładając list == entry.getValue()w twoim pytaniu):

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

Problemem w twoim konkretnym przykładzie jest dodatkowe połączenie z fillUpList(). Odpowiedź zależy w dużej mierze od tego, co ma zrobić ta metoda.

Uwaga dodatkowa: wezwanie do fillUpList()nie ma większego sensu, ponieważ nie zależy od elementu, który aktualnie iterujesz. Wydaje mi się, że jest to konsekwencja zmniejszenia kodu w celu dopasowania go do formatu pytania. Ale właśnie to prowadzi do sztucznego przykładu, który jest trudny do interpretacji, a zatem trudny do uzasadnienia. Dlatego tak ważne jest podanie minimalnego, kompletnego i weryfikowalnego przykładu .

Zakładam więc, że rzeczywisty kod przekazuje prąd entrydo metody.

Ale jest więcej pytań, które należy zadać:

• Czy listy na mapie są puste przed dotarciem do tego kodu? Jeśli tak, to dlaczego istnieje już mapa, a nie tylko lista lub zestaw BigIntegerkluczy? Jeśli nie są puste, dlaczego musisz wypełnić listy? Jeśli na liście znajdują się już elementy, czy nie jest to aktualizacja lub inne obliczenie w tym przypadku?
• Co powoduje, że lista staje się większa niż limit? Czy jest to warunek błędu, czy często się zdarza? Czy jest to spowodowane nieprawidłowymi danymi wejściowymi?
• Czy potrzebujesz list obliczonych do momentu, gdy osiągniesz listę większą niż limit?
• Co robi część „ Zrób coś ”?
• Czy ponownie zaczynasz napełnianie po tej części?

To tylko niektóre pytania, które przyszły mi do głowy, gdy próbowałem zrozumieć fragment kodu. Tak więc, moim zdaniem, jest to prawdziwy zapach kodu : Twój kod nie komunikuje jasno intencji.

Może to oznaczać albo („wszystko albo nic”, a osiągnięcie limitu oznacza błąd):

/**
 * 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.
}

Lub może to oznaczać („aktualizacja do pierwszego problemu”):

/**
 * 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.
}

Lub to („zaktualizuj wszystkie listy, ale zachowaj oryginalną listę, jeśli stanie się zbyt duża”):

/**
 * 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
}

Lub nawet następujące (używając computeFoos(…)pierwszego przykładu, ale bez wyjątków):

/**
 * 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);
}

Lub może to oznaczać coś zupełnie innego… ;-)