Przypisanie wartości logicznej najlepszych praktyk [zamknięte]

W programie przejętym od innego programisty natknąłem się na następujące warunki:

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.NeedsChange = false;
}

Uważam, że ten kod jest zbędny i brzydki, więc zmieniłem go na coś, co moim zdaniem było prostym zadaniem logicznym opartym na porównaniu:

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE;

Widząc to, ktoś sprawdzający mój kod skomentował, że chociaż moja zmiana jest funkcjonalnie poprawna, może to mylić kogoś innego, kto na nią patrzy. Uważa, że ​​użycie operatora trójskładnikowego czyni to zadanie bardziej zrozumiałym, podczas gdy nie podoba mi się dodanie bardziej redundantnego kodu:

obj.NeedsChange = (obj.Performance <= LOW_PERFORMANCE) ? true : false;

Jego rozumowanie jest takie, że robienie czegoś w najbardziej zwięzły sposób nie jest tego warte, jeśli powoduje to, że inny programista musi się zatrzymać i zastanowić się, co dokładnie zrobiłeś.

Prawdziwe pytanie brzmi: która z tych trzech metod przypisywania wartości do wartości logicznej obj.NeedsChangejest najbardziej przejrzysta i najłatwiejsza do utrzymania?

Wolę 2, ale może pójdę na drobną korektę:

obj.NeedsChange = ( obj.Performance <= LOW_PERFORMANCE );

Dla mnie nawiasy ułatwiają parsowanie linii i na pierwszy rzut oka widać, że przypisujesz wynik porównania, a nie podwójnego przypisania. Nie jestem pewien, dlaczego tak jest (ponieważ nie mogę wymyślić języka, w którym nawiasy faktycznie zapobiegałyby podwójnemu przypisaniu), ale jeśli musisz zadowolić swojego recenzenta, być może będzie to akceptowalny kompromis.

Wariant 1 jest łatwy do zrozumienia, ale to jego jedyna zaleta. Automatycznie zakładam, że każdy, kto pisze w ten sposób, tak naprawdę nie rozumie, o co chodzi w booleanach i pod wieloma innymi względami będzie pisać podobnie infantylny kod.

Wariant 2 jest tym, co zawsze bym pisał i spodziewałem się przeczytać. Myślę, że każdy, kto jest zdezorientowany tym idiomem, nie powinien być profesjonalnym pisarzem oprogramowania.

Wariant 3 łączy wady zarówno 1, jak i 2.

Kod w dowolnym momencie jest bardziej skomplikowany niż wymaga uruchomienia „co to ma robić?” zapach w czytniku.

Na przykład pierwszy przykład sprawia, że ​​zastanawiam się: „czy w instrukcji if / else była już inna funkcjonalność w pewnym momencie, który został usunięty?”

Przykład (2) jest prosty, jasny i robi dokładnie to, co jest potrzebne. Przeczytałem go i od razu zrozumiałem, co robi kod.

Dodatkowy puch w (3) spowodowałby, że zastanawiałem się, dlaczego autor napisał to w ten sposób zamiast (2). Nie powinno być powodem, ale w tym przypadku nie wydaje się być, więc to nie jest w ogóle pomocny i trudniejsze do odczytania, ponieważ składnia sugeruje coś prezent, który nie istnieje. Próba nauczenia się, co jest obecne (kiedy nic nie jest), utrudnia czytanie kodu.

Łatwo zauważyć, że Wariant 2 i Wariant 1 są powiązane poprzez szereg oczywistych i prostych refaktoryzacji:

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.NeedsChange = false;
}

Tutaj mamy niepotrzebne powielanie kodu, możemy wyróżnić przypisanie:

obj.NeedsChange = if (obj.Performance <= LOW_PERFORMANCE)
{
    true
}
else
{
    false
}

lub napisane bardziej zwięźle:

obj.NeedsChange = if (obj.Performance <= LOW_PERFORMANCE) true else false

Teraz powinno być od razu oczywiste, że to przypisze true, jeśli warunek jest prawdziwy, i false, jeśli warunek jest false, IOW po prostu przypisze wartość warunku, tzn. Jest równoważny

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE

Warianty 1 i 3 są typowym kodem nowicjusza napisanym przez kogoś, kto nie rozumie, jaka jest wartość zwracana porównania.

Chociaż programowanie powinno zmierzać w kierunku wyraźnego nad niejawna „jakby facet, który kończy się utrzymując swój kod będzie brutalny psychopata, który wie, gdzie mieszkasz”, ty może przyjąć kilka podstawowych rzeczy , że psycho następca będzie właściwy w.

Jednym z nich jest składnia języka, którego używa.

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE;

jest bardzo jasne dla każdego, kto zna składnię C / C ++ / C # / Java / JavaScript.

Jest także znacznie bardziej czytelny niż 8 linii.

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.NeedsChange = false;
}

i mniej podatne na błędy

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.Needschange = false;
}

Jest to lepsze niż dodawanie niepotrzebnych znaków, jakbyś na wpół zapomniał składni języka:

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE ? true : false;

obj.NeedsChange = (obj.Performance <= LOW_PERFORMANCE);

Myślę, że jest wiele błędów w składni podobnej do C w wielu językach: niespójna kolejność operacji, niespójna asocjacja lewo / prawo, przeciążone użycie symboli, nawiasy klamrowe / wcięcia, operatory trójskładnikowe, notacja infiksowa itp.

Ale rozwiązaniem nie jest wymyślenie własnej, zastrzeżonej wersji. W ten sposób leży szaleństwo, ponieważ każdy tworzy swój własny.

Zasadniczo najważniejszą rzeczą, która sprawia, że kod Real World ^TM jest nieczytelny, jest jego ilość.

Między niepatologicznym programem z 200 liniami a trywialnie identycznym programem z 1600 liniami, ten krótszy prawie zawsze łatwiej będzie przeanalizować i zrozumieć. Z radością powitałbym Twoją zmianę każdego dnia.

Większość programistów będzie w stanie zrozumieć drugą formę na pierwszy rzut oka. Moim zdaniem uproszczenie w pierwszej formie jest po prostu niepotrzebne.

Możesz poprawić czytelność, dodając spacje i nawiasy klamrowe, takie jak:

obj.NeedsChange =    obj.Performance <= LOW_PERFORMANCE;

lub

obj.NeedsChange = ( obj.Performance <= LOW_PERFORMANCE );

jak wspomniał Jacob Raihle.