A bit of a code smell



  •  Came across this today:

            /// <summary>
            ///        Determines whether or not the specified column exists in the specified data reader.
            /// </summary>
            /// <param name="reader"></param>
            /// <param name="columnName"></param>
            /// <returns></returns>
            protected bool ColumnExists(IDataReader reader, string columnName) {
                try {
                    //causes an IndexOutOfRangeException if columName does not exist
                    reader.GetOrdinal(columnName);
                }
                catch (IndexOutOfRangeException) {
                    return false;
                }
                return true;
            }

    This is used like this:

    if (ColumnExists(reader, ColumnNameDescription))

    {
        entity.Description = reader[ColumnNameDescription].ToString();
    }
    if (ColumnExists(reader, ColumnNameFullImagePath))

    {
        entity.FullImagePath = reader[ColumnNameFullImagePath].ToString();
    }

    *** REPEAT FOR 200 MORE LINES TO COVER THE REST OF THE COLUMNS IN THE RETURNED VIEW ***

     Anyone see any WTF worthy issues with this? I see more than one :)


  • Considered Harmful

    1) Exceptions for flow control.
    2) Doesn't check for null or DBNull.
    3) Why would it ever be uncertain if a certain column was selected?
    4) That method doesn't use any instance members and should probably be static.
    5) 4 lines each for what should be single-line if statements.
    6) XML comments aren't fully filled-in.

    I doubt the connection is closed inside a finally block but that can neither be confirmed nor denied from this.

     What did I miss?



  • @joe.edwards@imaginuity.com said:

    1) Exceptions for flow control.

    We have a winner, I can't imagine how slow this is if columns are missing. 

    @joe.edwards@imaginuity.com said:


    2) Doesn't check for null or DBNull.

    And 2 points!

    @joe.edwards@imaginuity.com said:


    3) Why would it ever be uncertain if a certain column was selected?

    Crummy database/sproc design

    @joe.edwards@imaginuity.com said:


    4) That method doesn't use any instance members and should probably be static.

    To be fair to the original authors, they went for the enterprisy style. The check column method is a member of an abstract base entity class, that all data object entities inherit from. So its fine that its in instance method, and not static.

    @joe.edwards@imaginuity.com said:


    5) 4 lines each for what should be single-line if statements.
    6) XML comments aren't fully filled-in.

    Semantics.... I don't like compact code, and I don't usually fill in each parameter on the comments. Some are kinda obvious :)

    @joe.edwards@imaginuity.com said:


    I doubt the connection is closed inside a finally block but that can neither be confirmed nor denied from this.

     

    Actually it is.

     



  • To add to this, why even have the function to begin with?  Why not just have a try/catch around your processing and if one of the fields don't exist just catch the IndexOutOfRangeException.  If an expected field isn't there, how valid is the rest of your data, just throw an exception and get out.



  • OK my turn.

    • ColumnExists method shouldn't be static (or used like a static method like it is now).  Make it a method of IDataReader.
    • Seems to me like it's possible (and likely) 1 or more columns would not exist, so you can't just fail if one of them isn't found.
    • In fact, this whole design seems crazy.  Can't you do something like reader.getColumns() that returns a Map of column name -> value?
    • Or, if you can't do that, maybe the Column objects (if they exist) can modify the entity object directly?
        foreach( Column c in reader.getColumns() ) c.modifyEntity(entity);

     

    @Jonathan Holland said:

     Came across this today:

            /// <summary>
            ///        Determines whether or not the specified column exists in the specified data reader.
            /// </summary>
            /// <param name="reader"></param>
            /// <param name="columnName"></param>
            /// <returns></returns>
            protected bool ColumnExists(IDataReader reader, string columnName) {
                try {
                    //causes an IndexOutOfRangeException if columName does not exist
                    reader.GetOrdinal(columnName);
                }
                catch (IndexOutOfRangeException) {
                    return false;
                }
                return true;
            }

    This is used like this:

    if (ColumnExists(reader, ColumnNameDescription))

    {
        entity.Description = reader[ColumnNameDescription].ToString();
    }
    if (ColumnExists(reader, ColumnNameFullImagePath))

    {
        entity.FullImagePath = reader[ColumnNameFullImagePath].ToString();
    }

    *** REPEAT FOR 200 MORE LINES TO COVER THE REST OF THE COLUMNS IN THE RETURNED VIEW ***

     Anyone see any WTF worthy issues with this? I see more than one :)

     


  • @Outlaw Programmer said:

    OK my turn.

    • ColumnExists method shouldn't be static (or used like a static method like it is now).  Make it a method of IDataReader.
    • Seems to me like it's possible (and likely) 1 or more columns would not exist, so you can't just fail if one of them isn't found.
    • In fact, this whole design seems crazy.  Can't you do something like reader.getColumns() that returns a Map of column name -> value?
    • Or, if you can't do that, maybe the Column objects (if they exist) can modify the entity object directly?
        foreach( Column c in reader.getColumns() ) c.modifyEntity(entity);

     

     <hints id="hah_hints"></hints>

    How are the developers supposed to "make it a method" of a framework interface?  I guess you could write an extension method, but those only exist in .NET 3.5.

    For the rest, you're sort of right, I guess.  The non-retarded way of doing this with an IDataReader (if you truly only have access to the IDataReader and truly don't know what fields will actually be present at runtime) is to use its GetSchemaTable method.  You could also enumerate the fields using IDataRecord.FieldCount and IDataRecord.GetName(int i).  Might be slower or faster depending on the provider and the number of fields.



  • @Aaron said:

    @Outlaw Programmer said:

    OK my turn.

    • ColumnExists method shouldn't be static (or used like a static method like it is now).  Make it a method of IDataReader.
    • Seems to me like it's possible (and likely) 1 or more columns would not exist, so you can't just fail if one of them isn't found.
    • In fact, this whole design seems crazy.  Can't you do something like reader.getColumns() that returns a Map of column name -> value?
    • Or, if you can't do that, maybe the Column objects (if they exist) can modify the entity object directly?
        foreach( Column c in reader.getColumns() ) c.modifyEntity(entity);

     

     

    How are the developers supposed to "make it a method" of a framework interface?  I guess you could write an extension method, but those only exist in .NET 3.5.

    For the rest, you're sort of right, I guess.  The non-retarded way of doing this with an IDataReader (if you truly only have access to the IDataReader and truly don't know what fields will actually be present at runtime) is to use its GetSchemaTable method.  You could also enumerate the fields using IDataRecord.FieldCount and IDataRecord.GetName(int i).  Might be slower or faster depending on the provider and the number of fields.

    Since IDataReader is just an interface, there is nothing stopping you from creating your own implementation that contains ColumnExists.

    Also, this is .NET 1.1 code, so there was no GetSchema method at the time. 

     



  • Sorry, I'm not a .NET guy, just figured that these were your own classes. 



  • I ran across the same thing a couple days ago.  I shivered.


Log in to reply