Przekazywanie obiektu do metody, która go zmienia, czy jest to wspólny (anty-) wzorzec?

17

Czytam o typowych zapachach kodu w książce Martin Fowler Refactoring . W tym kontekście zastanawiałem się nad wzorem, który widzę w bazie kodu i czy można obiektywnie uznać go za anty-wzór.

Wzorzec to taki, w którym obiekt jest przekazywany jako argument do jednej lub więcej metod, z których wszystkie zmieniają stan obiektu, ale żadna z nich nie zwraca obiektu. Opiera się więc na charakterze referencyjnym (w tym przypadku) C # / .NET.

var something = new Thing();
// ...
Foo(something);
int result = Bar(something, 42);
Baz(something);

Uważam, że (zwłaszcza gdy metody nie są odpowiednio nazwane) muszę przyjrzeć się takim metodom, aby zrozumieć, czy stan obiektu się zmienił. To sprawia, że ​​rozumienie kodu jest bardziej złożone, ponieważ muszę śledzić wiele poziomów stosu wywołań.

Chciałbym zaproponować ulepszenie takiego kodu, aby zwracał inny (sklonowany) obiekt z nowym stanem lub cokolwiek, co jest potrzebne do zmiany obiektu w miejscu wywołania.

var something1 =  new Thing();
// ...

// Let's return a new instance of Thing
var something2 = Foo(something1);

// Let's use out param to 'return' other info about the operation
int result;
var something3 = Bar(something2, out result);

// If necessary, let's capture and make explicit complex changes
var changes = Baz(something3)
something3.Apply(changes);

Wydaje mi się, że pierwszy wzór jest wybierany na podstawie założeń

  • że jest to mniej pracy lub wymaga mniej wierszy kodu
  • że pozwala nam zarówno zmienić obiekt, jak i zwrócić inną informację
  • że jest bardziej wydajny, ponieważ mamy mniej przypadków Thing.

Ilustruję alternatywę, ale aby ją zaproponować, trzeba argumentować przeciwko oryginalnemu rozwiązaniu. Jakie argumenty można przedstawić, aby stwierdzić, że oryginalne rozwiązanie jest anty-wzorcem?

A co, jeśli w ogóle, jest nie tak z moim alternatywnym rozwiązaniem?

Michiel van Oosterhout
źródło
1
To jest efekt uboczny
Dave Hillier
1
@DaveHillier Dzięki, znałem ten termin, ale nie nawiązałem połączenia.
Michiel van Oosterhout,

Odpowiedzi:

9

Tak, oryginalne rozwiązanie jest anty-wzorcem z powodów, które opisujesz: utrudnia rozumowanie o tym, co się dzieje, obiekt nie jest odpowiedzialny za swój własny stan / implementację (zerwanie enkapsulacji). Dodałbym również, że wszystkie te zmiany stanu są niejawnymi umowami metody, co czyni tę metodę kruchą w obliczu zmieniających się wymagań.

To powiedziawszy, twoje rozwiązanie ma swoje wady, z których najbardziej oczywistym jest to, że klonowanie obiektów nie jest świetne. W przypadku dużych obiektów może być wolny. Może to prowadzić do błędów, w których inne części kodu trzymają się starych odniesień (co prawdopodobnie ma miejsce w opisywanej bazie kodu). Uczynienie tych obiektów niezmiennymi rozwiązuje przynajmniej kilka z tych problemów, ale jest bardziej drastyczną zmianą.

O ile obiekty nie są małe i nieco przejściowe (co czyni ich dobrymi kandydatami na niezmienność), byłbym skłonny po prostu przenieść więcej przejścia stanu do samych obiektów. To pozwala ukryć szczegóły implementacji tych przejść i ustalić bardziej rygorystyczne wymagania dotyczące tego, kto / co / gdzie te zmiany stanu występują.

Telastyn
źródło
1
Na przykład, jeśli mam obiekt „Plik”, nie próbowałbym przenieść do tego obiektu żadnej metody zmieniającej stan - naruszałoby to SRP. Jest to ważne nawet wtedy, gdy masz własne klasy zamiast klasy bibliotecznej, takiej jak „Plik” - umieszczenie logiki przejścia stanu w klasie obiektu tak naprawdę nie ma sensu.
Doc Brown
@Tetastyn Wiem, że to stara odpowiedź, ale mam problemy z wizualizacją twojej sugestii w ostatnim akapicie w konkretnych słowach. Czy mógłbyś opracować lub podać przykład?
AaronLS
@AaronLS - Zamiast Bar(something)(i modyfikować stan something), uczyń Barczłonkiem somethingtypu. something.Bar(42)jest bardziej prawdopodobne, że mutuje something, jednocześnie umożliwiając korzystanie z narzędzi OO (stan prywatny, interfejsy itp.) w celu ochrony somethingstanu
Telastyn
14

gdy metody nie są odpowiednio nazwane

Właściwie, to jest prawdziwe zapachy kodu. Jeśli masz obiekt podlegający zmianom, udostępnia metody zmiany jego stanu. Jeśli masz wywołanie takiej metody osadzonej w zadaniu kilku instrukcji, dobrze jest przefiltrować to zadanie do własnej metody - co pozostawia dokładną opisaną sytuację. Ale jeśli nie masz nazw metod takich jak Fooi Bar, ale metody, które wyjaśniają, że zmieniają obiekt, nie widzę tutaj problemu. Myśleć o

void AddMessageToLog(Logger logger, string msg)
{
    //...
}

lub

void StripInvalidCharsFromName(Person p)
{
// ...
}

lub

void AddValueToRepo(Repository repo,int val)
{
// ...
}

lub

void TransferMoneyBetweenAccounts(Account source, Account destination, decimal amount)
{
// ...
}

lub coś w tym rodzaju - nie widzę tu powodu, aby zwracać sklonowany obiekt dla tych metod, a także nie ma powodu, aby sprawdzać ich implementację, aby zrozumieć, że zmienią one stan przekazywanego obiektu.

Jeśli nie chcesz efektów ubocznych, spraw, aby twoje obiekty były niezmienne, wymusi metody takie jak powyższe, aby zwrócić zmieniony (sklonowany) obiekt bez zmiany oryginalnego.

Doktor Brown
źródło
Masz rację, refaktoryzacja metody zmiany nazwy może poprawić sytuację, wyjaśniając skutki uboczne. Może to być trudne, jeśli modyfikacje są takie, że zwięzła nazwa metody nie jest możliwa.
Michiel van Oosterhout,
2
@ michielvoo: jeśli nazywanie metod Consise wydaje się niemożliwe, twoja metoda grupuje złe rzeczy razem, zamiast budować abstrakcję funkcjonalną dla wykonywanego zadania (i jest to prawdą z efektami ubocznymi lub bez nich).
Doc Brown,
4

Tak, zobacz http://codebetter.com/matthewpodwysocki/2008/04/30/side-effecting-functions-are-code-smells/, gdzie znajduje się jeden z wielu przykładów ludzi wskazujących, że nieoczekiwane skutki uboczne są złe.

Zasadniczo podstawową zasadą jest to, że oprogramowanie jest zbudowane warstwowo, a każda warstwa powinna przedstawiać następną możliwie najczystszą abstrakcję. A czysta abstrakcja to taka, w której musisz mieć jak najmniej na uwadze, aby z niej korzystać. Nazywa się to modułowością i dotyczy wszystkiego - od pojedynczych funkcji po protokoły sieciowe.

btilly
źródło
Scharakteryzowałbym to, co OP opisuje jako „oczekiwane skutki uboczne”. Na przykład, delegat możesz przekazać do jakiegoś silnika, który działa na każdym elemencie na liście. Zasadniczo to ForEach<T>robi.
Robert Harvey,
@RobertHarvey Skargi na niepoprawne nazewnictwo metod oraz na konieczność czytania kodu w celu wykrycia skutków ubocznych sprawiają, że zdecydowanie nie spodziewane są skutki uboczne.
btilly,
Dam ci to. Ale następstwem jest to, że właściwie nazwana udokumentowana metoda z oczekiwanymi efektami witryny może wcale nie być anty-wzorem.
Robert Harvey,
@RobertHarvey Zgadzam się. Kluczem jest to, że znaczące skutki uboczne są bardzo ważne, aby o nich wiedzieć i muszą być starannie udokumentowane (najlepiej w nazwie metody).
btilly,
Powiedziałbym, że jest to mieszanka nieoczekiwanych i nieoczywistych skutków ubocznych. Dzięki za link.
Michiel van Oosterhout,
3

Przede wszystkim nie zależy to od „natury referencyjnej”, lecz od obiektów będących zmiennymi typami referencyjnymi. W językach niefunkcjonalnych prawie zawsze tak będzie.

Po drugie, to, czy jest to problem, czy nie, zależy zarówno od obiektu, jak i od tego, jak ściśle zmiany w różnych procedurach są ze sobą powiązane - jeśli nie dokonasz zmiany w Foo i spowoduje to awarię Bar, to problem. Niekoniecznie zapach kodu, ale jest to problem z Foo, Bar lub Coś (prawdopodobnie Bar tak jak powinien sprawdzać jego wejście, ale może to być Coś w niewłaściwym stanie, któremu powinien zapobiegać).

Nie powiedziałbym, że wznosi się do poziomu anty-wzoru, ale raczej coś, o czym należy pamiętać.

jmoreno
źródło
2

Twierdziłbym, że nie ma różnicy między A.Do(Something)modyfikowaniem somethinga something.Do()modyfikowaniem something. W obu przypadkach powinno być jasne z nazwy wywoływanej metody, która somethingzostanie zmodyfikowana. Jeśli nie jest jasne z nazwy metody, niezależnie od tego, czy somethingjest to parametr this, czy część środowiska, nie należy go modyfikować.

svidgen
źródło
1

Myślę, że w niektórych sytuacjach można zmienić stan obiektu. Na przykład mam listę użytkowników i chcę zastosować różne filtry do listy przed zwróceniem jej do klienta.

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();

var excludeAdminUsersFilter = new ExcludeAdminUsersFilter();
var filterByAnotherCriteria = new AnotherCriteriaFilter();

excludeAdminUsersFilter.Apply(users);
filterByAnotherCriteria.Apply(users); 

I tak, możesz uczynić to ładnym, przenosząc filtrowanie na inną metodę, dzięki czemu uzyskasz coś w stylu:

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();
Filter(users);

Gdzie Filter(users)można wykonać powyższe filtry.

Nie pamiętam, gdzie dokładnie się z tym spotkałem, ale myślę, że nazywano to potokiem filtrującym.

CodeART
źródło
0

Nie jestem pewien, czy proponowane nowe rozwiązanie (kopiowania obiektów) jest wzorem. Problemem, jak wskazałeś, jest zła nomenklatura funkcji.

Powiedzmy, że piszę złożoną operację matematyczną jako funkcję f () . I udokumentować, że f () jest funkcją, która odwzorowuje NXNsię N, a algorytm za nim. Jeśli funkcja jest nazwana niewłaściwie, nieudokumentowana i nie ma towarzyszących jej przypadków testowych, musisz zrozumieć kod, w którym to przypadku kod jest bezużyteczny.

O twoim rozwiązaniu, kilka spostrzeżeń:

  • Aplikacje są projektowane z różnych aspektów: gdy obiekt jest używany tylko do przechowywania wartości lub jest przekazywany przez granice komponentów, mądrze jest zmienić wewnętrzne elementy obiektu na zewnątrz, zamiast wypełniać go szczegółami dotyczącymi zmiany.
  • Klonowanie obiektów prowadzić do wzdęcia zapotrzebowanie na pamięć, a w wielu przypadkach prowadzi do istnienia równoważnych obiektów niezgodnych stanów ( Xstała Ypo f(), ale Xto faktycznie Y,) i ewentualnie niezgodność czasową.

Problem, który próbujesz rozwiązać, jest poprawny; jednak nawet przy ogromnej inżynierii, problem jest omijany, a nie rozwiązywany.

CMR
źródło
2
To byłaby lepsza odpowiedź, gdybyś powiązał swoją obserwację z pytaniem PO. W rzeczywistości jest to bardziej komentarz niż odpowiedź.
Robert Harvey,
1
@RobertHarvey +1, dobra obserwacja, zgadzam się, edytuję to.
CMR,