Best practice in C#: Should caller or callee create object?

Question

Although my question is similar to Freeing memory in caller or callee?, this is for C# and its more towards who should create an object rather then who should free it.

Which approach (or some other approach) represents best practice? My intuition says Approach 1 because who knows who instantiated the object otherwise? On the other hand, a DataTable doesn't need explicit disposing, so who cares who instantiates it?

Approach #1

DataTable dataTable = dataAccessLayer._QueryDbGetAllTablesForDb(new DataTable());
.
//or 
.
DataTable dataTable = new DataTable();
dataTable = dataAccessLayer._QueryDbGetAllTablesForDb(dataTable);
.
//then
DataTable _QueryDbGetAllTablesForDb(DataTable datatable);
{
    .
    sqlDataAdapter.Fill(dataTable);
    .

    return dataTable
}

Approach #2

DataTable dataTable = dataAccessLayer._QueryDbGetAllTablesForDb();
    .
//then
DataTable _QueryDbGetAllTablesForDb();
{
    .
    DataTable dataTable = new DataTable();
    sqlDataAdapter.Fill(dataTable);
    .
    return dataTable
}

The linked question is about C++, which is an entirely different beast than C# - C# has a garbage collector which takes care of memory management — UnholySheep, Mar 15 '18 at 16:42
"best practice" and `DataTable` / `DbDataAdapter` *rarely* co-exist in the same question... I think you might be worrying about the wrong thing here — Marc Gravell, Mar 15 '18 at 16:44
Second approach is better. No point of getting the data table from the calling method and then returning it back to calling method. data tables are reference types so any changes made inside the called method will reflect in the calling method (except, of course, referencing a different object). — Zohar Peled, Mar 15 '18 at 16:46
@MarcGravell: Perhaps, but the nut I'm trying to crack involves reading DB tables without knowledge of their structure/schema at design time (i.e., discovering their structure/schema at run time) so this seemed to me to be the simplest and most direct approach — VA systems engineer, Mar 15 '18 at 16:48
Is there ever a reason to pass in a not-empty data table? If there's only one sensible way to call a method, then allowing the user the option to call it some other way is *making opportunities for bugs*. — Eric Lippert, Mar 15 '18 at 16:48
@NovaSysEng fair enough: if you don't know the schema at runtime is the *main good use-case* for `DataTable`: good call, thanks for keeping me honest :) — Marc Gravell, Mar 15 '18 at 16:50
"`DataTable` doesn't need explicit disposing" - it implements `IDisposable`, so it most assuredly should be deterministically disposed. — Jesse C. Slicer, Mar 15 '18 at 16:52
@MarcGravell: I don't understand your question "what is Dapper?" — VA systems engineer, Mar 15 '18 at 16:52
The second approach is best. You're making a helper method to create a DataTable, the caller shouldn't have to worry about the details. It should of course be aware that DataTable is disposable in any case. — Mikkel K., Mar 15 '18 at 16:55
@JesseC.Slicer: Agreed re: Idisposable, but not so sure about having to use it. Please see https://stackoverflow.com/questions/913228/should-i-dispose-dataset-and-datatable. On the other hand, why not? — VA systems engineer, Mar 15 '18 at 17:00
@NovaSysEng ignore me; I confused and inverted what you had said in my head; I replaced that with a clearer comment — Marc Gravell, Mar 15 '18 at 17:02
@NovaSysEng While true that the Dispose method is a no-op on DataSet and DataTable, that's an implementation details. As a rule of thumb, anything that you instantiate that is an `IDisposable` you should also dispose. You shouldn't bother yourself getting intimate knowledge of the implementation of every class you work with - that goes against the encapsulation principle which is one of the 3 base principles of object oriented programming. — Zohar Peled, Mar 16 '18 at 06:54

Zohar Peled · Accepted Answer · 2018-03-16T07:39:17.667

So, to recap what's been written in the comments (with a bit of rephrasing):

The use of a DataTable is in it for itself probably not best practice anyway. The main good use-case for using DataTables is when the database schema is not known at design time. (Marc Gravell's comment)
There is no point of passing a reference type to a method just to get it back as it's return value. Any change made to the argument's state will reflect anyway to the calling method since it's a reference type. (My comment)
If the method's only use of the DataTable is to fill it and return it, Allowing the user the option to call it with a DataTable is not a sensible use - therefor having the method accept a data table as an argument is just opening a door for potential confusion and bugs. (Eric Lippert's comment)
The purpose of the method is to create and return a DataTable. The reasonable behavior of the method is to instantiate the DataTable. A more general rule of thumb: The calling method doesn't need to be bothered with instantiating the return value of the called method. (Mikkel K.'s comment)

On the secondary point about disposing a DataTable / DataSet:

As a rule of thumb - Any instance of a class that implements the IDisposable interface should be disposed. While it's true that a DataTable and a DataSet does not use any non-managed resources, and therefor doesn't actually need to be disposed, that is an implementation detail. The fact that some type implements the IDisposable interface is the only thing you need to consider when asking if it should be disposed. One of the fundamental principles of object oriented programming is called Encapsulation. The main point of encapsulation that's related to this issue is that you should not be bothered with the implementation of the type you are dealing with, only with it's public interface. Another way to look at it is "Program to an 'interface', not an 'implementation'." (from Design Patterns: Elements of Reusable Object-Oriented Software) (Jesse C. Slicer's comment and My second comment)

Excellent summary. I adapted Approach 2, primarily based on Eric Lippert's observation. I will also dispose of my datatable objects. One note: the use of DataTable objects in a scenario where the DB schema is not known at design time was my reply to Marc's initial comment re: how I should probably not be using them. In my case, I cannot use any approach that requires this knowledge. I see the use of DataTables as the only possible approach in this scenario. Please chime in if you see another, better approach. — VA systems engineer, Mar 16 '18 at 12:01
Using a data table or dataset is probably the best approach in such a case. Glad to help :-) — Zohar Peled, Mar 16 '18 at 19:41

Best practice in C#: Should caller or callee create object?

1 Answers1