Ist es in Ordnung, wenn eine Funktion einen Parameter ändert?

Wir haben eine Datenschicht, die Linq To SQL umschließt. In dieser Datenebene haben wir diese Methode (vereinfacht)

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

Bei Änderungen wird die Berichts-ID mit dem Wert in der Datenbank aktualisiert, den wir dann zurückgeben.

Von der aufrufenden Seite sieht es so aus (vereinfacht)

var report = new Report();
DataLayer.InsertReport(report);
// Do something with report.ID

Beim Betrachten des Codes wurde ID in der InsertReport-Funktion als eine Art Nebeneffekt festgelegt, und dann ignorieren wir den Rückgabewert.

Meine Frage ist, sollte ich mich auf die Nebenwirkung verlassen und stattdessen so etwas tun.

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

oder sollten wir es verhindern

int InsertReport(Report report)
{
    var newReport = report.Clone();
    db.Reports.InsertOnSubmit(newReport);
    db.SubmitChanges();
    return newReport.ID; 
}

vielleicht sogar

Report InsertReport(Report report)
{
    var newReport = report.Clone();
    db.Reports.InsertOnSubmit(newReport);
    db.SubmitChanges();
    return newReport; 
}

Diese Frage wurde aufgeworfen, als wir einen Komponententest erstellten und feststellten, dass es nicht wirklich klar ist, dass die ID-Eigenschaft der Berichtsparameter aktualisiert wird und dass, um das Verhalten der Nebenwirkungen zu verspotten, ein Codegeruch empfunden wird, wenn Sie so wollen.

Ja, es ist in Ordnung und ziemlich verbreitet. Es kann jedoch nicht offensichtlich sein, wie Sie festgestellt haben.

Im Allgemeinen tendieren persistente Methoden dazu, die aktualisierte Instanz des Objekts zurückzugeben. Das ist:

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

Ja, Sie geben dasselbe Objekt zurück, das Sie übergeben haben, aber dadurch wird die API übersichtlicher. Der Klon ist nicht erforderlich - wenn etwas verwirrend ist, verwendet der Anrufer weiterhin das Objekt, an das er übergeben wurde, wie im ursprünglichen Code.

Eine andere Option ist die Verwendung eines DTO

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

Auf diese Weise ist die API sehr offensichtlich, und der Aufrufer kann nicht versehentlich versuchen, das übergebene / geänderte Objekt zu verwenden. Abhängig davon, was Ihr Code tut, kann es allerdings etwas schmerzhaft sein.

IMO Dies ist ein seltener Fall, in dem die Nebenwirkung einer Änderung erwünscht ist. Da Ihre Berichtseinheit eine ID hat, kann davon ausgegangen werden, dass sie die Bedenken eines DTO hat, und es besteht möglicherweise die ORM- Verpflichtung , sicherzustellen, dass der Bericht im Speicher vorliegt Die Entität wird mit dem Datenbankrepräsentationsobjekt synchronisiert.

Das Problem ist, dass ohne Dokumentation überhaupt nicht klar ist, was die Methode tut und vor allem, warum sie eine Ganzzahl zurückgibt.

Die einfachste Lösung wäre, einen anderen Namen für Ihre Methode zu verwenden. Etwas wie:

int GenerateIdAndInsert(Report report)

Dies ist jedoch nicht eindeutig genug: Wenn wie in C # die Instanz des reportObjekts als Referenz übergeben wird, ist es schwierig zu wissen, ob das ursprüngliche reportObjekt geändert wurde oder ob die Methode es geklont und nur den Klon geändert hat. Wenn Sie das ursprüngliche Objekt ändern möchten, ist es besser, die Methode zu benennen:

void ChangeIdAndInsert(Report report)

Eine kompliziertere (und möglicherweise weniger optimale) Lösung besteht darin, den Code stark umzugestalten. Wie wäre es mit:

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

Normalerweise erwarten Programmierer, dass nur die Instanzmethoden eines Objekts seinen Status ändern können. Mit anderen Worten, ich bin nicht überrascht, wennreport.insert() sich die ID des Berichts ändert, und es ist einfach zu überprüfen. Es ist nicht einfach, sich für jede Methode in der gesamten Anwendung zu fragen, ob sie die ID des Berichts ändert oder nicht.

Ich würde auch argumentieren, dass das vielleicht IDgar nicht dazu gehören sollteReport . Da es so lange keine gültige ID enthält, haben Sie vor und nach dem Einfügen zwei unterschiedliche Objekte mit unterschiedlichem Verhalten. Das "Vorher" -Objekt kann eingefügt, aber nicht abgerufen, aktualisiert oder gelöscht werden. Das "After" -Objekt ist das genaue Gegenteil. Einer hat einen Ausweis und der andere nicht. Die Art und Weise, wie sie angezeigt werden, kann unterschiedlich sein. Die Listen, in denen sie angezeigt werden, können unterschiedlich sein. Zugehörige Benutzerberechtigungen können unterschiedlich sein. Sie sind beide "Berichte" im englischen Sinne, aber sie sind sehr unterschiedlich.

Auf der anderen Seite mag Ihr Code so einfach sein, dass ein Objekt ausreicht, aber es ist zu prüfen, ob Ihr Code vollgestopft ist if (validId) {...} else {...}.

Nein, es ist nicht in Ordnung! Es ist für eine Prozedur geeignet, einen Parameter nur in prozeduralen Sprachen zu ändern, wo es keinen anderen Weg gibt. Rufen Sie in OOP-Sprachen die Änderungsmethode für das Objekt auf, in diesem Fall für den Bericht (etwa report.generateNewId ()).

In diesem Fall macht Ihre Methode zwei Dinge und bricht SRP ab: Sie fügt einen Datensatz in die Datenbank ein und generiert eine neue ID. Der Aufrufer kann nicht wissen, dass Ihre Methode auch eine neue ID generiert, da sie einfach insertRecord () heißt.