Eleganckie sposoby obsługi, jeśli (jeśli inaczej) jeszcze

161

Jest to drobny problem, ale za każdym razem, gdy muszę kodować coś takiego, powtarzanie przeszkadza mi, ale nie jestem pewien, czy którekolwiek z rozwiązań nie jest gorsze.

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}
  • Czy istnieje nazwa dla tego rodzaju logiki?
  • Czy jestem trochę za obsesyjna?

Jestem otwarty na sugestie złego kodu, choćby ze względu na ciekawość ...

Benjol
źródło
8
@Emmad Kareem: dwa DefaultActionpołączenia naruszają zasadę OSUSZANIA
Abyx
Dziękuję za odpowiedź, ale myślę, że jest w porządku, z wyjątkiem nie używania try / catch, ponieważ mogą występować błędy, które nie zwracają wyników i powodują błędy (w zależności od języka programowania).
NoChance
20
Myślę, że głównym problemem jest to, że pracujesz na niespójnych poziomach abstrakcji . Im wyższy poziom abstrakcji jest: make sure I have valid data for DoSomething(), and then DoSomething() with it. Otherwise, take DefaultAction(). Szczegółowe szczegóły dotyczące upewnienia się, że masz dane do DoSomething (), są na niższym poziomie abstrakcji, a zatem powinny mieć inną funkcję. Ta funkcja będzie miała nazwę na wyższym poziomie abstrakcji, a jej implementacja będzie na niskim poziomie. Dobre odpowiedzi poniżej rozwiązują ten problem.
Gilad Naor
6
Proszę podać język. Możliwe rozwiązania, standardowe idiomy i wieloletnie normy kulturowe są różne dla różnych języków i doprowadzą do różnych odpowiedzi na twoje pytanie.
Caleb
1
Możesz odnieść się do tej książki „Refaktoryzacja: ulepszanie projektu istniejącego kodu”. Istnieje kilka rozdziałów na temat struktury if-else, naprawdę przydatnej praktyki.
Vacker,

Odpowiedzi:

96

Wyodrębnij go do oddzielnej funkcji (metody) i użyj returninstrukcji:

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

A może lepiej oddzielić pobieranie treści i ich przetwarzanie:

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

Aktualizacja:

Dlaczego nie wyjątki, dlaczego OpenFilenie zgłasza wyjątku IO:
Myślę, że to naprawdę ogólne pytanie, a nie pytanie o IO pliku. Nazwy takie jak FileExists, OpenFilemoże być mylące, ale jeśli je zastąpić Foo, Baritp, - byłoby jasne, że DefaultActionmożna nazwać jak najczęściej DoSomething, więc może to nie być przypadek wyjątkowy. Péter Török napisał o tym na końcu swojej odpowiedzi

Dlaczego w drugim wariancie występuje trójskładnikowy operator warunkowy:
Gdyby istniał tag [C ++], napisałbym ifoświadczenie z deklaracją contentsczęści:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

Ale w przypadku innych (podobnych do C) języków if(contents) ...; else ...;jest to dokładnie to samo, co wyrażenie wyrażenia z potrójnym operatorem warunkowym, ale dłużej. Ponieważ główną częścią kodu była get_contentsfunkcja, właśnie użyłem krótszej wersji (a także pominiętego contentstypu). Tak czy inaczej, to jest poza tym pytaniem.

Abyx
źródło
93
+1 za wielokrotne zwroty - gdy metody są wystarczająco małe , to podejście działa najlepiej dla mnie
gnat
Nie jestem wielkim fanem wielu zwrotów, chociaż czasami go używam. Jest to całkiem rozsądne w przypadku czegoś prostego, ale po prostu nie skaluje się dobrze. Naszym standardem jest unikanie tego dla wszystkich szalonych prostych metod, ponieważ metody mają tendencję do powiększania się, a nie kurczenia.
Brian Knoblauch
3
Wiele ścieżek powrotnych może mieć negatywny wpływ na wydajność w programach C ++, co niweczy wysiłki optymalizatora dotyczące zastosowania RVO (także NRVO, chyba że każda ścieżka zwraca ten sam obiekt).
Functastic
Polecam odwrócenie logiki drugiego rozwiązania: {if (plik istnieje) {ustaw zawartość; if (kiedyś) {zwróć treść; }} return null; } Upraszcza przepływ i zmniejsza liczbę wierszy.
Wedge
1
Cześć Abyx. Zauważyłem, że włączyłeś niektóre opinie z komentarzy tutaj: dziękuję za zrobienie tego. Oczyściłem wszystko, co zostało poruszone w twojej odpowiedzi i innych odpowiedziach.
56

Jeśli używany język programowania (0) powoduje zwarcie porównań binarnych (tzn. Jeśli nie wywołuje, SomeTestjeśli FileExistszwraca false) i (1) przypisanie zwraca wartość (wynik OpenFilejest przypisywany, contentsa następnie ta wartość jest przekazywana jako argument do SomeTest), możesz użyć czegoś takiego jak poniżej, ale nadal zaleca się komentowanie kodu, zauważając, że singiel =jest celowy.

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

W zależności od tego, jak skomplikowany jest if, może być lepiej mieć zmienną flagową (która oddziela testowanie warunków powodzenia / niepowodzenia kodem, który obsługuje błąd DefaultActionw tym przypadku)

frozenkoi
źródło
Tak bym to zrobił.
Anthony
13
ifMoim zdaniem dość obrzydliwe jest umieszczenie tak dużej ilości kodu w oświadczeniu.
moteutsch,
15
Przeciwnie, lubię tego rodzaju stwierdzenie „jeśli coś istnieje i spełnia ten warunek”. +1
Gorpik
Ja też! Osobiście nie podoba mi się sposób, w jaki ludzie używają wielu zwrotów, niektóre przesłanki nie są spełnione. Dlaczego nie odwrócisz tych ifs i nie wykonasz kodu, jeśli zostaną spełnione?
klaar
„Jeśli coś istnieje i spełnia ten warunek” jest w porządku. „jeśli coś istnieje i zrób tutaj coś stycznie powiązanego i spełni ten warunek”, OTOH jest mylące. Innymi słowy, nie lubię skutków ubocznych w stanie.
Piskvor
26

Poważniejsze niż powtarzanie wywołania DefaultAction jest sam styl, ponieważ kod jest pisany nieortogonalny (patrz ta odpowiedź z dobrych powodów, aby pisać ortogonalnie).

Aby pokazać, dlaczego kod nieortogonalny jest zły, rozważ oryginalny przykład, kiedy wprowadzono nowy wymóg, aby nie otwierać pliku, jeśli jest on przechowywany na dysku sieciowym. Cóż, moglibyśmy po prostu zaktualizować kod do następującego:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Ale pojawia się również wymóg, aby nie otwierać dużych plików o wielkości powyżej 2 Gb. Cóż, właśnie aktualizujemy:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Powinno być bardzo jasne, że taki styl kodu będzie ogromnym problemem w utrzymaniu.

Wśród odpowiedzi tutaj poprawnie napisanych ortogonalnie są drugi przykład Abyxa i odpowiedź Jana Hudca , więc nie powtórzę tego, po prostu zaznaczę , że dodanie dwóch wymagań w tych odpowiedziach byłoby po prostu

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(lub goto notexists;zamiast return null;), nie wpływając na żaden inny kod niż te dodane wiersze . Np. Ortogonalny.

Podczas testowania ogólną zasadą powinno być testowanie wyjątków, a nie normalny przypadek .

hlovdal
źródło
8
+1 dla mnie. Wczesne zwroty pomagają uniknąć anty-wzoru grotu strzały. Zobacz codinghorror.com/blog/2006/01/flattening-arrow-code.html i lostechies.com/chrismissal/2009/05/27/... Przed przeczytaniem o tym wzorze zawsze subskrybowałem 1 wejście / wyjście na funkcję teorię z powodu tego, czego nauczono mnie 15 lat temu. Wydaje mi się, że dzięki temu kod jest o wiele łatwiejszy do odczytania i, jak wspomniałeś, łatwiejszy w utrzymaniu.
Pan Moose
3
@MrMoose: Twoja wzmianka o anty-wzorach grotów odpowiada wyraźne pytanie Benjola: „Czy istnieje nazwa dla tego rodzaju logiki?” Opublikuj to jako odpowiedź i masz mój głos.
poza
To świetna odpowiedź, dziękuję. I @MrMoose: „anty-wzór grotu strzałki” prawdopodobnie odpowiada na moją pierwszą kulę, więc tak, opublikuj ją. Nie mogę obiecać, że to zaakceptuję, ale zasługuje na głosy!
Benjol,
@outis. Dzięki. Dodałem odpowiedź. Anty wzór grotu strzały z pewnością jest istotny w poście hlovdala, a jego klauzule ochronne dobrze sprawdzają się przy ich omijaniu. Nie wiem, jak mogłeś odpowiedzieć na drugi punkt kulki na to. Nie jestem uprawniony do zdiagnozowania tego :)
Pan Moose
4
+1 za „wyjątki testowe, a nie normalny przypadek”.
Roy Tinker,
25

Oczywiście:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

Powiedziałeś, że jesteś otwarty nawet na złe rozwiązania, więc użycie zła goto się liczy, nie?

W rzeczywistości, w zależności od kontekstu, rozwiązanie to może być mniej złe niż zło wykonujące akcję dwukrotnie lub zła dodatkowa zmienna. Owinąłem ją w funkcję, ponieważ zdecydowanie nie byłaby OK w środku długiej funkcji (zwłaszcza ze względu na powrót w środku). Ale niż długa funkcja nie jest OK, kropka.

Kiedy masz wyjątki, będą łatwiejsze do odczytania, szczególnie jeśli możesz mieć OpenFile i DoSomething po prostu rzucać wyjątek, jeśli warunki nie są spełnione, więc nie potrzebujesz wyraźnych kontroli. Z drugiej strony w C ++, Javie i C # zgłaszanie wyjątku jest powolną operacją, więc od punktu wydajności goto jest nadal preferowane.


Uwaga na temat „zła”: C ++ FAQ 6.15 definiuje „zło” jako:

Oznacza to , że takiego i takiego należy unikać przez większość czasu, ale nie należy tego unikać przez cały czas. Na przykład skończysz używać tych „złych” rzeczy, ilekroć są one „najmniejszymi złymi alternatywami zła”.

I dotyczy to gotow tym kontekście. Strukturyzowane konstrukcje kontroli przepływu są lepsze przez większość czasu, ale kiedy znajdziesz się w sytuacji, w której gromadzą zbyt wiele własnych zła, takie jak przypisanie warunku, zagnieżdżanie na głębokości ponad 3 poziomów, powielanie kodu lub długie warunki, gotomogą po prostu się skończyć bycie mniej złym.

Jan Hudec
źródło
11
Mój kursor unosi się nad przyciskiem akceptacji ... po prostu na przekór wszystkim purystom. Och, pokusa: D
Benjol,
2
Tak tak! Jest to absolutnie „właściwy” sposób pisania kodu. Struktura kodu ma teraz postać „Jeśli błąd, obsłużyć błąd. Normalne działanie. Jeśli błąd, obsłużyć błąd. Normalne działanie”, dokładnie tak, jak powinno być. Cały „normalny” kod jest zapisywany tylko z jednym wcięciem poziomu, podczas gdy cały kod związany z błędem ma dwa poziomy wcięcia. Tak więc normalny I NAJWAŻNIEJSZY kod otrzymuje najbardziej widoczne miejsce wizualne i możliwe jest bardzo szybkie i łatwe odczytanie przepływu sekwencyjnie w dół. Za wszelką cenę zaakceptuj tę odpowiedź.
hlovdal
2
Innym aspektem jest to, że kod pisany w ten sposób jest ortogonalny. Na przykład dwa wiersze „if (! FileExists (plik)) \ n \ tgoto notexists;” są teraz TYLKO związane z obsługą tego aspektu pojedynczego błędu (KISS) i, co najważniejsze, nie wpływa na żadną z pozostałych linii . W odpowiedzi stackoverflow.com/a/3272062/23118 wymieniono kilka dobrych powodów, dla których kod powinien być ortogonalny.
hlovdal
5
Mówiąc o złych rozwiązaniach: Mogę dostać twoje rozwiązanie bez goto:for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction();
Herby
4
@herby: Twoje rozwiązanie jest bardziej złe niż goto, ponieważ nadużywasz breakw sposób, którego nikt nie spodziewa się, że zostanie wykorzystany, więc ludzie czytający kod będą mieli więcej problemów ze zrozumieniem, gdzie prowadzi przerwa, niż goto, co mówi wprost. Poza tym używasz nieskończonej pętli, która będzie działać tylko raz, co będzie dość mylące. Niestety, też do { ... } while(0)nie jest do końca czytelny, ponieważ widzisz tylko zabawny blok, gdy dojdziesz do końca, a C nie obsługuje przełamywania innych bloków (w przeciwieństwie do Perla).
Jan Hudec
12
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
herby
źródło
lub przejdź do dodatkowego mężczyzny i utwórz dodatkową metodę FileExistsAndConditionMet (plik) ...
UncleZeiv
@herby SomeTestmoże mieć taką samą semantykę jak istnienie pliku, jeśli SomeTestsprawdza typ pliku, na przykład sprawdza, czy .gif jest naprawdę plikiem GIF.
Abyx
1
Tak. Zależy. @Benjol wie lepiej.
herby
3
... oczywiście miałem na myśli „idź więcej” ... :)
UncleZeiv
2
To zabiera ravioli do kończyn, nawet ja nie chodzę (i jestem w tym ekstremalny) ... Myślę, że teraz jest to całkiem czytelne, biorąc pod uwagę contents && f(contents). Dwie funkcje, aby uratować jeszcze jedną?
herby
12

Jedna możliwość:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Oczywiście powoduje to, że kod jest nieco bardziej złożony w inny sposób. Jest to więc w dużej mierze pytanie dotyczące stylu.

Innym podejściem byłoby stosowanie wyjątków, np .:

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

Wygląda to na prostsze, ale ma zastosowanie tylko wtedy, gdy

  • wiemy dokładnie, jakich wyjątków się spodziewać i DefaultAction()pasuje do każdego z nich
  • spodziewamy się, że przetwarzanie pliku zakończy się powodzeniem, a brakujący plik lub awaria SomeTest()jest oczywiście błędnym warunkiem, dlatego należy na niego rzucić wyjątek.
Péter Török
źródło
19
Nieeee ~! Nie jest to zmienna flagowa, to zdecydowanie zły sposób, ponieważ prowadzi do złożonego, trudnego do zrozumienia (gdzie-ta-flaga-staje się prawdziwa) i trudnego do zmiany kodu.
Abyx
Nie, jeśli ograniczysz go do możliwie lokalnego zasięgu. (function () { ... })()w JavaScript, { flag = false; ... }w C-jak itp.
herby
+1 za logikę wyjątku, która może być najbardziej odpowiednim rozwiązaniem w zależności od scenariusza.
Steven Jeuris
4
+1 To wzajemne „Nooooo!” jest śmieszne. Myślę, że zmienna statusu i wczesny powrót są uzasadnione w niektórych przypadkach. W bardziej skomplikowanych procedurach wybrałbym zmienną statusu, ponieważ zamiast dodawania złożoności, tak naprawdę robi ona jawną logikę. Nic w tym złego.
Grossvogel
1
To jest nasz preferowany format, w którym pracuję. Dwie główne użyteczne opcje wydają się być „wielokrotnymi zwrotami” i „zmiennymi flagowymi”. Żadne z nich nie wydaje się mieć żadnej prawdziwej przewagi, ale oba pasują do pewnych okoliczności lepiej niż inne. Muszę iść z typową skrzynką. Po prostu kolejna wojna religijna „Emacs” vs. „Vi”. :-)
Brian Knoblauch
11

Jest to na wyższym poziomie abstrakcji:

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

I to uzupełnia szczegóły.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }
}
Jeanne Pindar
źródło
11

Funkcje powinny zrobić jedną rzecz. Powinni to zrobić dobrze. Powinni to zrobić tylko.
- Robert Martin w Clean Code

Niektórzy uważają, że takie podejście jest nieco ekstremalne, ale jest również bardzo czyste. Pozwól mi zilustrować w Pythonie:

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

Kiedy mówi, że funkcje powinny robić jedną rzecz, ma na myśli jedną rzecz. processFile()wybiera akcję na podstawie wyniku testu i to wszystko. fileMeetsTest()łączy wszystkie warunki testu i to wszystko. contentsTest()przenosi pierwszą linię firstLineTest()i to wszystko, co robi.

Wygląda na wiele funkcji, ale brzmi praktycznie jak prosty angielski:

Aby przetworzyć plik, sprawdź, czy spełnia test. Jeśli tak, to zrób coś. W przeciwnym razie podejmij domyślną akcję. Plik spełnia test, jeśli istnieje, i pozytywnie przechodzi test zawartości. Aby przetestować zawartość, otwórz plik i przetestuj pierwszy wiersz. Test dla pierwszej linii ...

To prawda, że ​​to trochę nieporadne, ale należy pamiętać, że jeśli opiekun nie dba o szczegóły, może przestać czytać po zaledwie 4 liniach kodu processFile()i nadal będzie miał dobrą znajomość wysokiego poziomu na temat funkcji.

Karl Bielefeldt
źródło
5
+1 To dobra rada, ale to, co stanowi „jedną rzecz”, zależy od obecnej warstwy abstrakcji. processFile () to „jedna rzecz”, ale dwie rzeczy: fileMeetsTest () i doSomething () lub defaultAction (). Obawiam się, że aspekt „jednej rzeczy” może dezorientować początkujących, którzy z góry nie rozumieją tego pojęcia.
Caleb
1
To dobry cel ... To wszystko, co muszę o tym powiedzieć ... ;-)
Brian Knoblauch
1
Nie lubię pośrednio przekazywać argumentów jako takich zmiennych instancji. Dostajesz mnóstwo „bezużytecznych” zmiennych instancji i istnieje wiele sposobów na zarabianie na stanie i łamanie niezmienników.
hugomg,
@Caleb, ProcessFile () rzeczywiście robi jedną rzecz. Jak mówi Karl w swoim poście, używa testu, aby zdecydować, które działanie podjąć, i odkłada faktyczną implementację możliwości działania na inne metody. Gdyby dodać wiele innych alternatywnych akcji, kryteria jednego celu dla tej metody byłyby nadal spełnione, o ile w bezpośredniej metodzie nie występuje zagnieżdżenie logiki.
S.Robins,
6

W odniesieniu do tego, co to się nazywa, może łatwo rozwinąć się w anty-wzór grotu strzałki, gdy Twój kod rośnie, aby obsłużyć więcej wymagań (jak pokazano w odpowiedzi podanej na https://softwareengineering.stackexchange.com/a/122625/33922 ) i następnie wpada w pułapkę posiadania ogromnych sekcji kodów z zagnieżdżonymi instrukcjami warunkowymi przypominającymi strzałkę.

Zobacz linki takie jak;

http://codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://lostechies.com/chrismissal/2009/05/27/anti-patterns-and-worst-practices-the-arrowhead-anti-pattern/

W Google jest wiele innych informacji na temat tego i innych anty-wzorów.

Oto kilka świetnych wskazówek, które Jeff udziela na swoim blogu;

1) Zastąp warunki klauzulami ochronnymi.

2) Rozłóż bloki warunkowe na osobne funkcje.

3) Zamień kontrole ujemne na kontrole pozytywne

4) Zawsze oportunistycznie wracaj jak najszybciej z funkcji.

Zobacz także niektóre komentarze na blogu Jeffa dotyczące sugestii Steve'a McConnellsa dotyczących wczesnych powrotów;

„Użyj znaku powrotu, gdy poprawia to czytelność: w niektórych procedurach, gdy znasz odpowiedź, chcesz ją natychmiast przywrócić do procedury wywołującej. Jeśli procedura jest zdefiniowana w taki sposób, że nie wymaga żadnego dalszego czyszczenia wykrywa błąd, nie zwrócenie go natychmiast oznacza, że ​​musisz napisać więcej kodu. ”

...

„Zminimalizuj liczbę zwrotów w każdej procedurze: trudniej jest zrozumieć procedurę, gdy czytając ją na dole, nie jesteś świadomy możliwości, że powróciła gdzieś powyżej. Z tego powodu używaj zwrotów rozsądnie - tylko wtedy, gdy poprawią się czytelność ”.

Zawsze zapisywałem się na teorię 1 wejście / wyjście na funkcję ze względu na to, czego nauczono mnie 15 lat temu. Wydaje mi się, że dzięki temu kod jest o wiele łatwiejszy do odczytania i, jak wspomniałeś, łatwiejszy do utrzymania

Mr Moose
źródło
6

Jest to zgodne z zasadami DRY, no-goto i no-multiple-return, moim zdaniem jest skalowalne i czytelne:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
mouviciel
źródło
1
Zgodność ze standardami niekoniecznie oznacza dobry kod. Obecnie nie jestem zdecydowany na temat tego fragmentu kodu.
Brian Knoblauch
to po prostu zastępuje 2 defaultAction (); z 2 identycznymi jeśli warunki i dodaje zmienną flagową, która imo jest znacznie gorsza.
Ryathal
3
Zaletą korzystania z takiej konstrukcji jest to, że wraz ze wzrostem liczby testów kod nie zaczyna zagnieżdżać więcej ifs wewnątrz innych ifs. Ponadto kod do obsługi nieudanej instrukcji case ( DefaultAction()) znajduje się tylko w jednym miejscu i do celów debugowania kod nie przeskakuje funkcji pomocniczych, a dodawanie punktów przerwania do linii, w których successzmieniona jest zmienna, może szybko pokazać, które testy przeszły (powyżej wyzwalanego punkt przerwania), a które nie zostały przetestowane (poniżej).
frozenkoi
1
Tak, podoba mi się to, ale myślę, że successok_so_far
zmieniłbym
Jest to bardzo podobne do tego, co robię, gdy (1) proces jest bardzo liniowy, gdy wszystko idzie dobrze i (2) w przeciwnym razie miałbyś anty-wzór strzałki. Staram się jednak unikać dodawania dodatkowej zmiennej, co zwykle jest łatwe, jeśli myślisz o warunkach wstępnych do następnego kroku (co jest nieco inne niż pytanie, czy wcześniejszy krok nie powiódł się). Jeśli plik istnieje, otwórz go. Jeśli plik jest otwarty, przeczytaj zawartość. Jeśli mam treść, przetworz ją, w przeciwnym razie wykonaj domyślną akcję.
Adrian McCarthy
3

Wyodrębnię go do oddzielnej metody, a następnie:

if(!FileExists(file))
{
    DefaultAction();
    return;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}

DoSomething(contents);

co również pozwala

if(!FileExists(file))
{
    DefaultAction();
    return Result.FileNotFound;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return Result.TestFailed;
}

DoSomething(contents);
return Result.Success;            

następnie prawdopodobnie możesz usunąć DefaultActionpołączenia i pozostawić wykonanie DefaultActiondla dzwoniącego:

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

Lubię też podejście Jeanne Pindar .

Konrad Morawski
źródło
3

W tym konkretnym przypadku odpowiedź jest dość łatwa ...

Występuje warunek wyścigu pomiędzy FileExistsi OpenFile: co się stanie, jeśli plik zostanie usunięty?

Jedynym rozsądnym sposobem rozwiązania tego konkretnego przypadku jest pominięcie FileExists:

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

To starannie rozwiązuje ten problem i sprawia, że ​​kod jest czystszy.

Ogólnie: spróbuj przemyśleć problem i opracować inne rozwiązanie, które całkowicie go omija.

Martin Wickman
źródło
2

Inną możliwością, jeśli nie lubisz widzieć zbyt wielu innych, jest całkowite zrezygnowanie z użycia else i dodanie dodatkowej instrukcji return. W przeciwnym razie jest to zbyteczne, chyba że potrzebujesz bardziej złożonej logiki, aby ustalić, czy istnieją więcej niż dwie możliwości działania.

Zatem twój przykład może wyglądać następująco:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Osobiście nie mam nic przeciwko użyciu klauzuli else , która wyraźnie określa, w jaki sposób logika powinna działać, a zatem poprawia czytelność twojego kodu. Niektóre narzędzia upiększania kodu wolą jednak upraszczać od pojedynczej instrukcji if , aby zniechęcić do logiki zagnieżdżania.

S.Robins
źródło
2

Przypadek pokazany w przykładowym kodzie można zwykle sprowadzić do pojedynczej ifinstrukcji. W wielu systemach funkcja otwierania pliku zwróci niepoprawną wartość, jeśli plik jeszcze nie istnieje. Czasami jest to zachowanie domyślne; innym razem należy to określić za pomocą argumentu. Oznacza to, że FileExiststest można przerwać, co może również pomóc w warunkach wyścigu wynikających z usunięcia pliku między testem istnienia a otwarciem pliku.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

Nie rozwiązuje to bezpośrednio problemu mieszania poziomów abstrakcji, ponieważ całkowicie pomija kwestię wielu, niemożliwych do łańcuchowania testów, chociaż rezygnacja z testu istnienia pliku nie jest niezgodna z oddzielaniem poziomów abstrakcji. Zakładając, że nieprawidłowe uchwyty plików są równoważne „fałszowi”, a uchwyty plików zamykają się, gdy wykraczają poza zakres:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}
outis
źródło
2

Jednak zgadzam się z frozenkoi, jednak w przypadku C #, pomyślałem, że pomogłoby to postępować zgodnie ze składnią metod TryParse.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();
bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}
Biff MaGriff
źródło
1

Twój kod jest brzydki, ponieważ robisz za dużo w jednej funkcji. Chcesz przetworzyć plik lub podjąć domyślną akcję, więc zacznij od stwierdzenia, że:

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

Programiści Perla i Ruby piszą processFile(file) || defaultAction()

Teraz idź napisz ProcessFile:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;
Kevin Cline
źródło
1

Oczywiście możesz posunąć się tak daleko tylko w takich scenariuszach, ale jest na to sposób:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Możesz chcieć dodatkowe filtry. Następnie zrób to:

var appleFile = appleStorage.get(fileURI, isEdible);
//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Chociaż może to mieć również sens:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

Które jest najlepsze? To zależy od rzeczywistego problemu , przed którym stoisz.
Ale rzeczą, którą należy zabrać, jest: możesz wiele zrobić z kompozycją i polimorfizmem.

back2dos
źródło
1

Co jest nie tak z tym, co oczywiste

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

Wydaje mi się to dość standardowe? Dla tego rodzaju wielkiej procedury, w której musi wydarzyć się wiele drobiazgów, których żadna z nich nie zapobiegłaby. Wyjątki sprawiają, że jest to nieco czystsze, jeśli jest to opcja.

Steve Bennett
źródło
0

Wiem, że to stare pytanie, ale zauważyłem wzorzec, o którym nie wspomniano; głównie, ustawiając zmienną w celu późniejszego określenia metody, którą chcesz wywołać (poza if ... else ...).

Jest to kolejny punkt widzenia, aby ułatwić obsługę kodu. Pozwala również na określenie, kiedy można dodać kolejną metodę, która ma zostać wywołana, lub zmienić odpowiednią metodę, która musi zostać wywołana w określonych sytuacjach.

Zamiast wymieniać wszystkie wzmianki o metodzie (i być może brakuje niektórych scenariuszy), wszystkie są wymienione na końcu bloku if ... else ... i są łatwiejsze do odczytania i zmiany. Zazwyczaj używam tego, gdy na przykład można wywołać kilka metod, ale w zagnieżdżonym, jeśli ... w przeciwnym razie ... można wywołać metodę w kilku dopasowaniach.

Jeśli ustawisz zmienną, która definiuje stan, możesz mieć wiele głęboko zagnieżdżonych opcji i aktualizować stan, gdy coś ma zostać wykonane (lub nie).

Można to wykorzystać jak w przykładzie zadanym w pytaniu, w którym sprawdzamy, czy wystąpiło „DoSomething”, a jeśli nie, wykonaj domyślną akcję. Możesz też podać stan dla każdej metody, którą chcesz wywołać, ustawić, gdy ma to zastosowanie, a następnie wywołać odpowiednią metodę poza ...

Na końcu zagnieżdżonych instrukcji if ... else ... sprawdzasz stan i odpowiednio postępujesz. Oznacza to, że potrzebujesz tylko jednej wzmianki o metodzie zamiast wszystkich lokalizacji, w których powinna zostać zastosowana.

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}
Steve Padmore
źródło
0

Aby zmniejszyć zagnieżdżone JEŻELI:

1 / wczesny powrót;

2 / wyrażenie złożone (rozpoznawanie zwarcia)

Zatem twój przykład może zostać zmieniony w następujący sposób:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();
DQ_vn
źródło
0

Widziałem wiele przykładów z „return”, których też używam, ale czasami chcę uniknąć tworzenia nowych funkcji i zamiast tego użyć pętli:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

Jeśli chcesz pisać mniej linii lub nienawidzisz nieskończonych pętli tak jak ja, możesz zmienić typ pętli na „do ... while (0)” i uniknąć ostatniego „break”.

XzKto
źródło
0

Co z tym rozwiązaniem:

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

Przyjąłem założenie, że OpenFile zwraca wskaźnik, ale może to również działać ze zwrotem typu wartości poprzez określenie pewnej wartości domyślnej, która nie podlega zwrotowi (kody błędów lub coś podobnego).

Oczywiście nie oczekuję żadnych możliwych działań za pomocą metody SomeTest na wskaźniku NULL (ale nigdy nie wiadomo), więc może to być również postrzegane jako dodatkowe sprawdzenie wskaźnika NULL dla wywołania SomeTest (zawartość).

chedi
źródło
0

Najwyraźniej najbardziej eleganckim i zwięzłym rozwiązaniem jest użycie makra preprocesora.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

Co pozwala napisać piękny kod w ten sposób:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

Może być trudno polegać na automatycznym formatowaniu, jeśli często używasz tej techniki, a niektóre środowiska IDE mogą krzyczeć na ciebie trochę za to, co błędnie podejrzewa, że ​​jest zniekształcone. Jak mówi przysłowie, wszystko jest kompromisem, ale przypuszczam, że nie jest to zła cena, aby uniknąć zła wynikającego z powtarzania się kodu.

Peter Olson
źródło
Dla niektórych osób i w niektórych językach makra preprocesora złym kodem :)
Benjol
@Benjol Powiedziałeś, że jesteś otwarty na złe sugestie, nie? ;)
Peter Olson
tak, absolutnie, to było po prostu wasze „unikanie zła” :)
Benjol
4
To jest tak okropne, że musiałem je głosować: D
back2dos
Shirley, nie mówisz poważnie !!!!!!
Jim In Texas
-1

Ponieważ zadałeś pytanie z ciekawości, a twoje pytanie nie jest oznaczone określonym językiem (chociaż jasne jest, że masz na myśli języki imperatywne), warto dodać, że języki obsługujące leniwe ocenianie pozwalają na zupełnie inne podejście. W tych językach wyrażenia są oceniane tylko wtedy, gdy są potrzebne, więc można zdefiniować „zmienne” i używać ich tylko wtedy, gdy ma to sens. Na przykład w fikcyjnym języku z leniwymi let/ instrukturami zapominasz o kontroli przepływu i piszesz:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()
tokland
źródło