W przeszłości zwykle manipulowałem obiektem w ramach podstawowej metody, która jest tworzona / aktualizowana, ale ostatnio przyjąłem inne podejście i jestem ciekawy, czy to zła praktyka.
Oto przykład. Załóżmy, że mam repozytorium, które akceptuje User
byty, ale przed wstawieniem bytu wywołujemy niektóre metody, aby upewnić się, że wszystkie jego pola są ustawione zgodnie z tym, czego chcemy. Teraz zamiast wywoływać metody i ustawiać wartości pól w ramach metody Insert, wywołuję szereg metod przygotowania, które kształtują obiekt przed jego wstawieniem.
Stara metoda:
public void InsertUser(User user) {
user.Username = GenerateUsername(user);
user.Password = GeneratePassword(user);
context.Users.Add(user);
}
Nowe metody:
public void InsertUser(User user) {
SetUsername(user);
SetPassword(user);
context.Users.Add(user);
}
private void SetUsername(User user) {
var username = "random business logic";
user.Username = username;
}
private void SetPassword(User user) {
var password = "more business logic";
user.Password = password;
}
Zasadniczo, czy praktyka ustalania wartości nieruchomości z innej metody jest złą praktyką?
c#
design
coding-style
JD Davis
źródło
źródło
user
były przekazywane przez referencję, kod mógł szarpać go z rąk rozmówcy i zastąpić ją po prostu mówiąc, powiedzmyuser = null;
.Odpowiedzi:
Problem polega na tym, że
User
faktycznie może zawierać dwie różne rzeczy:Kompletny byt użytkownika, który można przekazać do magazynu danych.
Zestaw elementów danych wymaganych od osoby dzwoniącej, aby rozpocząć proces tworzenia encji użytkownika. System musi dodać nazwę użytkownika i hasło, zanim będzie naprawdę prawidłowym użytkownikiem, jak w punkcie 1 powyżej.
Obejmuje to nieudokumentowane niuanse w twoim modelu obiektowym, które w ogóle nie są wyrażone w twoim systemie typów. Musisz tylko „znać” go jako programistę. To nie jest świetne i prowadzi do dziwnych wzorców kodu, takich jak ten, z którym się spotykasz.
Sugerowałbym, że potrzebujesz dwóch bytów, np.
User
Klasy iEnrollRequest
klasy. Ten ostatni może zawierać wszystko, co musisz wiedzieć, aby utworzyć użytkownika. Wyglądałby jak twoja klasa użytkownika, ale bez nazwy użytkownika i hasła. Następnie możesz to zrobić:Dzwoniący zaczyna od samych informacji rejestracyjnych i odzyskuje ukończonego użytkownika po włożeniu. W ten sposób unikasz mutowania dowolnych klas, a także rozróżniasz między typem użytkownika, który jest wstawiony, a użytkownikiem, który nie jest.
źródło
InsertUser
może wywoływać efekt uboczny wstawiania. Nie jestem pewien, co w tym kontekście oznacza „niegrzeczny”. Jeśli chodzi o używanie „użytkownika”, który nie został dodany, wydaje się, że nie trafiłeś w sedno. Jeśli nie został dodany, to nie jest użytkownikiem, tylko prośbą o utworzenie użytkownika. Oczywiście możesz swobodnie pracować z tą prośbą.Efekty uboczne są w porządku, o ile nie są nieoczekiwane. Więc ogólnie nie ma nic złego, gdy repozytorium ma metodę akceptowania użytkownika i zmiany jego stanu wewnętrznego. Ale IMHO nazwa metody jak
InsertUser
ta nie komunikuje tego wyraźnie , i dlatego jest podatna na błędy. Korzystając z repozytorium, oczekiwałbym połączenia takiego jakaby zmienić stan wewnętrzny repozytoriów, a nie stan obiektu użytkownika. Ten problem występuje w obu twoich implementacjach, w jaki
InsertUser
sposób jest on wewnętrznie całkowicie nieistotny.Aby rozwiązać ten problem, możesz albo
oddzielna inicjalizacja od wstawiania (więc osoba wywołująca
InsertUser
musi dostarczyć w pełni zainicjowanyUser
obiekt, lubwbudować inicjalizację w proces budowy obiektu użytkownika (jak sugerują niektóre inne odpowiedzi), lub
po prostu spróbuj znaleźć lepszą nazwę dla metody, która lepiej wyraża jej działanie.
Więc wybrać nazwę metody takie jak
PrepareAndInsertUser
,OrchestrateUserInsertion
,InsertUserWithNewNameAndPassword
lub, co wolisz, aby bardziej wyraźniejszy efekt uboczny.Oczywiście, tak długa nazwa metody wskazuje, że metoda może robić „za dużo” (naruszenie SRP), ale czasami nie chcesz lub nie możesz łatwo tego naprawić, a to jest najmniej inwazyjne, pragmatyczne rozwiązanie.
źródło
Patrzę na dwie opcje, które wybrałeś i muszę powiedzieć, że zdecydowanie wolę starą metodę niż proponowaną nową. Jest kilka powodów, mimo że zasadniczo robią to samo.
W obu przypadkach ustawiasz
user.UserName
iuser.Password
. Mam zastrzeżenia do hasła, ale te zastrzeżenia nie są związane z tematem.Implikacje modyfikacji obiektów odniesienia
Stara metoda a nowa metoda
Stara metoda ułatwiła testowanie:
GenerateUserName()
jest niezależnie testowalny. Możesz napisać testy dla tej metody i upewnić się, że nazwy są generowane poprawnieGenerateUserName(User user)
i zachować tę testowalnośćNowa metoda ukrywa mutacje:
User
obiekt się zmienia, dopóki nie przejdziesz 2 warstw w głąbUser
obiektu są bardziej zaskakująceSetUserName()
robi więcej niż ustawia nazwę użytkownika. To nie jest prawda w reklamie, co utrudnia nowym programistom odkrycie, jak działają Twoje aplikacjeźródło
W pełni zgadzam się z odpowiedzią Johna Wu. Jego sugestia jest dobra. Ale to trochę pomija twoje bezpośrednie pytanie.
Nie z natury.
Nie możesz posunąć się za daleko, bo napotkasz nieoczekiwane zachowanie. Np.
PrintName(myPerson)
Nie powinien zmieniać obiektu osoby, ponieważ metoda sugeruje, że interesuje ją tylko odczytanie istniejących wartości. Ale to jest inny argument niż w twoim przypadku, ponieważSetUsername(user)
silnie implikuje, że zamierza ustawić wartości.W rzeczywistości jest to podejście, którego często używam do testów jednostkowych / integracyjnych, w których tworzę metodę specjalnie do modyfikowania obiektu w celu ustawienia jego wartości na określoną sytuację, którą chcę przetestować.
Na przykład:
Wyraźnie oczekuję, że
ArrrangeContractDeletedStatus
metoda zmieni stanmyContract
obiektu.Główną zaletą jest to, że metoda pozwala mi przetestować usunięcie kontraktu z różnymi umowami początkowymi; np. umowa z długą historią statusu lub taka, która nie ma poprzedniej historii statusu, umowa z umyślnie błędną historią statusu, umowa, której mój użytkownik testowy nie może usunąć.
Gdybym scalone
CreateEmptyContract
iArrrangeContractDeletedStatus
do jednej metody; Musiałbym utworzyć wiele wariantów tej metody dla każdej innej umowy, którą chciałbym przetestować w stanie usuniętym.I chociaż mógłbym zrobić coś takiego:
Jest to albo zbędne (ponieważ i tak zmieniam obiekt), albo zmuszam się do głębokiego sklonowania
myContract
obiektu; co jest zbyt trudne, jeśli chcesz objąć każdą sprawę (wyobraź sobie, że chcę właściwości nawigacyjnych na kilku poziomach. Czy muszę je wszystkie sklonować? Tylko jednostka najwyższego poziomu? ... Tyle pytań, tyle ukrytych oczekiwań )Zmiana obiektu jest najłatwiejszym sposobem na uzyskanie tego, czego chcę, bez konieczności wykonywania dodatkowej pracy, aby uniknąć zmiany obiektu z zasady.
Tak więc bezpośrednia odpowiedź na twoje pytanie jest taka, że nie jest to z natury zła praktyka, o ile nie ukrywasz, że metoda może zmienić przekazywany obiekt. W twojej obecnej sytuacji jest to wyraźnie wyjaśnione poprzez nazwę metody.
źródło
Uważam, że zarówno stare, jak i nowe problematyczne.
Stara droga:
1)
InsertUser(User user)
Kiedy czytam tę nazwę metody pojawiającą się w moim IDE, pierwszą rzeczą, która przychodzi mi na myśl, jest
Nazwa metody powinien przeczytać
AddUserToContext
. Przynajmniej tak robi metoda na końcu.2)
InsertUser(User user)
To wyraźnie narusza zasadę najmniejszego zaskoczenia. Powiedzmy, że wykonałem swoją pracę do tej pory i stworzyłem świeżą instancję a
User
i dałem mu aname
i ustawiłempassword
, zaskoczyłoby mnie:a) to nie tylko wstawia użytkownika w coś
b) również podważa mój zamiar wstawiania użytkownika w obecnej postaci; nazwa i hasło zostały zastąpione.
c) czy oznacza to, że jest to także naruszenie zasady pojedynczej odpowiedzialności.
Nowy sposób:
1)
InsertUser(User user)
Nadal łamanie SRP i zasada najmniejszego zaskoczenia
2)
SetUsername(User user)
Pytanie
Lepiej:
SetRandomName
lub coś, co odzwierciedla intencję.3)
SetPassword(User user)
Pytanie
Lepiej:
SetRandomPassword
lub coś, co odzwierciedla intencję.Sugestia:
Wolałbym czytać coś w stylu:
Odnośnie twojego pierwszego pytania:
Nie. To bardziej kwestia gustu.
źródło