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 Userbyty, 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ą?

Problem polega na tym, że Userfaktycznie może zawierać dwie różne rzeczy:

1. Kompletny byt użytkownika, który można przekazać do magazynu danych.

2. 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. UserKlasy i EnrollRequestklasy. 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ć:

public User InsertUser(EnrollRequest request) {
    var userName = GenerateUserName();
    var password = GeneratePassword();

    //You might want to replace this with a factory call, but "new" works here as an example
    var newUser = new User
    (
        request.Name, 
        request.Email, 
        userName, 
        password
    );
    context.Users.Add(user);
    return newUser;
}

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.

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 jak

 repo.InsertUser(user);

aby zmienić stan wewnętrzny repozytoriów, a nie stan obiektu użytkownika. Ten problem występuje w obu twoich implementacjach, w jaki InsertUsersposó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 InsertUsermusi dostarczyć w pełni zainicjowany Userobiekt, lub

• wbudować 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, InsertUserWithNewNameAndPasswordlub, 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.

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.UserNamei user.Password. Mam zastrzeżenia do hasła, ale te zastrzeżenia nie są związane z tematem.

Implikacje modyfikacji obiektów odniesienia

• Utrudnia to jednoczesne programowanie - ale nie wszystkie aplikacje są wielowątkowe
• Te modyfikacje mogą być zaskakujące, szczególnie jeśli nic w metodzie nie sugeruje, że tak się stanie
• Te niespodzianki mogą utrudnić konserwację

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 poprawnie
• Jeśli nazwa wymaga informacji od obiektu użytkownika, możesz zmienić podpis GenerateUserName(User user)i zachować tę testowalność

Nowa metoda ukrywa mutacje:

• Nie wiesz, że Userobiekt się zmienia, dopóki nie przejdziesz 2 warstw w głąb
• W tym przypadku zmiany Userobiektu są bardziej zaskakujące
• SetUserName()robi więcej niż ustawia nazwę użytkownika. To nie jest prawda w reklamie, co utrudnia nowym programistom odkrycie, jak działają Twoje aplikacje

W pełni zgadzam się z odpowiedzią Johna Wu. Jego sugestia jest dobra. Ale to trochę pomija twoje bezpośrednie pytanie.

Zasadniczo, czy praktyka ustalania wartości nieruchomości z innej metody jest złą praktyką?

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:

var myContract = CreateEmptyContract();

ArrrangeContractDeletedStatus(myContract);

Wyraźnie oczekuję, że ArrrangeContractDeletedStatusmetoda zmieni stan myContractobiektu.

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 CreateEmptyContracti ArrrangeContractDeletedStatusdo 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:

myContract = ArrrangeContractDeletedStatus(myContract);

Jest to albo zbędne (ponieważ i tak zmieniam obiekt), albo zmuszam się do głębokiego sklonowania myContractobiektu; 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.

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

Gdzie jest wstawiony użytkownik?

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 Useri dałem mu a namei ustawiłem password, 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

Ustaw nazwę użytkownika? Do czego?

Lepiej: SetRandomNamelub coś, co odzwierciedla intencję.

3) SetPassword(User user)

Pytanie

Ustaw hasło? Do czego?

Lepiej: SetRandomPasswordlub coś, co odzwierciedla intencję.

Sugestia:

Wolałbym czytać coś w stylu:

public User GenerateRandomUser(UserFactory Uf){
    User u = Uf.GetUser();
    u.Name = GenerateRandomUserName();
    u.Password = GenerateRandomUserPassword();
    return u;
}

...

public void AddUserToContext(User u){
    this.context.Users.Add(u);
}

Odnośnie twojego pierwszego pytania:

Czy modyfikowanie obiektu przekazywanego przez odwołanie jest złą praktyką?

Nie. To bardziej kwestia gustu.