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

17

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.

John Petrak
quelle
2
Dafür ist die API-Dokumentation gedacht.

Antworten:

16

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.

Ben Hughes
quelle
Ich würde das Objekt nicht zurückgeben, wenn Ira es nicht brauchte. Das sieht nach zusätzlicher Verarbeitung und Überbelegung des Speichers aus. Ich gehe für eine Leere oder Ausnahme, wenn nicht von Nöten. Ansonsten stimme ich für deine DTO Probe.
Unabhängige
All diese Antworten fassen unsere eigenen Diskussionen zusammen. Dies scheint eine Frage ohne echte Antwort zu sein. Ihre Aussage "Ja, Sie geben das gleiche Objekt zurück, das Sie übergeben haben, aber es macht die API klarer." hat so ziemlich unsere Frage beantwortet.
John Petrak
4

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.

StuartLC
quelle
6
+1 - Nach dem Einfügen einer Entität in die Datenbank wird eine ID erwartet . Es wäre überraschender, wenn die Entität ohne sie zurückkäme.
MattDavey
2

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();
}
Arseni Mourzenko
quelle
2

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

Karl Bielefeldt
quelle
0

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.

m3th0dman
quelle
3
Ähm ... was ist, wenn db.Reports.InsertOnSubmit(report)die Änderungsmethode für das Objekt aufgerufen wird ?
Stephen C
Es ist in Ordnung ... es sollte vermieden werden, aber in diesem Fall nimmt LINQ to SQL die Parameteränderung vor, sodass das OP dies nicht ohne den Kloneffekt "Hoop Jumping" vermeiden könnte (was seine eigene Verletzung von SRP ist).
Telastyn
@StephenC Ich habe gesagt, dass Sie diese Methode für das Berichtsobjekt aufrufen sollten. In diesem Fall hat die Übergabe desselben Objekts als Parameter keine Bedeutung.
m3th0dman
@Telastyn Ich sprach im allgemeinen Fall; Gute Praktiken können nicht zu 100% eingehalten werden. In seinem speziellen Fall kann niemand aus 5 Codezeilen den besten Weg ableiten ...
m3th0dman