Please critique my Enumerable DataReader with yield and log data access pattern

Question

We are generating our data access code following this pattern:

public static IEnumerable<SomeDataOutputDTO> GetSomeData(SomeDataInputDTO dto, IDbConnection dbConnection) {
 var queryString = @"Some SQL query";
 using (var command = (OracleCommand)dbConnection.CreateCommand()) {
 command.CommandText = queryString;
 command.BindByName = true;
 command.Parameters.Add("SomeParam", OracleDbType.Int16, dto.SomeParam, ParameterDirection.Input);
 command.Parameters.Add("SomeOtherParam", OracleDbType.Int16, dto.SomeOtherParam, ParameterDirection.Input);
 var success = false;
 try {
 using (var reader = command.ExecuteReader()) {
 success = true;
 while (reader.Read()) {
 object data;
 var newDto = new SomeDataOutputDTO();
 data = reader["first_column"];
 newDto.FirstColumn = (short)Convert.ChangeType(data, typeof(short));
 data = reader["second_column"];
 if (data != DBNull.Value) {
 newDto.SecondColumn = (short)Convert.ChangeType(data, typeof(short));
 }
 yield return newDto;
 }
 }
 }
 finally {
 if (success) {
 QueryLogger.Debug(queryString, command.Parameters, "TheseQueries.GetSomeData");
 }
 else {
 QueryLogger.Error(queryString, command.Parameters, "DocumentTypeQueries.GetDocumentTypesByApplyType");
 }
 }
}

I want to know if there are any pitfalls regarding this code. As you can see the control flow is a bit complex since it returns an Enumerable and to do that it contains a

yield return 

statement.

Note also that there are two using() blocks and one try block in the middle. The purpose of the try-finally block is to ensure proper logging of the query, in Error level if there was an error, or Debug level if the query ran OK. The purpose of the using blocks is to ensure proper disposal of the command and reader objects.

The QueryLogger helper class uses log4net, not that it should matter much.

Please also remember that this is code generated by a tool, so whether it's readable or easy to maintain is not a concern. My concerns are with proper resource disposal, proper logging, etc.

Also, returning an IEnumerable is very important because it allows to use LINQ when consuming the results. Of particular interest when using LINQ are:

• Take / Skip
• ToLookup / ToDictionary
• ToList
• FirstOrDefault

• svick: It seems your code is badly indented, try doesn't match with finally.
• Mast: Please do not change the code inside the question after getting a review. This possibly invalidates the answers provided.
• Alvaro Rodriguez: @Mast the code didn't change, I accepted an edit with some reindents that's all
• Mast: Take a look at the revision history and click on 'side-by-side markdown' to see what exactly did change. It was more than indentation.

Answer 1

Initially, I shared your concern over resource disposal. My assumption was that there was the possibility the command could be left hanging around in cases where you did not enumerate over the entire collection (as with something like FirstOrDefault).

However, a few quick tests with a test project reveals that the using statement performs its clean-up as soon as you are done with the enumerator. A LINQ statement or a foreach loop may only partially traverse the results, but they both still clean up the enumerator when exiting scope.

Keep in mind, though, that there is still potential confusion that might arise from the deferred execution of GetSomeData. The command doesn't execute until you start enumerating the result. If anything were to modify the data before between the time you call GetSomeData and when you enumerate the result, you could get different results than you expect.

• Chris Hannon: +1 - Both the foreach and the LINQ statements clean up by calling Dispose() on the enumerator, which disposes any open using statements and executes finally blocks. The only case worth mentioning where neither would be executed is when you call GetSomeData().GetEnumerator() yourself and don't bother to call Dispose(). That's relatively uncommon, though.

Answer 2

Careful with yield and Dispose.

It is true that LINQ and foreach call the Dispose method; but if you use the Enumerator manually (by using MoveNext() and Current etc.) for some reason you are also responsible to dispose the enumerator yourself. And a lot less seasoned developers simply forget to check or assume it's not needed at all. This is one of those major pitfalls that are not obvious without knowing the implementation details of your method.