Zwraca wartość logiczną set.add () w warunkowym?

15

Operator add klasy set zwraca wartość logiczną, która jest prawdą, jeśli element (który ma zostać dodany) jeszcze nie istniał, a fałsz w przeciwnym razie. Pisze

if (set.add(entry)) {
    //do some more stuff
}

uważany za dobry styl pod względem pisania czystego kodu? Zastanawiam się, skoro robicie dwie rzeczy naraz. 1) dodanie elementu i 2) sprawdzenie, czy element istniał.

java coding-style clean-code

— Andreas Braun
źródło

6

Mówisz o standardzie java.util.Set, który powraca, addgdy element nie był jeszcze obecny, prawda?

— użytkownik2357112 obsługuje Monikę

1

Zwykle rozważałbym test przeciwny:if (!set.add(entry)) {// entry already present, possibly a case you want to handle}

— njzk2,

Czy jest coś nie tak z robieniem dwóch rzeczy naraz?

— user207421

@ user2357112 tak, zgadza się.

— Andreas Braun,

19

Tak to jest.

Zazwyczaj operacja zwraca wartość logiczną, ponieważ można jej użyć do podejmowania decyzji, tj. W obrębie ifkonstrukcji. Jedynym innym sposobem, w jaki mógłbyś zrealizować ten potencjał, byłoby przechowywanie wartości zwracanej w zmiennej, a następnie natychmiastowe użycie jej w zmiennej if, która jest po prostu głupia i na pewno nie jest lepsza od pisania if(operation()) { ... }. Więc po prostu idź i zrób to, nie będziemy cię oceniać (za to).

— Kilian Foth
źródło

Dziękuję za odpowiedź. Jak wskazuje @Niels vanReijmersdal w swojej odpowiedzi, zrobienie tego przerywa rozdzielanie poleceń i zapytań, prawda? Nie uważasz, że to ma znaczenie?

— Andreas Braun,

4

@AndreasBraun osobiście uważam, że rozdzielanie zapytań jest głupie. Często logiczne jest, aby jedna metoda zarówno wykonywała akcję, jak i zwracała wartość związaną z tą akcją. Egzekwowanie sztucznego rozdzielenia między tymi dwoma kodami prowadzi do niepotrzebnie pełnego kodu.

1

@ dan1111: rzeczywiście. Ponadto, „rozdzielanie poleceń i zapytań” prowadzi do „sprawdzania, a następnie działania” i podobnych anty-wzorców, które mogą pękać w kontekstach wielowątkowych. To dość dziwne, aby reklamować taką separację, gdy reszta świata stara się budować atomowych skondensowane operacje solidnych i skutecznych rozwiązań SMP w tym samym czasie ...

— Holger

2

@ Jörg W Mittag: strona 759 czego? Ponieważ operacja atomowa jest wewnętrznie odporna na anty-wzorzec „sprawdź, a następnie działaj”, podzielenie go nie wystarczy, aby przeczytać „cały rozdział” czegokolwiek, aby dowiedzieć się, jak zapewnić bezpieczeństwo wątku konstrukcyjnego jeszcze raz. Ponieważ nie podałeś żadnych faktycznych argumentów, nie mam „konkretnych zastrzeżeń do tych argumentów”.

— Holger,

1

@ Jörg W Mittag: Cóż, mówię o odpowiedzi na tej stronie, zniechęcając do używania Set.addjako pojedynczej operacji bez nazywania alternatywy. Jeśli zamierzoną operacją jest dodanie pojedynczego elementu i sprawdzenie, czy został on dodany, nie ma potrzeby stosowania bardziej wydajnej alternatywy, która nadal komplikuje operację bez korzyści. Mam nadzieję, że wiesz, że Set.addjest to wbudowana metoda JRE, której można używać bez wcześniejszego zapoznania się z> 700 stronicową książką.

— Holger,

12

Powiedziałbym, że nie jest tak czysty, jak to możliwe, ponieważ zmusza opiekuna albo już wie, albo sprawdza, co oznacza wartość zwracana. Czy to oznacza, że wartość już istniała, jeszcze nie istniała, została pomyślnie wstawiona? Jeśli nie użyjesz go dużo, nie będziesz wiedział, a nawet jeśli to zrobisz, będzie to o wiele więcej obciążenia psychicznego.

Wolałbym następujące:

boolean added = set.add(entry);

if (added) {
    //do some more stuff
}

Tak, nieco bardziej szczegółowe, ale kompilator powinien generować dokładnie ten sam kod bajtowy, a nawet ludzie, którzy nie używali zestawów Java od lat, mogą podążać za logiką, nie szukając niczego.

— Karl Bielefeldt
źródło

2

addzwraca true, jeśli elementu już tam nie było, a nie, jeśli tak było. (Brzmienie pytania jest mylące.)

— user2357112 obsługuje Monikę

9

Wyszukanie metody jest mniej obciążeniem niż próba zrozumienia zamiaru pierwotnego dewelopera dla nadmiarowej zmiennej zadeklarowanej poza zakresem, w którym jest używana. We wszystkich współczesnych IDE Java programista może najechać kursorem myszy na metodę i natychmiast uzyskać dostęp do jej dokumentacji. Dobrzy programiści robią to stale, pracując z dowolnym nieznanym API.

— Kevin Krumwiede,

9

I drugi @ Holger. Jeśli nie wiesz Set.add, jesteś niedoświadczony i powinieneś szukać tych metod, aby się ich nauczyć i stać się bardziej doświadczonym.

— Przywróć Monikę

7

notAlreadyPresentnie jest najlepszym sformułowaniem. Użyłbym addedi oczekuję, że czytelnik będzie wiedział, dlaczego wartość nie zostanie dodana do zestawu.

— njzk2

3

@JustinTime: Używanie komentarzy do opisywania tego, co robi kod, jest powszechnie uważane za złą praktykę. Twój kod powinien być samoopisujący. W recenzji kodu oznaczam twój przykład jako niedopuszczalny.

— BlueRaja - Danny Pflughoeft

8

Jeśli prawda oznacza sukces, to jest to dobry, czytelny kod.

Istnieje powszechna konwencja, że funkcja lub metoda zwraca true (lub coś, co daje wartość true) po sukcesie. Tak długo, jak podąża za tym twój kod, myślę, że umieszczenie metody w warunku jest w porządku.

Moim zdaniem taki kod jest niepotrzebnie zaśmiecony:

boolean frobulate_succeeded = thing.frobulate();

if (frobulate_succeeded) {
    ...
}

Czujesz się, jakbyś się powtarzał.

Pytanie jest jednak dwuznaczne co do znaczenia zwracanej wartości. Mówisz „boolean, który wskazuje, czy dodany element już istniał”, co może sugerować, że prawda oznacza, że element istniał (i dodawanie nie nastąpiło). W takim przypadku idealnie zmieniłbym zachowanie zwrotne metody na bardziej konwencjonalne. Jeśli nie jest to możliwe, dodałbym dodatkową zmienną pośrednią, która pozwala wyraźnie oznaczyć wynik w kodzie (jak sugerują inni).

Co oznacza sukces w kontekście dodawania elementu do zestawu? Brak zgłoszenia wyjątku oznacza sukces; prawda vs. fałsz oznacza w tym kontekście coś bardziej szczegółowego.

— Przywróć Monikę

@ Solomonoff'sSecret W tym przypadku wyjątek oznacza, że zestaw odmówił dodania elementu, falseoznacza, że element nie został dodany, ponieważ był już zawarty, i trueoznacza, że element nie był już zawarty. Gdy add()dodaje element do zestawu, jeśli zestaw jeszcze go nie zawierał, trueoznacza to, że element został pomyślnie dodany do zestawu.

— Justin Time - Przywróć Monikę

@JustinTime Dodajesz własne oceny wartości do dwóch przypadków. Inną interpretacją jest to, że sukces polega na tym, że element jest w zestawie, gdy metoda powraca. Jeśli przedmiot był już w zestawie, addkończy się sukcesem bez potrzeby robienia czegokolwiek. Jeśli element nie był jeszcze w zestawie, addsukces polega na dodaniu elementu do zestawu. Która interpretacja jest poprawna, jest arbitralna. Mniej arbitralna definicja sukcesu pochodzi z języka: metoda odnosi sukces, jeśli zwraca normalnie.

— Przywróć Monikę

1

Najmniej arbitralną definicją sukcesu jest to, że metoda pomyślnie wykonała zadanie, które wyznaczyła do wykonania. Jeśli mam metodę firstLessThanSecond(int l, int r)i zwraca ona, truejeśli l > rlub falsejeśli l <= r, to metoda ta nie jest skuteczna, mimo że zwraca normalnie.

— Justin Time - Przywróć Monikę

1

@JustinTime addjest surowym skrótem umowy. Dokumentacja zaczyna się od „Dodaje określony element do tego zestawu, jeśli nie jest już obecny”, co jest wystarczające, niezależnie od tego, czy element był już w zestawie. Możesz dowolnie interpretować zwracaną wartość, ale ostatecznie jest to tylko twoja interpretacja. W drugim przykładzie metoda ocenia, czy warunek jest spełniony. Jeśli warunek jest fałszywy, metoda z pewnością nie zawiodła, ponieważ pomyślnie oceniła warunek.

— Przywróć Monikę

2

Powiedziałbym, że jest bardzo podobny do C. Przez większość czasu wolałbym mieć opisowo nazwaną zmienną dla wyniku mutacji i żadna mutacja nie zachodzi w ifstanie.

Kompilator wyeliminuje tę zmienną, jeśli zostanie natychmiast ponownie wykorzystana. Ludziom łatwiej będzie czytać źródło; dla mnie jest to ważniejsze.

Gdyby ktoś musiał rozszerzyć warunek poprzez dodanie klauzuli and/ ordo warunku if, może .add()w niektórych przypadkach nie zadzwonić z powodu oceny zwarcia. O ile nie przewiduje się zwarcia, może to być błąd.

— 9000
źródło

Jeśli piszę, if (set.contains(entry)){set.add(entry); //do more stuff}czy to również jest eliminowane przez kompilator?

— Andreas Braun,

1

@AndreasBraun: Nie sądzę; kompilator nie ma pojęcia o semantycznym powiązaniu między containsi add. Ponadto podwójnie sprawdza się entryw zestawie; może to odgrywać rolę w przypadku bardzo dużych zestawów i bardzo lekkich pętli.

— 9000

1

Rozumiem więc, że wolałbyś nie pisać, if (set.contains(entry)){set.add(entry); //do more stuff}a zamiast tego iść np. Z odpowiedzią Karla Bielefeldta?

— Andreas Braun,

@AndreasBraun: dokładnie (pozytywnie ocenił tę odpowiedź).

— 9000

1

dobry punkt. Mutacje w ifs są trudne, ponieważ przyszły twórca może mieć inne warunki ze zwarciami.

— njzk2

0

Wygląda na to, że Twój kod przerywa rozdzielanie zapytań poleceń . Jest to omówione w książce Clean Code i wideo Struktury funkcji . Więc z perspektywy Clean Code myślę, że nie jest uważany za dobry styl.

„Czysty kod jest prosty i bezpośredni. Czysty kod czyta się jak dobrze napisana proza. Czysty kod nigdy nie przesłania intencji projektanta, ale jest pełen wyraźnych abstrakcji i prostych linii kontroli.

- Grady Booch autor Object Oriented Analysis and Design with Applications ”

Dla mnie cel twojego kodu jest niejasny. Czy oba są wykonywane, gdy pozycja jest dodawana, czy też już istnieje? Co add()zwraca przedmiot? Kod błędu?

— Niels van Reijmersdal
źródło

2

Tak, tak też myślałem. Ale myślę też, że @Kilian Foth ma rację. Gdybym chciał rozdzielić zapytania i polecenia, musiałbym napisać, if (set.contains(entry)){set.add(entry); //do more stuff}co wydaje się trochę głupie. Jakie jest twoje zdanie na ten temat?

— Andreas Braun,

Nie znam domeny i kontekstu tego kodu, aby zasugerować dobrą nazwę, ale chciałbym zawrzeć to w funkcji z wyraźną intencją. Na przykład: doesEntryExistOrCreateEntry (pozycja), może zawierać logikę zawierającą () + add () i zwracać wartość logiczną. Podobne pytanie tutaj: softwareengineering.stackexchange.com/questions/149708/..., który omawia tę praktykę poza czystym kodem.

— Niels van Reijmersdal

6

Myślę, że reguła rozdzielania zapytań jest głupia, szczególnie w przypadku takiego przypadku, w którym metoda zarówno wykonuje akcję, jak i zwraca stan powodzenia akcji. Ale także nie wyjaśniasz, czym jest reguła, ani nie podajesz jej w uzasadnieniu. Z tych powodów doceniono.

1

„Co zwraca funkcja add ()?” Oczekuję, że każdy programista Java, który dokonuje przeglądu kodu, zna CollectionAPI.

— njzk2

3

Uzgodniony z @ dan1111. Jest to szczególnie głupie, ponieważ jest to rodzaj rzeczy, które tworzą żyzny grunt dla warunków rasowych.

— Paul Draper,