Odwracanie instrukcji JEŻELI

39

Więc programuję od kilku lat, a ostatnio zacząłem używać ReSharpera więcej. Jedną z rzeczy, które ReSharper zawsze mi sugeruje, jest „odwrócenie” instrukcji „if” w celu zmniejszenia zagnieżdżania ”.

Powiedzmy, że mam ten kod:

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
    }
}

A ReSharper zasugeruje, żebym to zrobił:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Wygląda na to, że ReSharper ZAWSZE sugeruje, że odwrócę IF, bez względu na to, jak dużo się dzieje. Problem polega na tym, że lubię zagnieżdżanie się w przynajmniej NIEKTÓRYCH sytuacjach. Wydaje mi się, że łatwiej jest przeczytać i dowiedzieć się, co się dzieje w niektórych miejscach. Nie zawsze tak jest, ale czasami czuję się bardziej komfortowo w gniazdowaniu.

Moje pytanie brzmi: oprócz osobistych preferencji lub czytelności, czy istnieje powód, aby ograniczyć zagnieżdżanie? Czy istnieje różnica w wydajności lub coś innego, czego mogę nie być świadomy?

Barry Franklin
źródło
1
Możliwy duplikat utrzymywania niskiego poziomu wcięcia
komnata
24
Kluczową kwestią jest tutaj „Resharper sugeruje”. Spójrz na sugestię, jeśli ułatwia ona odczytanie kodu i jest spójna, użyj go. Robi to samo z / dla każdej pętli.
Jon Raynor
Zasadniczo obniżam te wskazówki dotyczące zmiany kolejności, nie zawsze stosuję się do nich, więc nie pojawiają się już w bocznym pasku.
CodesInChaos
1
ReSharper usunął negatyw, co zwykle jest dobrą rzeczą. Nie sugerowało to jednak warunku trójskładnikowego, który pasowałby tutaj, jeśli blok jest tak prosty jak twój przykład.
dcorking
2
Czy ReSharper nie sugeruje również przeniesienia warunku do zapytania? foreach (someObject w someObjectList.Where (object => object! = null)) {...}
Roman Reiner

Odpowiedzi:

63

To zależy. W twoim przykładzie posiadanie nieodwróconej ifinstrukcji nie stanowi problemu, ponieważ jej treść ifjest bardzo krótka. Jednak zwykle chcesz odwrócić instrukcje, gdy ciała rosną.

Jesteśmy tylko ludźmi i trzymanie wielu rzeczy na raz w naszych głowach jest trudne.

Gdy treść instrukcji jest duża, odwrócenie instrukcji nie tylko zmniejsza zagnieżdżenie, ale także mówi deweloperowi, że mogą zapomnieć o wszystkim powyżej tej instrukcji i skupić się tylko na pozostałych. Bardzo prawdopodobne jest, że użyjesz odwróconych instrukcji, aby pozbyć się niewłaściwych wartości tak szybko, jak to możliwe. Gdy dowiesz się, że są one wyłączone z gry, pozostały tylko wartości, które można faktycznie przetworzyć.

Andy
źródło
64
Uwielbiam patrzeć w tę stronę fala opinii deweloperów. Jest tyle zagnieżdżanie w kodzie po prostu dlatego, że z niezwykłą popularnej złudzenia tym break, continuei wielokrotne returnnie są skonstruowane.
JimmyJames
6
Problemem jest przerwa itp. Nadal masz kontrolę nad przepływem, którą musisz zachować w głowie „To nie może być x lub opuściłby pętlę u góry” może nie wymagać dalszych przemyśleń, jeśli jest to zerowa kontrola, która i tak rzuciłaby wyjątek. Ale nie jest tak dobrze, gdy jest bardziej dopracowany „uruchamia tylko połowę tego kodu dla tego statusu w czwartki”
Ewan
2
@Ewan: Jaka jest różnica między „to nie może być x, ponieważ opuściłby pętlę u góry”, a „to nie może być x lub nie wszedłby w ten blok”?
Collin,
nic nie jest złożonością i znaczeniem x, co jest ważne
Ewan
4
@Ewan: myślę break/ continue/ returnmają realną przewagę nad elseblokiem. W przypadku wczesnych wyjść, są 2 bloki : blok wyjścia i blok pobytu; ze else3 bloki : if, else, a co przychodzi po (który jest używany niezależnie od dziedziny podjęte). Wolę mieć mniej bloków.
Matthieu M.
33

Nie unikaj negatywów. Ludzie ssają jak je analizują, nawet jeśli CPU je uwielbia.

Szanuj jednak idiomy. My, ludzie, jesteśmy przyzwyczajeni, więc wolimy dobrze zużyte ścieżki od bezpośrednich ścieżek pełnych chwastów.

foo != nullniestety stał się idiomem. Ledwo zauważam już poszczególne części. Patrzę na to i po prostu myślę: „och, teraz można bezpiecznie wysadzić foo”.

Jeśli chcesz to obalić (och, któregoś dnia, proszę), potrzebujesz naprawdę mocnego argumentu. Potrzebujesz czegoś, co pozwoli moim oczom bez trudu zsunąć się z tego i zrozumie to w jednej chwili.

Jednak dałeś nam to:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Co wcale nie jest takie samo!

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    if(someGlobalObject == null) continue;
    someSideEffectObject = someGlobalObject.SomeProperty;
}

To nie robi tego, czego spodziewał się mój leniwy mózg. Myślałem, że mogę dodawać niezależny kod, ale nie mogę. To sprawia, że ​​myślę o rzeczach, o których nie chcę teraz myśleć. Złamałeś mój idiom, ale nie dałeś mi lepszego. Wszystko dlatego, że chciałeś mnie ochronić przed tym jednym negatywnym, który spłonął, to moje paskudne ja w mojej duszy.

Przykro mi, może ReSharper ma rację, sugerując 90% czasu, ale w zerowych czekach myślę, że znalazłeś przypadek narożny, w którym najlepiej go zignorować. Narzędzia po prostu nie są dobre w uczeniu cię, czym jest czytelny kod. Zapytaj człowieka. Dokonaj przeglądu. Ale nie ślepo podążaj za narzędziem.

candied_orange
źródło
1
Cóż, jeśli masz więcej kodu w pętli, to jak to działa, zależy od tego, jak wszystko się połączy. To nie jest niezależny kod. Refaktoryzowany kod w pytaniu był poprawny na przykład, ale może nie być poprawny, gdy nie jest w izolacji - czy to był punkt, w którym zamierzałeś? (+1 za „nie unikaj negatywów”)
Baldrickk
@Baldrickk if (foo != null){ foo.something() }pozwala mi dodawać kod bez względu na to, czy fooma wartość null. To nie Nawet jeśli nie muszę tego robić teraz, nie czuję się zmotywowany, by popsuć przyszłość, mówiąc „no cóż, mogą to po prostu zreformować, jeśli będą tego potrzebować”. Feh. Oprogramowanie jest miękkie tylko wtedy, gdy można je łatwo zmienić.
candied_orange
4
„kontynuuj dodawanie kodu bez opieki” Albo kod powinien być powiązany (w przeciwnym razie prawdopodobnie powinien być osobny) lub należy zadbać o zrozumienie funkcjonalności modyfikowanej funkcji / bloku, ponieważ dodanie dowolnego kodu może zmienić zachowanie poza zamierzonym. W przypadku takich prostych przypadków nie wydaje mi się, że to i tak ma znaczenie. W bardziej skomplikowanych przypadkach spodziewam się, że zrozumienie kodu będzie miało pierwszeństwo, więc wybrałbym to, co uważam za najbardziej jasne, którym może być dowolny styl.
Baldrickk
powiązane i powiązane nie są tym samym.
candied_orange
1
Nie należy unikać negatywów: D
VitalyB
20

To stary argument, czy przerwa jest gorsza niż zagnieżdżanie.

Wydaje się, że ludzie akceptują wczesny powrót do sprawdzania parametrów, ale w większości sposób jest to niezadowolony.

Osobiście unikam kontynuowania, przerywania, goto itp., Nawet jeśli oznacza to więcej zagnieżdżania. Ale w większości przypadków można uniknąć obu.

foreach (someObject in someObjectList.where(i=>i != null))
{
    someOtherObject = someObject.SomeProperty;
}

Oczywiście zagnieżdżanie utrudnia śledzenie wielu warstw

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
            }
        }
    }
}

Najgorsze jest to, że oboje macie

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
                return;
            }
            continue;
        }
        someObject.z --;
        if(myFunctionWithSideEffects(someObject) > 6) break;
    }
}
Ewan
źródło
11
Uwielbiam tę linię: foreach (subObject in someObjectList.where(i=>i != null))nie wiem, dlaczego nigdy nie myślałem o czymś takim.
Barry Franklin
@BarryFranklin ReSharper powinien mieć zielone kropki na foreach z „Konwertuj na wyrażenie LINQ” jako sugestią, która by to zrobiła za Ciebie. W zależności od operacji może również wykonywać inne metody linq, takie jak Sum lub Max.
Kevin Fee,
@BarryFranklin Jest to prawdopodobnie sugestia, którą chcesz ustawić na niskim poziomie i używać tylko według uznania. Chociaż działa dobrze w przypadku trywialnych przypadków, takich jak ten przykład, w bardziej złożonym kodzie widzę sposób, w jaki bardziej złożone warunkowe części dzielą się na bity, które można poddać Linqifiable, a pozostała część ciała ma tendencję do tworzenia rzeczy, które są znacznie trudniejsze do zrozumienia niż oryginał. Zasadniczo spróbuję przynajmniej zasugerować, ponieważ kiedy można przekonwertować całą pętlę na Linq, wyniki są często rozsądne; ale pół i pół wyniki wydają się brzydko szybkie IMO.
Dan Neely,
Jak na ironię, edycja przez resharper ma dokładnie taki sam poziom zagnieżdżenia, ponieważ ma także if, po którym następuje jedna linijka.
Peter
heh, ale nie ma aparatów ortodontycznych! co jest najwyraźniej dozwolone: ​​/
Ewan
6

Problem continuepolega na tym, że jeśli dodasz więcej wierszy do kodu, logika komplikuje się i może zawierać błędy. na przykład

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    ...  imagine that there is some code here ...

    // added later by a Junior Developer
    doSomeOtherFunction(someObject);
}

Czy chcesz zadzwonić doSomeOtherFunction()do wszystkich someObject, czy tylko, jeśli nie ma wartości null? W przypadku oryginalnego kodu masz tę opcję i jest ona wyraźniejsza. Z continuejednym może zapomnieć „och, tak, tam pominęliśmy nulls” i nie masz nawet możliwości wywołania go dla niektórych obiektów, chyba że umieścisz to wywołanie na początku metody, co może być niemożliwe lub poprawne z innych powodów.

Tak, wiele zagnieżdżeń jest brzydkich i być może mylących, ale w tym przypadku użyłbym tradycyjnego stylu.

Pytanie dodatkowe: dlaczego na początku są puste wartości? Lepiej nigdy nie dodawać null w pierwszej kolejności, w którym to przypadku logika przetwarzania jest znacznie prostsza.

użytkownik949300
źródło
12
Wewnątrz pętli nie powinno być wystarczającej ilości kodu, aby nie można było zobaczyć kontroli zerowej na górze bez przewijania.
Bryan Boettcher,
@Bryan Boettcher zgodził się, ale nie cały kod jest tak dobrze napisany. I nawet kilka wierszy złożonego kodu może sprawić, że zapomnisz (lub pomylisz się) co do kontynuacji. W praktyce czuję się dobrze, returnponieważ tworzą one „czystą przerwę”, ale IMO breaki continueprowadzą do zamieszania, a w większości przypadków najlepiej ich unikać.
user949300,
4
@ user949300 Po dziesięciu latach rozwoju nigdy nie znalazłem przerwy ani nie powodowałem zamieszania. Miałem ich w zamieszaniu, ale nigdy nie byli przyczyną. Zwykle przyczyną były złe decyzje programisty i powodowały zamieszanie, niezależnie od używanej broni.
corsiKa
1
@corsiKa Błąd Apple SSL jest tak naprawdę błędem goto, ale łatwo może być błędem przerwy lub kontynuacji. Kod jest jednak okropny ...
user949300,
3
@BryanBoettcher problem wydaje mi się, że ci sami deweloperzy, którzy myślą, że kontynuacja lub wielokrotne powroty są w porządku, również myślą, że zakopanie ich w ciałach dużych metod jest również w porządku. Więc każde doświadczenie, jakie mam z kontynuowaniem / wielokrotnymi zwrotami, było ogromne.
Andy,
3

Chodzi o wczesny powrót. Wczesna returnpętla staje się wcześnie continue.

Wiele dobrych odpowiedzi na to pytanie: czy powinienem wcześniej wrócić z funkcji, czy użyć instrukcji if?

Zauważ, że chociaż pytanie jest zamknięte jako oparte na opinii, 430+ głosów jest za wczesnym powrotem, ~ 50 głosów to „to zależy”, a 8 jest przeciwnych wcześniejszemu powrotowi. Istnieje więc wyjątkowo silny konsensus (albo to, albo źle oceniam, co jest prawdopodobne).

Osobiście, jeśli ciało pętli będzie podlegać wczesnemu kontynuowaniu i wystarczająco długo, aby miało znaczenie (3-4 linie), mam tendencję do wydobywania ciała pętli do funkcji, w której używam wczesnego powrotu zamiast wczesnego kontynuowania.

Piotr
źródło
2

Ta technika jest przydatna do poświadczania warunków wstępnych, gdy główna część metody ma miejsce, gdy te warunki są spełnione.

Często odwracam się ifsw mojej pracy i zatrudniamy fantomów. Często zdarzało się, że podczas przeglądu PR mogłem wskazać, w jaki sposób mój kolega może usunąć trzy poziomy zagnieżdżania! Zdarza się to rzadziej, ponieważ wdrażamy nasze ifs.

Oto zabawkowy przykład czegoś, co można rozwinąć

function foobar(x, y, z) {
    if (x > 0) {
        if (z != null && isGamma(y)) {
            // ~10 lines of code
            // return something
        }
        else {
           return y
        }
    }
    else {
      return y + z
    }
}

Kod taki jak ten, w którym występuje JEDNA interesująca sprawa (gdzie reszta to po prostu zwroty lub małe bloki instrukcji) są powszechne. Przepisanie powyższej wersji jako trywialnej jest banalne

function foobar(x, y, z) {
    if (x <=0) {
        return y + z
    }
    if (z == null || !isGamma(y) {
       return y
    }
    // ~ 10 lines of code
    // return something
}

Tutaj nasz znaczący kod, nieuwzględnione 10 wierszy, nie jest potrójnie wcięty w funkcji. Jeśli masz pierwszy styl, albo kusi cię przeformułowanie długiego bloku na metodę, albo tolerowanie wcięcia. W tym miejscu zaczynają się oszczędności. W jednym miejscu są to trywialne oszczędności, ale w wielu metodach w wielu klasach oszczędza się potrzeby generowania dodatkowej funkcji, aby kod był bardziej przejrzysty i nie ma aż tyle ohydnych wielopoziomowych wcięcie .


W odpowiedzi na przykładowy kod OP zgadzam się, że jest to nadmierne załamanie. Zwłaszcza, że ​​w używanym przeze mnie stylu 1 TB inwersja powoduje zwiększenie rozmiaru kodu.

Lan
źródło
0

Sugestie handlarzy są często przydatne, ale zawsze należy je oceniać krytycznie. Szuka pewnych predefiniowanych wzorców kodu i sugeruje transformacje, ale nie może brać pod uwagę wszystkich czynników, które czynią kod mniej lub bardziej czytelnym dla ludzi.

Wszystkie inne rzeczy są równe, preferowane jest mniej zagnieżdżania, dlatego ReSharper zasugeruje transformację, która zmniejszy zagnieżdżanie. Ale wszystkie inne rzeczy niesobie równe: w tym przypadku transformacja wymaga wprowadzenia a continue, co oznacza, że ​​wynikowy kod jest trudniejszy do naśladowania.

W niektórych przypadkach zamiana poziomu zagnieżdżenia na rzeczywiście continue może być ulepszeniem, ale zależy to od semantyki omawianego kodu, co wykracza poza zrozumienie reguł mechanicznych ReSharpers.

W twoim przypadku sugestia ReSharper pogorszyłaby kod. Więc zignoruj ​​to. Lub lepiej: Nie używaj sugerowanej transformacji, ale zastanów się, jak można poprawić kod. Zauważ, że możesz przypisywać tę samą zmienną wiele razy, ale efekt ma tylko ostatnie przypisanie, ponieważ poprzednie zostanie po prostu nadpisane. To nie wygląda jak błąd. Sugestia ReSharpers nie naprawia błędu, ale sprawia, że ​​nieco bardziej oczywiste jest, że dzieje się jakaś wątpliwa logika.

JacquesB
źródło
0

Myślę, że większą kwestią jest to, że cel pętli określa najlepszy sposób organizacji przepływu w pętli. Jeśli odfiltrowujesz niektóre elementy, zanim przejdziesz przez pozostałe, wtedy continuewcześniejsze usunięcie ma sens. Jeśli jednak wykonujesz różne rodzaje przetwarzania w pętli, sensowniej byłoby uczynić to ifnaprawdę oczywistym, na przykład:

for(obj in objectList) {
    if(obj == null) continue;
    if(obj.getColour().equals(Color.BLUE)) {
        // Handle blue objects
    } else {
        // Handle non-blue objects
    }
}

Zauważ, że w strumieniach Java lub innych idiomach funkcjonalnych możesz oddzielić filtrowanie od zapętlenia, używając:

items.stream()
    .filter(i -> (i != null))
    .filter(i -> i.getColour().equals(Color.BLUE))
    .forEach(i -> {
        // Process blue objects
    });

Nawiasem mówiąc, jest to jedna z rzeczy, które uwielbiam w odwróconym Perl if ( unless) zastosowanym do pojedynczych instrukcji, co pozwala na następujący rodzaj kodu, który uważam za bardzo łatwy do odczytania:

foreach my $item (@items) {
    next unless defined $item;
    carp "Only blue items are supported! Failed on item $item" 
       unless ($item->get_colour() eq 'blue');
    ...
}
Gauraw
źródło