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
}

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. — Cylon Cat
– Cylon Cat, Commented Jun 25, 2010 at 11:37

BlueMonkMN · Accepted Answer · 2010-06-25 11:31:14Z

3

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?

answered Jun 25, 2010 at 11:31

BlueMonkMN

25.7k11 gold badges86 silver badges151 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Cylon Cat Over a year ago

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

MrFox · Accepted Answer · 2010-06-25 11:35:00Z

3

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.

answered Jun 25, 2010 at 11:35

MrFox

5,1849 gold badges48 silver badges83 bronze badges

1 Comment

Ashish Gupta Over a year ago

+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."

VladV · Accepted Answer · 2010-06-25 11:35:50Z

2

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"]

answered Jun 25, 2010 at 11:35

VladV

10.5k3 gold badges36 silver badges48 bronze badges

Comments

Dr Herbie · Accepted Answer · 2010-06-25 14:56:43Z

1

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

edited Jun 25, 2010 at 14:56

answered Jun 25, 2010 at 11:31

Dr Herbie

3,9401 gold badge29 silver badges28 bronze badges

3 Comments

Meff Over a year ago

Strangely he is grabbing the column ordinals from the strings, then not using them - Instead re-using the strings :s

Meff Over a year ago

@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 Over a year ago

yeah..that was because of copy paste...:-) I always intended to have ordinals not the column names. :-)

Meff · Accepted Answer · 2010-06-25 11:32:16Z

0

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
}

answered Jun 25, 2010 at 11:32

Meff

5,99930 silver badges36 bronze badges

Comments

Matej · Accepted Answer · 2010-06-25 11:36:35Z

0

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.

answered Jun 25, 2010 at 11:36

Matej

7,6372 gold badges38 silver badges46 bronze badges

Collectives™ on Stack Overflow

Code review - Retrieving data from SqlDataReader

6 Answers 6

1 Comment

1 Comment

Comments

3 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

1 Comment

1 Comment

Comments

3 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related