6
\$\begingroup\$

I need to call the function that will get the value of a datatable cell with the ColIndex, either a text (column name) or an integer (columnindex).

To avoid writing two functions that, I wrote this code:

Public Function DTableCellValue(ByRef RowData As DataRow, _
                                    ByVal ColIndex As Object, _
                                    Optional ByVal DefaultVal As String = "") As Object
        Try
            Dim ret As Object = RowData.Item(ColIndex)

            If IsDBNull(ret) Then
                Return DefaultVal
            Else
                Return ret
            End If
        Catch ex As Exception
            Throw
        End Try
    End Function

Can you tell me if it uses bad coding practices or is potentially problematic?

\$\endgroup\$
1
  • \$\begingroup\$ What do you mean by "issues"? Do you mean will it run? Is it bad coding practice? Might it cause problems down the line? Try and be as specific as you can about what your question is. \$\endgroup\$
    – Kaz
    Commented Aug 27, 2015 at 8:36

2 Answers 2

7
\$\begingroup\$

If you use this method quit often by passing an Integer you will get performance problems because of the boxing and unboxing of the struct.

My advice would be to write two methods one using an Integer and the other using a String as argument.


Based on the naming guidelines you should name methods by using a verb or a verb phrase. DTableCellValue looks by its name more as a property than a method.

Based on the same guidelines method arguments should be named using camelCase casing. Although VB.NET isnt case sensitive I would like to suggest to follow these guidelines.


The try..catch is in its current form senseless because if an exception is thrown you are just rethrowing it.


A construct like

If condition Then
    Return Value
Else
    Return otherValue
End If  

can be simplified by removing the redundant else like so

If condition Then
    Return Value
End If 
Return otherValue

because if the condition is true the else won't be reached.


Implementing the mentioned points (leaving aside the method name) leads to two methods like so

Public Function DTableCellValue(ByRef rowData As DataRow, _
                                    ByVal colIndex As Integer, _
                                    Optional ByVal defaultVal As String = "") As Object
    Dim ret As Object = rowData.Item(colIndex)

    If IsDBNull(ret) Then
        Return defaultVal
    End If

    Return ret

End Function

Public Function DTableCellValue(ByRef rowData As DataRow, _
                                    ByVal colIndex As String, _
                                    Optional ByVal defaultVal As String = "") As Object
    Dim ret As Object = rowData.Item(colIndex)

    If IsDBNull(ret) Then
        Return defaultVal
    End If

    Return ret

End Function
\$\endgroup\$
4
  • \$\begingroup\$ I don't think a generic method would work since String is a noninheritable class. \$\endgroup\$
    – RubberDuck
    Commented Aug 27, 2015 at 17:32
  • 1
    \$\begingroup\$ Or rather, you could, but you couldn't put a proper type constraint on it. \$\endgroup\$
    – RubberDuck
    Commented Aug 27, 2015 at 17:35
  • \$\begingroup\$ i did, making colindex and object variable instead of a type contraint \$\endgroup\$
    – Smith
    Commented Aug 30, 2015 at 10:35
  • \$\begingroup\$ @Heslacher what are your suggestions for names for this function \$\endgroup\$
    – Smith
    Commented Sep 11, 2015 at 21:09
4
\$\begingroup\$

I'm a little late to the party so I will not repeat what @Heslacher said in his/her review.


A method that doesn't use any members from a class instance should be static (shared). If the method reside in a module (not a class) then you're fine because the method is static (shared) by default.

Now, since it looks like you're creating an extension method, we'll put the method in a module so we can append the Extension attribute.

Public Module DataRowExtensions

    <Extension()>
    Public Function DTableCellValue(...) As Object

Microsoft provide a .net assembly named System.Data.DataSetExtensions.dll which extend some of the classes located in the the System.Data namespace. One very related class is the DataRowExtensions class and its `Field<T> methods. It's almost identical to your method except the ability to provide a default value.

So I suggest we use the same name (method and parameters) and make it generic.

Public Function Field(Of T)(row As DataRow, columnIndex As Integer, Optional ByVal defaultValue As T = Nothing) As T

The DataRow class has a method named IsNull which accepts both a string (column name) and an integer (column index). Combine this with the short-circuit ternary if operator and the return statement becomes an easy to read one-liner.

Return If(row.IsNull(columnIndex), defaultValue, row.Item(columnIndex))

Apply all the suggestions and the code will look like this:

Imports System.Runtime.CompilerServices

Public Module DataRowExtensions

    <Extension()>
    Public Function Field(Of T)(row As DataRow, columnIndex As Integer, Optional ByVal defaultValue As T = Nothing) As T
        Return If(row.IsNull(columnIndex), defaultValue, row.Item(columnIndex))
    End Function

    <Extension()>
    Public Function Field(Of T)(row As DataRow, columnName As String, Optional ByVal defaultValue As T = Nothing) As T
        Return If(row.IsNull(columnName), defaultValue, row.Item(columnName))
    End Function

End Module

Usage

Dim firstName As String = table.Rows(0).Field(Of String)(0, "(null)")
Dim firstName As String = table.Rows(0).Field(Of String)("firstName", "(null)")

Inferred by the type of the default value:

Dim firstName As String = table.Rows(0).Field(0, "(null)")
Dim firstName As String = table.Rows(0).Field("firstName", "(null)")
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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