¿Está bien que una función modifique un parámetro?

Tenemos una capa de datos que envuelve Linq To SQL. En esta capa de datos tenemos este método (simplificado)

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

Al enviar los cambios, la ID del informe se actualiza con el valor en la base de datos que luego devolvemos.

Desde el lado de la llamada se ve así (simplificado)

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

Mirando el código, la ID se ha establecido dentro de la función InsertReport como una especie de efecto secundario, y luego estamos ignorando el valor de retorno.

Mi pregunta es, ¿debería confiar en el efecto secundario y hacer algo como esto?

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

o deberíamos evitarlo

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

tal vez incluso

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

Esta pregunta se planteó cuando creamos una prueba unitaria y descubrimos que no está realmente claro que la propiedad ID de los parámetros del informe se actualice y que, para burlarse del comportamiento de los efectos secundarios, se sintiera mal, un olor a código, por así decirlo.

Sí, está bien y es bastante común. Sin embargo, puede no ser obvio, como has descubierto.

En general, tiendo a que los métodos de tipo persistencia devuelvan la instancia actualizada del objeto. Es decir:

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

Sí, está devolviendo el mismo objeto que pasó, pero hace que la API sea más clara. No es necesario el clon, si hay algo que pueda causar confusión si, como en el código original, la persona que llama continúa usando el objeto que pasó.

Otra opción es usar un DTO

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

De esta forma, la API es muy obvia, y la persona que llama no puede intentar accidentalmente usar el objeto pasado / modificado. Dependiendo de lo que esté haciendo su código, puede ser un poco complicado.

En mi opinión, este es un caso raro en el que el efecto secundario del cambio es deseable, ya que su entidad de informes TIENE un ID, ya se podría suponer que tiene las preocupaciones de un DTO, y posiblemente existe la obligación de ORM de garantizar que el informe en memoria La entidad se mantiene sincronizada con el objeto de representación de la base de datos.

El problema es que sin documentación, no está nada claro qué está haciendo el método y, especialmente, por qué devuelve un número entero.

La solución más fácil sería usar un nombre diferente para su método. Algo como:

int GenerateIdAndInsert(Report report)

Aún así, esto no es lo suficientemente inequívoco: si, como en C #, la instancia del reportobjeto se pasa por referencia, sería difícil saber si reportse modificó el objeto original , o si el método lo clonó y modificó solo el clon. Si elige modificar el objeto original, sería mejor nombrar el método:

void ChangeIdAndInsert(Report report)

Una solución más complicada (y quizás menos óptima) es refactorizar en gran medida el código. Qué pasa:

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

Por lo general, los programadores esperan que solo los métodos de instancia de un objeto puedan cambiar su estado. En otras palabras, no me sorprende si report.insert()cambia la identificación del informe, y es fácil de verificar. No es fácil preguntarse por cada método en toda la aplicación si cambia la identificación del informe o no.

También diría que quizás IDni siquiera debería pertenecer Reporten absoluto. Dado que no contiene una identificación válida durante tanto tiempo, realmente tiene dos objetos diferentes antes y después de la inserción, con un comportamiento diferente. El objeto "antes" se puede insertar, pero no se puede recuperar, actualizar o eliminar. El objeto "después" es exactamente lo contrario. Uno tiene una identificación y el otro no. La forma en que se muestran puede ser diferente. Las listas en las que aparecen pueden ser diferentes. Los permisos de usuario asociados pueden ser diferentes. Ambos son "informes" en el sentido inglés de la palabra, pero son muy diferentes.

Por otro lado, su código puede ser lo suficientemente simple como para que un objeto sea suficiente, pero es algo a considerar si su código está salpicado if (validId) {...} else {...}.

No, no está bien! Es adecuado para un procedimiento modificar un parámetro solo en lenguajes de procedimiento, donde no hay otra forma; en los idiomas OOP, llame al método de cambio en el objeto, en este caso en el informe (algo así como report.generateNewId ()).

En este caso, su método hace 2 cosas, por lo tanto, rompe SRP: inserta un registro en la base de datos y genera una nueva identificación. La persona que llama no puede saber que su método también genera una nueva identificación, ya que simplemente se llama insertRecord ().