Please critique my Enumerable DataReader with yield and log data access pattern We are generating our data access code following this pattern:  

public static IEnumerable&lt;SomeDataOutputDTO&gt; 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
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.