Odwracanie instrukcji JEŻELI

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?

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

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.

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;
    }
}

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.

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.

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.

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 nie są sobie 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.

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');
    ...
}