CODEWITHSUNDEEP
c#ado.net
Ashish Gupta
Ashish Gupta

Reputation: 15139

Code review - Retrieving data from SqlDataReader

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
}

Upvotes: 2

Views: 2804

Answers (6)

Dr Herbie
Dr Herbie

Reputation: 3940

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

Upvotes: 1

Matej
Matej

Reputation: 7627

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.

Upvotes: 0

VladV
VladV

Reputation: 10389

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

Upvotes: 2

MrFox
MrFox

Reputation: 5136

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.

Upvotes: 3

Meff
Meff

Reputation: 5999

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
}

Upvotes: 0

BlueMonkMN
BlueMonkMN

Reputation: 25601

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?

Upvotes: 3

Related Questions