Code Quality – Is It OK for a Function to Modify a Parameter?

code smellcode-qualitydesign

We have a data layer that wraps Linq To SQL. In this datalayer we have this method (simplified)

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

On submit changes, the report ID is updated with the value in the database which we then return.

From the calling side it looks like this (simplified)

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

Looking at the code, ID has been set inside the InsertReport function as a kind of side effect, and then we are ignoring the return value.

My question is, should I rely on the side effect and do something like this instead. 

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

or should we prevent it

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

maybe even 

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

This question was raised when we created a unit test and found that its not really clear that the report parameters ID property gets updated and that to mock the side effect behavior felt wrong, a code smell if you will.

Best Answer

Yes, it's OK, and fairly common. It can be non-obvious though, as you've discovered.

In general, I tend to have persistence-type methods return the updated instance of the object. That is:

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

Yes, you're returning the same object as you passed in, but it makes the API clearer. There's no need for the clone - if anything that will cause confusion if, as in the your original code, the caller continues to use the object they passed in.

Another option is to use a DTO

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

That way the API is very obvious, and the caller can't accidentally try and use the passed in/modified object. Depending on what your code is doing, it can be a bit of a pain though.

Related Topic