Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
2  
This is what API documentation is for. –  user1249 Aug 13 '12 at 6:37

5 Answers 5

up vote 16 down vote accepted

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.

share|improve this answer
    
I would not return the object if Ira not needed. Thats look like extra processing and memory (over)use. I go för a void or exception, if not of need. Else, i vote för your DTO sample. –  Independent Aug 13 '12 at 7:07
    
All these answers pretty much sum up our own discussions. This seems to be a question with no real answer. Your statement "Yes, you're returning the same object as you passed in, but it makes the API clearer." pretty much answered our question. –  John Petrak Aug 14 '12 at 8:34

IMO this is a rare instance where the side effect of change is desirable - because your Report entity HAS an ID it could already be assumed to have the concerns of a DTO, and arguably there is the ORM obligation to ensure that the in-memory Report entity is kept in synch with the database representation object.

share|improve this answer
6  
+1 - after inserting an entity in the DB you would expect it to have an ID. It would be more surprising if the entity came back without it. –  MattDavey Aug 13 '12 at 10:18

The problem is that without documentation, it's not clear at all what the method is doing and especially why is it returning an integer.

The easiest solution would be to use a different name for your method. Something like:

int GenerateIdAndInsert(Report report)

Still, this is not unambiguous enough: if, like in C#, the instance of the report object is passed by reference, it would be difficult to know if the original report object was modified, or if the method cloned it and modified only the clone. If you choose to modify the original object, it would be better to name the method:

void ChangeIdAndInsert(Report report)

A more complicated (and maybe less optimal) solution is to heavily refactor the code. What about:

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();
}
share|improve this answer

No it is not OK! It is suitable for a procedure to modify a parameter only in procedural languages, where there is no other way; in OOP languages call the changing method on the object, in this case on the report (something like report.generateNewId()).

In this case your method does 2 things, hence breaks SRP: it inserts a record in the db and it generates a new id. The caller is not able to know that your method also generates a new id since it is simply called insertRecord().

share|improve this answer
3  
Ermm ... what if db.Reports.InsertOnSubmit(report) calls the change method on the object? –  Stephen C Aug 13 '12 at 9:39
    
It's okay... it should be avoided, but in this case LINQ to SQL is doing the parameter modification so it's not like the OP could avoid that without the hoop jumping clone effect (which is its own violation of SRP). –  Telastyn Aug 13 '12 at 12:26
    
@StephenC I was saying that you should call that method on the report object; in this case there is no meaning in passing the same object as a parameter. –  m3th0dman Aug 13 '12 at 14:25
    
@Telastyn I was speaking in the general case; good practices cannot be respected 100%. In his particular case no one can infer what is the best way from 5 lines of code... –  m3th0dman Aug 13 '12 at 14:26

Usually programmers expect that only an object's instance methods can change its state. In other words, I'm not surprised if report.insert() changes the report's id, and it's easy to check. It's not easy to wonder for every method in the whole application if it changes the report's id or not.

I would also argue that perhaps ID shouldn't even belong to Report at all. Since it doesn't contain a valid id for so long, you really have two different objects before and after insertion, with different behavior. The "before" object can be inserted, but it can't be retrieved, updated, or deleted. The "after" object is the exact opposite. One has an id and the other doesn't. The way they are displayed may be different. The lists they appear in may be different. Associated user permissions may be different. They are both "reports" in the English sense of the word, but they are very different.

On the other hand, your code may be simple enough that one object will suffice, but it's something to consider if your code is peppered with if (validId) {...} else {...}.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.