Code review - Retrieving data from SqlDataReader

Question

I am retrieving columns of different types and checking for null before assigning the class's corresponding property. For string column Its all good. However, I need to decide what to do for the DateTime, Bool and the Enum type?

a) Do I should use nullable DateTime property for Class A or there is a better practice?

b) Is the checking for enum and the bool correct in the below code or there is a better way fo doing this?

 public static List<ClassA> Select(string connectionString)
 {
 List<ClassA> ClassAList = new List<ClassA>();
 using (SqlConnection con = new SqlConnection(connectionString))
 {
 con.Open();
 using (SqlCommand command = new SqlCommand(SPROC_ClassA_Select, con))
 {
 using (SqlDataReader reader = command.ExecuteReader())
 {
 int MyGuidOrdinal = reader.GetOrdinal("MyGuid") ;
 int MyStringOrdinal = reader.GetOrdinal("MyString") ;
 int MyDateTimeOrdinal = reader.GetOrdinal("MyDateTime") ;
 int MyBooleanOrdinal = reader.GetOrdinal("MyBoolean") ;
 int MyEnumValueOrdinal = reader.GetOrdinal("MyEnumValue") ;
 if(reader.HasRows)
 {
 while (reader.Read())
 {
 ClassA classA = new ClassA
 {
 MyGuid = reader.GetGuid(MyGuidOrdinal),
 MyString = reader["MyString"] is DBNull ? null : reader.GetString(MyStringOrdinal),
 MyDateTime = reader["MyDateTime"] is DBNull ? DateTime.MinValue : reader.GetDateTime(MyDateTimeOrdinal), 
 MyBoolean = reader["MyBoolean"] is DBNull ? false : reader.GetBoolean(MyBooleanOrdinal), 
 MyEnumValue = reader["MyEnumValue"] is DBNull ? (int)MyEnumValue.Value1 : reader.GetInt32(MyEnumValueOrdinal),
 };
 ClassAList.Add(classA);
 }
 }
 return ClassAList;
 }
 }
 }

And below is the enum:-

public enum MyEnumValue 
{
 value1 =1,
 value2
}

• Andrey: so strange to see that pile of code in century of ORMs
• Ashish Gupta: Unfortunately we don't use any ORM at this point of time.
• Cylon Cat: Still, utter amazement that you aren't at least using a DataAdapter and DataTable. There's very little overhead in that, and it reduces the amount of code significantly. And as BlueMonkMN says, use nullables! Translating a DBNull to some "special value" is altering your data.

Answer 1

If you want your class to be able to properly maintain database values without loss, you should use nullable types for boolean, date, and probably enum values. Otherwise, if you send your data back to the database, you could be changing all null values to default values when you update data.

Also, wouldn't the code be a bit better if you used something like reader.IsDBNull() to check for null values?

• Cylon Cat: +Lots for emphasizing nullables in .NET types. Yes, use them for anything that is a value type.

Answer 2

a) That depends on what you want the application to do, if you have a valid default value (most use DateTime.Now) then use that, otherwise make it nullable.

b) If false is a valid default value the bool is fine, otherwise make it nullable. Use GetByte to get the enum and cast it to your enum type.

(MyEnumValue)reader.GetByte(MyEnumValueOrdinal)

Note: if any of these columns is not nullable in your database, don't make it nullable in your class and don't even check for DBNull.

• Ashish Gupta: +1 for "if any of these columns is not nullable in your database, don't make it nullable in your class and don't even check for DBNull."

Answer 3

Nullable properties are fine to use when they correspond to nullable columns in the database.

The following code is mostly equivalent to yours, but shorter:

MyString = reader["MyString"] is DBNull ? null : (string)reader["MyString"]

Answer 4

a) I would personally prefer nullable DateTime values returned as I feel this is a better abstraction of the state of the database field (rather than having to check for DateTime.MinValue).

b) Having false as a representation for DBNull may match your business logic, if not then I would go with a nullable type again.

Enum casting can catch you out -- you may want to validate the result using Enum.IsDefined(typeof(MyEnumValue), reader["MyEnumValue"])

In addition, I would define all the field names as string constants to avoid typing mistakes (and intellisense will give you a boost with auto-completion, as well).

Herbie

• Meff: Strangely he is grabbing the column ordinals from the strings, then not using them - Instead re-using the strings :s
• Meff: @Ashish, you're using them half the time, your line starts 'MyString = reader["MyString"]' and it should be 'MyString = reader[MyStringOrdinal]' so you do the numeric lookup on the column, rather than the string based lookup :)
• Ashish Gupta: yeah..that was because of copy paste...:-) I always intended to have ordinals not the column names. :-)

Answer 5

Nullable DateTime and Nullable Boolean are fine to use here, at the moment you assign false so there's no way for callers to know if it is meant to be false, or if it is unknown.

Also with the use of DateTime.MinValue it may appear to callers that something is on sale (As its 'Sell From' date is MinValue for example) when in fact it is NULL in the DB to prevent it being sold.

Note that you can also have Nullable Enums, but they are not great due to the constant .Value requests you'll need to make on it. So maybe this is preferable:

public enum MyEnumValue 
{
 ItWasNullInTheDB = 0,
 value1 =1,
 value2
}

Answer 6

Be carefull wit Enum as valid base types are byte, sbyte, short, ushort, int, uint, long, or ulong. Default type is int - Int32 so you are safe with your code for default enums. SQL Server does not support unsigned type so you only have to care for long - Int64.