Czy funkcja jest w stanie zmodyfikować parametr?

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

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.

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.

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();
}

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 {...}.

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