Czytelność: pętle + warunki

Hejże, hejże! :)

Którą wersję kodu preferujecie i dlaczego?

Pierwsza:

foreach (tablicaCzegośtam as cośtam) {
	if (cośtam.isProsiaczek()) {
		// powiedzmy, że tutaj przychodzi 10 linijek kodu
	}
}

Druga:

foreach (tablicaCzegośtam as cośtam) {
	if (!cośtam.isProsiaczek()) {
		continue;
	}
	
	// powiedzmy, że tutaj przychodzi 10 linijek kodu
}

zakładając, że nie można dokonać innej transformacji tego kodu (np. filtrując dane przed pętlą)

Ja od zarania dziejów wykorzystuję wersję drugą, bo choć nieco dłuższa, to wydaje mi się bardziej czytelna, poza tym okropnie nie lubię zagnieżdżania bloków kodu w sobie... natomiast nigdy jakoś nie zwracałem większej uwagi, jak mogą to postrzegać inni, dlatego też dzisiaj tak zaczęło mnie to trapić i postanowiłem dopytać ;-) (zwłaszcza że widuję w kodach na przemian obydwie wersje)

Wracając więc do pytania: która wersja kodu i dlaczego?

Pierwsza bo wersje ze skokiem zawsze są mniej czytelne. Bardzo łatwo takie coś przeoczyć i potem myślisz że pętla wykonuje się zawsze.

Zdecydowanie 1, wydaje mi się bardziej czytelna. IMO Zagnieżdżone bloki nie są problemem, o ile nie jest ich za dużo.

Trzecia:

foreach (tablicaCzegośtam as cośtam) {
    if (cośtam.isProsiaczek()) {
        zróbMielone(cośtam);
    }
}

ja pierwsza ale z troszke innym ukladem

foreach (tablicaCzegośtam as cośtam) {
    bool isProsiaczek = costam.isProsiaczek();
    if (isProsiaczek) {
        // i zamiast 10 linijek to tutaj funkcja jak juz @somekind pokazal
    }
} 

nie lubie wykonywac funkcji w warunkach jezeli da sie to ominac.

foreach (tablicaCzegośtam as cośtam) {
bool isProsiaczek = costam.isProsiaczek();
if (isProsiaczek) {
// i zamiast 10 linijek to tutaj funkcja jak juz @somekind pokazal
}
}

nie lubie wykonywac funkcji w warunkach jezeli da sie to ominac.

Myślałem, że to typowy antypattern. Tworzysz zmienną z za dużym scopem, która dodatkowo jeśli jest deklarowana z dala od warunku mogła być (przypadkowo lub i nie) zmieniona, a jeśli jest tuż nad warunkiem, to w zasadzie nie wiem w jaki sposób miałoby to być czytelniejsze.

@Lectre

zmienna deklaruje przed tylko i wylacznie przed ifem by nie wykonywac w ifie "ciezkich" operacji. Jezeli if ma spelniac kilka warunkow to wole debugowac i czytac cos takiego

bool isProsiaczek = costam.isProsiaczek();
bool isTrujace = costam.isTrujace();
bool isSmaczne = costam.isSmaczne();
if (isProsiaczek && !isTrujace && isSmaczne)  

niz cos takiego
if(costam.isProsiaczek() && !costam.isTrujace() && costam.isSmaczne())

duzo latwiej (przynajmniej mi) debuguje sie opcje pierwsza niz druga.

(te nazwy polsko angielskie sa straszne :D)

Dla mnie obie są podobne, ale tak czy siak zamknąłbym ten kod w metodę.
Możliwe opcje:

• To co napisał @somekind
• Jeśli jest więcej zmiennych to wydzielenie ich do osobnej metody, w środku można je rozbić tak jak to zrobił fasadin, ale niekoniecznie jeśli są np dwie (zawsze mogę użyć watcha). Im ich jest jednak więcej tym większy nacisk bym położył na rozbicie i nadanie stosownych nazw. Dla czytelności. Rozbicia nie uznałbym nigdy za coś złego, ale jeśli mamy ładnie nazwaną metodę obiekt.IsCostam(), to robienie dla niej jednego boola isCostam to trochę overkill.
• Opcja druga z postu, ale całość zamknięta w metodę, i ten guard na początku tej metody. Wtedy wygląda to lepiej niż w środku pętli.

foreach (tablicaCzegośtam as cośtam) {
    DoCostamWith(costam);
}
private void DoCostamWith( Costam costam)
{
    if (!cośtam.isProsiaczek()) {
        return;
    }
 
    // powiedzmy, że tutaj przychodzi 10 linijek kodu
}

• Jeśli możesz (może się nie dać bo wymagania innych obiektów do współpracy, czy ze względu na ten warunek IsCostam), to jeszcze bardziej obiektowe podejście:

foreach (tablicaCzegośtam as cośtam) {
    costam.DoSomething();//I tutaj całe zachowanie obiektu, sprawdzenie czy jest prosiaczkiem itd
}

Problem nie istnieje.

for (cośtam <- tablicaCzegośtam if cośtam.isProsiaczek) {
  // powiedzmy, że tutaj przychodzi 10 linijek kodu
}

No zawsze można przecież zrobić filter->map albo filter->reduce ;)

Podobnie jak powiedział @Krolik. Użył bym iteratora, który przefiltrował by mi wszystko. Przykład z Rusta:

for costam in tablicaCzegostam.filter(Czegostam::is_prosiaczek) {
    // tutaj 10 linijek kodu
}

lub z Ruby

tablicaCzegostam.filter(&:prosiaczek?).each do |costam|
  # tutaj 10 linijek kodu
end

Ok, teraz mi się trochę już światopogląd rozszerzył ;)

1. @somekind @AreQrm - wydzielanie do metody jest najlepszą wersją, jeśli kod zaczyna (lub może) niebezpiecznie się rozrosnąć, natomiast tutaj mi chodziło o taki przypadek, gdy jeszcze nie jest to opłacalne. Z tej perspektywy już dostrzegam, że może faktycznie dopisek, że tam przychodzi 10 linijek kodu nie ułatwił przekazania tej myśli w czysty sposób :D
2. @fasadin - takiego kombo jeszcze nie miałem okazji widzieć w kodzie; wydaje mi się, że w tym stopniu skomplikowane warunki nadają się już na wydzielenie do osobnej metody niż robienie kilku zmiennych lokalnych, nie sądzisz?
3. @Krolik @Shalom @winerfresh - ciekawa opcja, gdy język wspiera takie konstrukcje. Muszę przyznać: pierwszy raz widziałem tak zbudowaną pętlę, ale np. w PHPie takich bajerów nie ma niestety ;c (tzn. pewnie jakieś filter są, albo w każdym razie można by napisać, ale na pewno nie wyglądałoby to tak przejrzyście)

Nikt już nie zjeżdża klamrą pod instrukcje? :S

 if(bla == blabla)
{
            //code
}

Według mnie druga opcja jest lepsza w celu uniknięcia wielu zagnieżdżeń (wiele zagnieżdżeń jest zdecydowanie mniej czytelna niż przeskok). Natomiast w tak prostych przykładach opcja pierwsza jest lepsza.

@bonifacy bez sensu, zagnieżdżenia wyciągasz i tak do osobnych metod żeby mieć max 1 poziom pętli i 1 warunek w metodzie.