As of now, we have two different styles of object intializations based on SQL row in our codebase:
public Config(SqlDataReader sdr) {
for (int i = 0; i < sdr.FieldCount; i++) if (!sdr.IsDBNull(i)) {
switch (sdr.GetName(i)) {
case 'Id': Id = sdr.GetInt32(i); break;
...
}
}
}
and
public User(SqlDataReader sdr)
{
UserId = sdr.GetInt32(0);
if (!sdr.IsDBNull(1)) Name = sdr.GetString(1);
...
}
They are called like this:
while(sdr.Read()) users.Add(new User(sdr));
C# coding guide says that style should be consistent, so one of them has to change to the other, if we don’t find a reason to keep them different. So, before we implement the whole project with its 50+ models, we need a design guideline regarding this.
Both designs have their drawbacks, but I don’t know how to make it better:
- In the first, no one will notice until later in the code if required fields are dropped from the query.
- The second one will fail whenever the fields in the query are reordered.
Which one should we prefer, or how could we improve them to address the drawbacks? Are they really drawbacks, in terms of code maintenance/reuse?
1
Both options have drawbacks: the first depends on the “magic string” of the column name, the second depends on the “magic number” of the column index. A good suite of automated unittests could cover either usage and help a great deal in preventing breaking changes though…
I would personally favor the first for a number of reasons, but they are probably very personal. (Actually, I would prefer NOT to write this kind of code and use an OR/M but that does not make this question any less valid)
Searchable field names
Field names can be found using Find in Files. ‘Id’ is obviously very generic, but I imagine if you want to remove a column called ‘OrderPaymentTerm’ you could easily look for the usages in the code. Or even better, put the field names in constants that are part of the class that creates the query. That way, field names and usages are right beside each other.
Required field validation is easy
If you want to make sure all the fields are present in the query, simply keep a list of fields that you read from the SqlDataReader and throw an exception if a field is missing.
Column order should not matter
Nobody expects a change in the ordering of columns in a SELECT clause to be a breaking change. And if you use SELECT * FROM, you can’t control the ordering of the columns anyway.
Removing a column is less painful
If you remove the sixth column in a ten-column SELECT, you have to update all the indexes in the second case. Painful, and prone to error.