Mamy warstwę danych, która otacza Linq To SQL. W tej warstwie danych mamy tę metodę (uproszczoną)
int InsertReport(Report report)
{
db.Reports.InsertOnSubmit(report);
db.SubmitChanges();
return report.ID;
}
Po przesłaniu zmian identyfikator raportu jest aktualizowany o wartość w bazie danych, którą następnie zwracamy.
Od strony wywołującej wygląda to tak (uproszczone)
var report = new Report();
DataLayer.InsertReport(report);
// Do something with report.ID
Patrząc na kod, ID został ustawiony wewnątrz funkcji InsertReport jako rodzaj efektu ubocznego, a następnie ignorujemy zwracaną wartość.
Moje pytanie brzmi: czy powinienem polegać na skutkach ubocznych i zamiast tego zrobić coś takiego?
void InsertReport(Report report)
{
db.Reports.InsertOnSubmit(report);
db.SubmitChanges();
}
czy powinniśmy temu zapobiec
int InsertReport(Report report)
{
var newReport = report.Clone();
db.Reports.InsertOnSubmit(newReport);
db.SubmitChanges();
return newReport.ID;
}
może nawet
Report InsertReport(Report report)
{
var newReport = report.Clone();
db.Reports.InsertOnSubmit(newReport);
db.SubmitChanges();
return newReport;
}
To pytanie pojawiło się, gdy stworzyliśmy test jednostkowy i okazało się, że nie jest do końca jasne, czy właściwość ID identyfikatora parametrów raportu została zaktualizowana i że aby kpić zachowanie efektu ubocznego było źle, kod może pachnieć.
źródło
Odpowiedzi:
Tak, jest OK i dość powszechne. Jak już odkryłeś, może to być nieoczywiste.
Ogólnie rzecz biorąc, zazwyczaj metody typu trwałości zwracają zaktualizowaną instancję obiektu. To jest:
Tak, zwracasz ten sam obiekt, który przekazałeś, ale dzięki temu interfejs API jest bardziej przejrzysty. Klon nie jest potrzebny - jeśli coś, co spowoduje zamieszanie, jeśli, tak jak w oryginalnym kodzie, osoba dzwoniąca nadal korzysta z obiektu, który przekazała.
Inną opcją jest użycie DTO
W ten sposób interfejs API jest bardzo oczywisty, a osoba dzwoniąca nie może przypadkowo spróbować użyć przekazanego / zmodyfikowanego obiektu. W zależności od tego, co robi Twój kod, może to być trochę uciążliwe.
źródło
IMO jest to rzadki przypadek, w którym pożądany jest efekt uboczny zmiany - ponieważ twoja jednostka raportująca MA ID, można już założyć, że ma obawy DTO, i prawdopodobnie istnieje obowiązek ORM, aby zapewnić, że raport w pamięci obiekt jest zsynchronizowany z obiektem reprezentacji bazy danych.
źródło
Problem polega na tym, że bez dokumentacji wcale nie jest jasne, co robi metoda, a zwłaszcza dlaczego zwraca liczbę całkowitą.
Najłatwiejszym rozwiązaniem byłoby użycie innej nazwy dla metody. Coś jak:
Nadal nie jest to jednoznaczne: jeśli, podobnie jak w języku C #, instancja
report
obiektu zostanie przekazana przez odniesienie, trudno byłoby ustalić, czy oryginalnyreport
obiekt został zmodyfikowany, czy metoda sklonowała go i zmodyfikowała tylko klon. Jeśli zdecydujesz się zmodyfikować oryginalny obiekt, lepiej nazwać metodę:Bardziej skomplikowanym (i być może mniej optymalnym) rozwiązaniem jest dokładne przebudowanie kodu. Co powiesz na:
źródło
Zwykle programiści oczekują, że tylko metody instancji obiektu mogą zmienić jego stan. Innymi słowy, nie jestem zaskoczony, jeśli
report.insert()
zmieni identyfikator raportu, i łatwo to sprawdzić. Nie jest łatwo zastanawiać się nad każdą metodą w całej aplikacji, czy zmienia ona identyfikator raportu, czy nie.Twierdziłbym również, że być
ID
może w ogóle nie powinien on należećReport
. Ponieważ nie zawiera on tak ważnego identyfikatora przez tak długi czas, naprawdę masz dwa różne obiekty przed i po wstawieniu, z innym zachowaniem. Obiekt „przed” można wstawić, ale nie można go pobrać, zaktualizować ani usunąć. Obiekt „po” jest dokładnie odwrotny. Jeden ma identyfikator, a drugi nie. Sposób ich wyświetlania może być inny. Listy, w których się pojawiają, mogą się różnić. Powiązane uprawnienia użytkownika mogą się różnić. Oba są „raportami” w angielskim znaczeniu, ale są bardzo różne.Z drugiej strony, twój kod może być na tyle prosty, że wystarczy jeden obiekt, ale jest coś do rozważenia, jeśli Twój kod jest nasycony
if (validId) {...} else {...}
.źródło
Nie, nie jest OK! Jest odpowiedni dla procedury modyfikowania parametru tylko w językach proceduralnych, gdzie nie ma innego sposobu; w językach OOP wywołuje metodę zmiany na obiekcie, w tym przypadku na raporcie (coś takiego jak report.generateNewId ()).
W tym przypadku twoja metoda robi 2 rzeczy, dlatego psuje SRP: wstawia rekord do db i generuje nowy id. Program wywołujący nie może wiedzieć, że twoja metoda generuje również nowy identyfikator, ponieważ jest on po prostu nazywany insertRecord ().
źródło
db.Reports.InsertOnSubmit(report)
metodę zmiany na obiekcie?