Czy funkcja jest w stanie zmodyfikować parametr?

17

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ć.

John Petrak
źródło
2
Do tego służy dokumentacja API.

Odpowiedzi:

16

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:

Report InsertReport(Report report)
{        
    db.Reports.InsertOnSubmit(report);
    db.SubmitChanges();
    return report; 
}

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

Report InsertReport(ReportDTO dto)
{
    var newReport = Report.Create(dto);
    db.Reports.InsertOnSubmit(newReport);
    db.SubmitChanges();
    return newReport; 
}

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.

Ben Hughes
źródło
Nie zwróciłbym obiektu, jeśli Ira nie byłaby potrzebna. To wygląda na dodatkowe przetwarzanie i wykorzystanie pamięci. Idę do pustki lub wyjątku, jeśli nie jest to potrzebne. W przeciwnym razie głosuję na twoją próbkę DTO.
Niezależny
Wszystkie te odpowiedzi podsumowują nasze własne dyskusje. To wydaje się być pytaniem bez realnej odpowiedzi. Twoje stwierdzenie „Tak, zwracasz ten sam obiekt, który przekazałeś, ale dzięki temu interfejs API jest bardziej przejrzysty”. prawie odpowiedział na nasze pytanie.
John Petrak,
4

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.

StuartLC
źródło
6
+1 - po wstawieniu obiektu do bazy danych można oczekiwać, że będzie on miał identyfikator. Byłoby bardziej zaskakujące, gdyby istota wróciła bez niej.
MattDavey,
2

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:

int GenerateIdAndInsert(Report report)

Nadal nie jest to jednoznaczne: jeśli, podobnie jak w języku C #, instancja reportobiektu zostanie przekazana przez odniesienie, trudno byłoby ustalić, czy oryginalny reportobiekt został zmodyfikowany, czy metoda sklonowała go i zmodyfikowała tylko klon. Jeśli zdecydujesz się zmodyfikować oryginalny obiekt, lepiej nazwać metodę:

void ChangeIdAndInsert(Report report)

Bardziej skomplikowanym (i być może mniej optymalnym) rozwiązaniem jest dokładne przebudowanie kodu. Co powiesz na:

using (var transaction = new TransactionScope())
{
    var id = this.Data.GenerateReportId(); // We need to find an available ID...
    this.Data.AddReportWithId(id, report); // ... and use this ID to insert a report.
    transaction.Complete();
}
Arseni Mourzenko
źródło
2

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ć IDmoż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 {...}.

Karl Bielefeldt
źródło
0

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 ().

m3th0dman
źródło
3
Ermm ... co jeśli wywoła db.Reports.InsertOnSubmit(report)metodę zmiany na obiekcie?
Stephen C
Jest w porządku ... należy tego unikać, ale w tym przypadku LINQ do SQL wykonuje modyfikację parametru, więc nie jest tak, że OP mógłby tego uniknąć bez efektu klonowania skoku z obręczą (co jest jego własnym naruszeniem SRP).
Telastyn
@StephenC Mówiłem, że powinieneś wywołać tę metodę na obiekcie raportu; w tym przypadku przekazanie tego samego obiektu jako parametru nie ma sensu.
m3th0dman
@Telastyn Mówiłem w ogólnym przypadku; dobre praktyki nie mogą być przestrzegane w 100%. W jego szczególnym przypadku nikt nie może wywnioskować, co jest najlepszym sposobem na podstawie 5 linii kodu ...
m3th0dman,