Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following codes, which I created as extension for datatable in my project. It actually worked. Just wonder if there is any optimization can be made through. Thanks. =)

<Extension()>
Public Function HasNull(ByVal dataTable As DataTable) As Boolean
    For Each column As DataColumn In dataTable.Columns
        If dataTable.Rows.OfType(Of DataRow)().Any(Function(r) r.IsNull(column)) Then
            Return True
        End If
    Next
    Return False
End Function

<Extension()>
Public Function SetDefaultForNull(ByVal dataTable As DataTable) As DataTable
    For Each row As DataRow In dataTable.Rows
        For Each col As DataColumn In dataTable.Columns
            Dim value As Object = row(col)
            If IsDBNull(value) Then
                Dim dataType As String = col.DataType.ToString

                Select Case dataType
                    Case "System.DateTime"
                        value = New DateTime
                    Case "System.Decimal", "System.Int16", "System.Int32", "System.Int64"
                        value = 0
                    Case "System.String"
                        value = String.Empty
                    Case "System.Boolean"
                        value = False
                    Case Else
                        value = 0
                End Select
                row(col) = value
            End If
        Next
    Next
    Return dataTable
End Function
share|improve this question
    
I'm rolling back the last edit as it invalidates my answer. Please see this meta post. For an iterative review, is it ok to edit my question to include updated code?. –  RubberDuck Jul 25 at 15:44
    
Welcome to CR @chinz. We're not trying to be mean, we're glad you're here. We just have some rules. Feel free to drop by chat if you have any questions about how the site works. –  RubberDuck Jul 25 at 15:46
    
It ok @ckuhn203, in the process of learning. Apologize for that and thanks for pointing out. =] –  chinz Jul 25 at 15:47

2 Answers 2

A couple of small points.

  • Single letter identifiers and variable names are the devil incarnate. Replace r with something meaningful.
  • There's not much point in using a variable for col.DataType.ToString. You only use the variable dataType once, so it would be perfectly acceptable to get rid of a line of a code and do this instead.

    Select Case col.DataType.ToString
        Case "System.DateTime"
            value = New DateTime
    
  • I also don't see much benefit to the value variable. You could just set row(col) = to whatever you would set value to.

  • There's no reason to explicitly check for Case "System.Decimal", "System.Int16", "System.Int32", "System.Int64". It's taken care of in the Else case.

    Select Case col.DataType.ToString
        Case "System.DateTime"
            row(col) = New DateTime
        Case "System.String"
            row(col) = String.Empty
        Case "System.Boolean"
            row(col) = False
        Case Else
            row(col) = 0
    End Select
    
share|improve this answer
2  
Single letter identifiers and variable names are the devil incarnate I saw someone invoke the devil by using a script with single letter only, it was horrible! –  Marc-Andre Jul 25 at 15:27

Credit to @Tim Schmelter:

According to the first method: I would handle the case that the input table is Nothing, either throw an ArgumentNullException or return True/False.

Also another minor improvement: i would loop the rows instead of the columns. Consider that all columns are non-null except the last. You would loop all rows for all column before you detect a null. If you loop the rows you'd determined that already in the first row(if the last column is full of nulls).

So something like this:

<Extension()>
Public Function HasNull(dataTable As DataTable) As Boolean
    If dataTable Is Nothing Then
        Throw New ArgumentNullException("dataTable must be initialized", "dataTable")
    End If
    Dim allColumns = dataTable.Columns.Cast(Of DataColumn).ToList() ' materialize
    Dim hasNullField As Boolean = dataTable.AsEnumerable().
        Any(Function(row) allColumns.Any(Function(c) row.IsNull(c)))
    Return hasNullField
End Function

The second method could use this extension instead to get the default value of all types:

<Extension()>
Public Function GetDefaultValue(t As Type) As Object
    If t.IsValueType Then
        Return Activator.CreateInstance(t)
    Else
        Return Nothing
    End If
End Function

The second method itself could then be implemented in this way(make it a Sub):

<Extension()>
Public Sub SetDefaultForNull(dataTable As DataTable)
    For Each row As DataRow In dataTable.Rows
        For Each col As DataColumn In dataTable.Columns
            If row.IsNull(col) Then
                row.SetField(col, col.DataType.GetDefaultValue())
            End If
        Next
    Next
End Sub

Tested with this sample data:

Dim table As New DataTable
table.Columns.Add("ID", GetType(Int32))
table.Columns.Add("Name", GetType(String))
table.Columns.Add("Date", GetType(DateTime))
table.Rows.Add(1, "test", DateTime.Now)
table.Rows.Add(DBNull.Value, Nothing, Nothing)
table.Rows.Add(Nothing, DBNull.Value, DBNull.Value)
If table.HasNull() Then
    table.SetDefaultForNull()
End If

Result:

1       test    25.07.2014 15:05:29
0               01.01.0001 00:00:00
0               01.01.0001 00:00:00
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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