10
\$\begingroup\$

I've the following code, which checks if one column belongs to the list I provide. After that, it takes the value contained in it.

I've tried and it's 9 times faster than raising and handling an exception. But I don't find this code very readable.

For example, I don't need to define a variable drsIDAgentClient. I've thought of using an extension method over reader and provide a ColumnExist(string columnName) method.

Can you see a better solution?

DataRow[] drsIDAgentClient = reader.GetSchemaTable().Select("ColumnName = 'IDAgentClient'");
if (drsIDAgentClient.Length > 0)
    coupon.IdAgentClient = reader["IDAgentClient"] == DBNull.Value ? null : (int?)reader["IDAgentClient"];
\$\endgroup\$
  • 4
    \$\begingroup\$ What you can and cannot do after receiving answers \$\endgroup\$ – Vogel612 Sep 30 '14 at 13:53
  • \$\begingroup\$ @Vogel612: ok.. but I really think the meaning wasn't changed. And it sound a bit weird to lose the ownership of a question after posting it online (moderator gets the right to decide at my place on a content I wrote, it's not very intuitive). But I strongly believe in this community and I appreciate especially the work you are doing here on code review. \$\endgroup\$ – Revious Sep 30 '14 at 15:34
  • 1
    \$\begingroup\$ I am not a moderator. Also it's more unintuitive to see code in a question and have answers relate to different code IMO.. \$\endgroup\$ – Vogel612 Sep 30 '14 at 17:51
  • \$\begingroup\$ @Vogel612: ok.. I've answered myself.. \$\endgroup\$ – Revious Sep 30 '14 at 20:36
  • \$\begingroup\$ @Revious Questions and answers on the Stack Exchange sites are owned by the community as a whole. The asker gets some limited extra privileges (able to edit/comment on their question regardless of rep; able to delete if there's no answers; getting to select a correct answer). Everything else, however, is owned by the community as a whole, hence why users of sufficient reputation can suggest edits or edit outright. \$\endgroup\$ – Chris Hayes Oct 1 '14 at 5:57
13
\$\begingroup\$

Can you see a better solution?

Yes use braces {} also for single line statements of an if condition.

Using .Any() instead of checking .Length > 0 is faster but only for an IEnumerable

I also would enclose reader["IDAgentClient"] == DBNull.Value in () as it is more readable or even better you can introduce a boolean isDbNullValue

DataRow[] drsIDAgentClient = reader.GetSchemaTable().Select("ColumnName = 'IDAgentClient'");
if (drsIDAgentClient.Length > 0)
{
    Boolean isDbNullValue = (reader["IDAgentClient"] == DBNull.Value);
    coupon.IdAgentClient = isDbNullValue  ? null : (int?)reader["IDAgentClient"];
}  

After rereading your question it seems that you want to know if a column with a specific name exists in the SchemaTable.

So you can use instead of the code above simply this

String columnName = "IDAgentClient";
if (reader.GetSchemaTable().Columns.Contains(columnName))
{
    Boolean isDbNullValue = (reader[columnName] == DBNull.Value);
    coupon.IdAgentClient = isDbNullValue  ? null : (int?)reader[columnName];
}  

I would like to expand @Malachi's answer with not using a tenary

Malachi's check

bool IsColumnEmpty(string columnName)
{
    return reader.GetSchemaTable().Columns.Contains(columnName)) 
        ? Reader[columnName] == DBNull.Value 
        : true;
}  

can be rewritten like

bool ColumnHasValue(string columnName)
{
    return reader.GetSchemaTable().Columns.Contains(columnName) && 
           Reader[columnName] != DBNull.Value;        
}  

and if you really need to use an extension method you can do this like

public static class HelperExtension
{
    public static bool ColumnHasValue(this IDataReader reader, string columnName)
    {
        return reader.GetSchemaTable().Columns.Contains(columnName) && 
               Reader[columnName] != DBNull.Value; 
    }
}  

and call it

coupon.IdAgentClient = reader.ColumnHasValue(columnName) ? (int?)reader[columnName] : null;
\$\endgroup\$
  • 3
    \$\begingroup\$ If I remember correctly, .Length is faster than Any because Length is already calculated versus Any which has to do calculations. Any is faster than Count() > 0 though \$\endgroup\$ – IEatBagels Sep 30 '14 at 13:24
  • 2
    \$\begingroup\$ @TopinFrassi Yes, for a array you are right. Updated my answer. \$\endgroup\$ – Heslacher Sep 30 '14 at 13:25
  • \$\begingroup\$ And always remember, Yes use braces {} also for single line statements of an if condition. is only an opinion. It can be helpful, but it is not part of every coding standard. \$\endgroup\$ – Magus Oct 1 '14 at 14:42
5
\$\begingroup\$

Heslacher did a great job cleaning it up, I still think it could still be cleaned up more though:

String columnName = "IDAgentClient";
bool columnEmpty = IsColumnEmpty(columnName);

if (columnEmpty)
{
    coupon.IdAgentClient = null;
}
else
{
    coupon.IDAgentClient = (int?)reader[columnName];
}

Then have a separate method:

bool IsColumnEmpty(string columnName)
{
    if (reader.GetSchemaTable().Columns.Contains(columnName))
    {
        return reader[columnName] == DBNull.Value;
    }
    return true;
}

Arguably you could also separate out checking if it exists and if it's empty.

\$\endgroup\$
  • \$\begingroup\$ Hey Steve I extended your answer a little bit....I hope you don't mind... \$\endgroup\$ – Malachi Sep 30 '14 at 15:11
  • 1
    \$\begingroup\$ I don't mind at all! I personally prefer not to use ternary operators but it's a good idea to expose people to everything and let them decide :) \$\endgroup\$ – Steve Michael Sep 30 '14 at 15:31
5
\$\begingroup\$

I am going to extend Steve's answer a little bit as I see something that would make his code short and to the point. I think Heslacher hit on Ternary statements as well.

in Steve's answer he has this

String columnName = "IDAgentClient";
bool columnEmpty = IsColumnEmpty(columnName);

if (columnEmpty)
{
    coupon.IdAgentClient = null;
}
else
{
    coupon.IDAgentClient = (int?)reader[columnName];
}

that utilizes this method

bool IsColumnEmpty(string columnName)
{
    if (reader.GetSchemaTable().Columns.Contains(columnName))
    {
        return reader[columnName] == DBNull.Value;
    }
    return true;
}

first thing is inside of the boolean method

I think you can utilize a Ternary here as well.

bool IsColumnEmpty(string columnName)
{
    return reader.GetSchemaTable().Columns.Contains(columnName)) 
        ? Reader[columnName] == DBNull.Value 
        : true;
}

and then with Steve's main bit of code you don't need to actually declare a boolean variable or that bulky if/then statement either, use a ternary like this.

String columnName = "IDAgentClient";
coupon.IdAgentClient = IsColumnEmpty(columnName) 
    ? null
    : (int?)reader[columnName];

If you understand ternary operators, then this is pretty clear and straightforward.

Again this is only looking at Steve's answer...

\$\endgroup\$
1
\$\begingroup\$

I've thought of using Extension Methods to being near to SRP and splitting the source code in a reusable part plus a specific part.

if(reader.ColumnExists("TournamentName"))
{
   odd.Championship = (string)reader["TournamentName"];
}

public static class HelperExtension
{
    public static bool ColumnExists(this IDataReader reader, string columnName)
    {
        DataRow[] rows = reader.GetSchemaTable().Select(string.Format("ColumnName = '{0}'", columnName));
        return rows.Length > 0;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.