Problem ze stylem kodowania: czy powinniśmy mieć funkcje, które pobierają parametr, modyfikują go, a następnie POWRÓT ten parametr?

19

Mam trochę dyskusji z przyjacielem, czy te dwie praktyki są tylko dwiema stronami tej samej monety, czy też jedna z nich jest naprawdę lepsza.

Mamy funkcję, która pobiera parametr, wypełnia jego element członkowski, a następnie zwraca go:

Item predictPrice(Item item)

Uważam, że ponieważ działa na tym samym przekazywanym obiekcie, nie ma sensu zwracać przedmiotu. W rzeczywistości, jeśli cokolwiek, z perspektywy dzwoniącego, myli sprawy, ponieważ można oczekiwać, że zwróci nowy przedmiot, którego nie ma.

Twierdzi, że to nie robi różnicy, i nie miałoby to nawet znaczenia, gdyby stworzył nowy przedmiot i zwrócił go. Zdecydowanie się nie zgadzam z następujących powodów:

  • Jeśli masz wiele odniesień do przekazywanych elementów (lub wskaźników lub cokolwiek innego), przydzielenie nowego obiektu i zwrócenie go ma istotne znaczenie, ponieważ odniesienia te będą niepoprawne.

  • W językach, które nie są zarządzane, funkcja alokująca nową instancję twierdzi, że jest właścicielem pamięci, a zatem musielibyśmy zaimplementować metodę czyszczenia, która jest w pewnym momencie wywoływana.

  • Przydział na stercie jest potencjalnie drogi, dlatego ważne jest, czy wywoływana funkcja to robi.

Dlatego uważam, że bardzo ważne jest, aby móc zobaczyć za pomocą sygnatury metod, czy modyfikuje obiekt, czy przydziela nowy. W rezultacie uważam, że ponieważ funkcja modyfikuje jedynie przekazywany obiekt, sygnatura powinna być:

void predictPrice(Item item)

W każdej bazie kodu (co prawda bazach C i C ++, nie Java, w której pracujemy), z którymi pracowałem, powyższy styl był zasadniczo przestrzegany i utrzymywany przez znacznie bardziej doświadczonych programistów. Twierdzi, że ponieważ moja próbka baz kodów i współpracowników jest niewielka ze wszystkich możliwych baz kodów i współpracowników, a zatem moje doświadczenie nie jest prawdziwym wskaźnikiem tego, czy ktoś jest lepszy.

Jakieś myśli?

Squimmy
źródło
strcat modyfikuje parametr na miejscu i nadal go zwraca. Czy strcat powinien zostać anulowany?
Jerry Jeremiah
Java i C ++ mają bardzo różne zachowania w zakresie przekazywania parametrów. Jeśli Itemjest class Item ...i nie typedef ...& Item, lokalny itemjest już kopią
Caleth
google.github.io/styleguide/cppguide.html#Output_Parameters Google woli używać wartości RETURN dla danych wyjściowych
Firegun

Odpowiedzi:

26

To naprawdę jest kwestia opinii, ale ze względu na to, co warto, zwrócenie zmodyfikowanego przedmiotu jest mylące, jeśli przedmiot został zmodyfikowany na miejscu. Osobno, jeśli predictPricezamierza zmodyfikować przedmiot, powinien on mieć nazwę wskazującą, że to zrobi (podobnie jak setPredictPriceniektóre inne).

Wolałbym (w kolejności)

  1. To predictPricebyła metodaItem (modulo dobry powód, aby tego nie było, co może być w twoim ogólnym projekcie), która albo

    • Zwrócono przewidywaną cenę lub

    • Miał setPredictedPricezamiast tego imię

  2. To predictPrice się nie zmieniło Item, ale zwróciło przewidywaną cenę

  3. To predictPricebyła voidmetoda o nazwiesetPredictedPrice

  4. To predictPricezwróciło this(dla łączenia łańcuchowego z innymi metodami w dowolnej instancji, której jest częścią) (i zostało wywołane setPredictedPrice)

TJ Crowder
źródło
1
Dziękuję za odpowiedź. W odniesieniu do 1 nie jesteśmy w stanie przewidzieć ceny wewnątrz przedmiotu. 2 to dobra sugestia - myślę, że to chyba poprawne rozwiązanie.
Squimmy,
2
@Squimmy: I rzeczywiście, spędziłem 15 minut na rozwiązywaniu problemu spowodowanego przez kogoś używającego dokładnie tego, o co się kłóciłeś: a = doSomethingWith(b) zmodyfikowałem b i zwróciłem go wraz z modyfikacjami, które wysadziły mój kod później, myśląc, że wie, co bmiało w tym. :-)
TJ Crowder
11

W paradygmacie projektowania obiektowego rzeczy nie powinny modyfikować obiektów poza samym obiektem. Wszelkie zmiany stanu obiektu należy wprowadzać metodami na obiekcie.

Zatem void predictPrice(Item item)jako funkcja członka innej klasy jest niepoprawna . Mogło to być akceptowalne w czasach C, ale w Javie i C ++ modyfikacje obiektu implikują głębsze sprzężenie z obiektem, co prawdopodobnie prowadziłoby do innych problemów projektowych na późniejszym etapie (po zmianie klasy i zmianie jej pól, teraz należy zmienić na „replacePrice” w innym pliku.

Zwracając nowy obiekt, nie ma powiązanych skutków ubocznych, przekazany parametr nie ulega zmianie. Ty (metoda przewidywana) nie wiesz, gdzie jeszcze ten parametr jest używany. Czy Itemgdzieś jest klucz do skrótu? czy robiąc to, zmieniłeś kod skrótu? Czy ktoś inny trzyma się tego, oczekując, że się nie zmieni?

Te problemy projektowe zdecydowanie sugerują, że nie powinieneś modyfikować obiektów (w wielu przypadkach argumentowałbym za niezmiennością), a jeśli tak, zmiany stanu powinny być ograniczone i kontrolowane przez sam obiekt, a nie coś jeszcze poza klasą.


Przyjrzyjmy się, co się stanie, jeśli ktoś bawi się polami czegoś w haszu. Weźmy trochę kodu:

import java.util.*;

public class Main {
    public static void main (String[] args) {
        Set set = new HashSet();
        Data d = new Data(1,"foo");
        set.add(d);
        set.add(new Data(2,"bar"));
        System.out.println(set.contains(d));
        d.field1 = 2;
        System.out.println(set.contains(d));
    }

    public static class Data {
        int field1;
        String field2;

        Data(int f1, String f2) {
            field1 = f1;
            field2 = f2;
        }

        public int hashCode() {
            return field2.hashCode() + field1;
        }

        public boolean equals(Object o) {
            if(!(o instanceof Data)) return false;
            Data od = (Data)o;
            return od.field1 == this.field1 && od.field2.equals(this.field2);

        }
    }
}

ideone

Przyznaję, że nie jest to najlepszy kod (bezpośredni dostęp do pól), ale służy on zademonstrowaniu problemu ze zmiennymi danymi używanymi jako klucz do HashMap, lub w tym przypadku po prostu umieszczanym w HashSet.

Dane wyjściowe tego kodu to:

true
false

Stało się tak, że hashCode użyty podczas wstawiania jest tam, gdzie obiekt znajduje się w haszu. Zmiana wartości użytych do obliczenia hashCode nie powoduje ponownego obliczenia samego skrótu. Istnieje niebezpieczeństwo, że dowolny zmienny obiekt będzie kluczem do skrótu.

Wracając do pierwotnego aspektu pytania, wywoływana metoda nie „wie”, w jaki sposób używany jest obiekt, który otrzymuje jako parametr. Podanie „pozwala mutować obiekt” jako jedyny sposób na to oznacza, że ​​istnieje wiele subtelnych błędów, które mogą się wkraść. Jak utrata wartości w haszu ... chyba że dodasz więcej i hasz zostanie zmieniony - zaufaj mi , to paskudny błąd do wyśledzenia (straciłem wartość, dopóki nie dodałem 20 kolejnych elementów do mapy hashMap, a potem nagle pojawia się ponownie).

  • O ile nie ma bardzo dobrego powodu, aby zmodyfikować obiekt, zwrócenie nowego obiektu jest najbezpieczniejsze.
  • Gdy nie jest to dobry powód do modyfikacji obiektu, że modyfikacje powinny być wykonywane przez sam (Methods powołując) przedmiotu, a nie przez jakiś zewnętrzny funkcji, która może kręcić jego pola.
    • Umożliwia to refaktoryzację obiektu przy niższych kosztach utrzymania.
    • Pozwala to obiektowi upewnić się, że wartości obliczające jego kod skrótu nie zmieniają się (lub nie są częścią jego obliczeń)

Powiązane: Nadpisywanie i zwracanie wartości argumentu użytego jako warunek instrukcji if, wewnątrz tej samej instrukcji if

Społeczność
źródło
3
To nie brzmi dobrze. Najprawdopodobniej predictPricenie powinno się bezpośrednio bawić z Itemczłonkami, ale co jest złego w wywoływaniu takich metod Item? Jeśli jest to jedyny modyfikowany obiekt, być może możesz argumentować, że powinna to być metoda modyfikowanego obiektu, ale innym razem modyfikowanych jest wiele obiektów (które zdecydowanie nie powinny być tej samej klasy), szczególnie w raczej metody wysokiego poziomu.
@delnan Przyznaję, że może to być (nadmierna) reakcja na błąd, który kiedyś musiałem wyśledzić znikającą i ponownie pojawiającą się wartość w haszu - ktoś wstawiał pola obiektu w haszu po jego wstawieniu zamiast tworzyć kopię obiektu na jego użytek. Istnieją defensywne środki programistyczne, które można podjąć (np. Niezmienne obiekty) - ale rzeczy takie jak ponowne obliczanie wartości w samym obiekcie, gdy nie jesteś „właścicielem” obiektu, ani sam obiekt nie jest czymś, co łatwo powrócić do gryzienia ty ciężko.
Wiesz, istnieje dobry argument za użyciem większej liczby wolnych funkcji zamiast funkcji klasy w celu zwiększenia enkapsulacji. Oczywiście funkcja otrzymująca stały obiekt (czy to domyślnie ten, czy parametr) nie powinna go zmieniać w żaden możliwy do zaobserwowania sposób (niektóre stałe obiekty mogą buforować rzeczy).
Deduplicator
2

Zawsze stosuję praktykę nie mutowania obiektu innej osoby. To znaczy, tylko mutujące obiekty należące do klasy / struct / w której jesteś.

Biorąc pod uwagę następującą klasę danych:

class Item {
    public double price = 0;
}

To jest wporządku:

class Foo {
    private Item bar = new Item();

    public void predictPrice() {
        bar.price = bar.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
foo.predictPrice();
System.out.println(foo.bar.price);
// Since I called predictPrice on Foo, which takes and returns nothing, it's
// apparent something might've changed

I to jest bardzo jasne:

class Foo {
    private Item bar = new Item();
}
class Baz {
    public double predictPrice(Item item) {
        return item.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
Baz baz = Baz();
foo.bar.price = baz.predictPrice(foo.bar);
System.out.println(foo.bar.price);
// Since I explicitly set foo.bar.price to the return value of predictPrice, it's
// obvious foo.bar.price might've changed

Ale to jest zbyt błotniste i tajemnicze jak na mój gust:

class Foo {
    private Item bar = new Item();
}
class Baz {
    public void predictPrice(Item item) {
        item.price = item.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
Baz baz = Baz();
baz.predictPrice(foo.bar);
System.out.println(foo.bar.price);
// In my head, it doesn't appear that to foo, bar, or price changed. It seems to
// me that, if anything, I've placed a predicted price into baz that's based on
// the value of foo.bar.price
Ben Leggiero
źródło
Prosimy o dołączenie komentarza do komentarza, dzięki czemu wiem, jak pisać lepsze odpowiedzi w przyszłości :)
Ben Leggiero
1

Składnia jest różna w różnych językach i nie ma sensu pokazywanie fragmentu kodu, który nie jest specyficzny dla danego języka, ponieważ oznacza zupełnie różne rzeczy w różnych językach.

W Javie Itemjest typem odniesienia - jest to typ wskaźników do obiektów, które są instancjami Item. W C ++ ten typ jest zapisany jako Item *(składnia tego samego jest różna w różnych językach). Tak więc Item predictPrice(Item item)w Javie jest równoważny z Item *predictPrice(Item *item)C ++. W obu przypadkach jest to funkcja (lub metoda), która pobiera wskaźnik do obiektu i zwraca wskaźnik do obiektu.

W C ++ Item(bez czegokolwiek innego) jest typem obiektu (zakładając, że Itemjest to nazwa klasy). Wartość tego typu „to” obiekt i Item predictPrice(Item item)deklaruje funkcję, która pobiera obiekt i zwraca obiekt. Ponieważ &nie jest używany, jest przekazywany i zwracany przez wartość. Oznacza to, że obiekt jest kopiowany po przekazaniu i kopiowany po zwróceniu. Java nie ma odpowiednika, ponieważ Java nie ma typów obiektów.

Więc co to jest? Wiele rzeczy, które zadajesz w pytaniu (np. „Wierzę, że działa na tym samym obiekcie, który jest przekazywany ...”) zależy od zrozumienia, co dokładnie jest przekazywane i zwracane tutaj.

użytkownik102008
źródło
1

Konwencja, do której się przyzwyczaiłem, opiera się na preferowaniu stylu, który jest jasny ze składni. Jeśli funkcja zmienia wartość, wolisz przekazać wskaźnik

void predictPrice(Item * const item);

Ten styl powoduje:

  • Nazywając to widzisz:

    przewidywanie ceny (& my_item);

i wiesz, że zostanie zmodyfikowany przez twoje przywiązanie do stylu.

  • W przeciwnym razie wolisz przekazać parametr const ref lub wartość, jeśli nie chcesz, aby funkcja modyfikowała instancję.
użytkownik27886
źródło
Należy zauważyć, że chociaż jest to znacznie bardziej przejrzysta składnia, nie jest dostępna w Javie.
Ben Leggiero
0

Chociaż nie mam silnych preferencji, głosowałbym, że zwracanie obiektu prowadzi do większej elastyczności interfejsu API.

Zauważ, że ma sens tylko wtedy, gdy metoda ma swobodę zwracania innego obiektu. Jeśli twój javadoc mówi, @returns the same value of parameter ato jest bezużyteczne. Ale jeśli mówi javadoc @return an instance that holds the same data than parameter a, to zwrócenie tej samej instancji lub innej jest szczegółem implementacji.

Na przykład w moim bieżącym projekcie (Java EE) mam warstwę biznesową; do przechowywania w DB (i przypisywania automatycznego identyfikatora) pracownikowi, powiedzmy pracownikowi, który mam

 public Employee createEmployee(Employee employeeData) {
   this.entityManager.persist(employeeData);
   return this.employeeData;
 }

Oczywiście nie ma różnicy w tej implementacji, ponieważ JPA zwraca ten sam obiekt po przypisaniu identyfikatora. Teraz, jeśli przejdę na JDBC

 public Employee createEmployee(Employee employeeData) {
   PreparedStatement ps = this.connection.prepareStatement("INSERT INTO EMPLOYEE(name, surname, data) VALUES(?, ?, ?)");
   ... // set parameters
   ps.execute();
   int id = getIdFromGeneratedKeys(ps);
   ??????
 }

Teraz w ????? Mam trzy opcje.

  1. Zdefiniuj seter dla Id, a ja zwrócę ten sam obiekt. Brzydkie, bo setIdbędą dostępne wszędzie.

  2. Wykonaj ciężkie odbicie i ustaw identyfikator w Employeeobiekcie. Brudne, ale przynajmniej ogranicza się do createEmployeezapisu.

  3. Podaj konstruktor, który kopiuje oryginał Employeei akceptuje również idzestaw.

  4. Odzyskaj obiekt z bazy danych (może być potrzebny, jeśli niektóre wartości pól są obliczane w bazie danych lub jeśli chcesz wypełnić tabelę).

Teraz dla 1. i 2. możesz zwracać tę samą instancję, ale dla 3. i 4. zwracasz nowe instancje. Jeśli z podmiotu ustanowionego w interfejsie API ta „rzeczywista” wartość zostanie zwrócona przez metodę, masz więcej swobody.

Jedynym prawdziwym argumentem, który mogę temu przeciwstawić, jest to, że przy użyciu implementacji 1. lub 2. osoby korzystające z interfejsu API mogą przeoczyć przypisanie wyniku, a ich kod może ulec awarii, jeśli zmienisz implementację na 3. lub 4.).

W każdym razie, jak widać, moje preferencje opierają się na innych osobistych preferencjach (takich jak zakaz ustawiania identyfikatorów), więc może nie będzie to dotyczyło wszystkich.

SJuan76
źródło
0

Podczas kodowania w Javie należy spróbować dopasować użycie każdego obiektu typu zmiennego do jednego z dwóch wzorców:

  1. Dokładnie jedna jednostka jest uważana za „właściciela” obiektu zmiennego i traktuje stan tego obiektu jako część swojego własnego. Inne podmioty mogą przechowywać referencje, ale powinny traktować referencje jako identyfikujące obiekt, który jest własnością innej osoby.

  2. Pomimo tego, że obiekt jest modyfikowalny, nikt, kto posiada odwołanie do niego, nie może go mutować, co czyni instancję efektywnie niezmienną (zauważ, że z powodu dziwactw w modelu pamięci Javy nie ma dobrego sposobu, aby sprawić, by obiekt niezmiennie niezmienny miał wątek bezpieczna niezmienna semantyka bez dodawania dodatkowego poziomu pośredniego do każdego dostępu). Każdy byt, który enkapsuluje stan za pomocą odwołania do takiego obiektu i chce zmienić stan enkapsulowany, musi utworzyć nowy obiekt, który zawiera odpowiedni stan.

Nie lubię, gdy metody mutują podstawowy obiekt i zwracają odniesienie, ponieważ dają wrażenie dopasowania drugiego wzorca powyżej. Jest kilka przypadków, w których takie podejście może być pomocne, ale kod, który ma mutować obiekty, powinien wyglądać tak, jak powinien. kod podobny myThing = myThing.xyz(q);wygląda o wiele mniej, niż mutuje obiekt zidentyfikowany przez, myThingniż po prostu myThing.xyz(q);.

supercat
źródło