optimizing this code in VBA

Question

Hi guys so I was browsing SO and one of the topics was "how to tell if a student coded this".. well I am a student working for a pretty big company as an intern. I recently coded a reporting tool for them in Access and have a question about a piece of code

    'get the dates'
'check if both date fields are filled out'
If A_AfterDateTxt.Value <> "" And A_BeforeDateTxt.Value <> "" Then
    'check if they are valid (ie, one date is not bigger then the other)'
    If CDate(A_AfterDateTxt.Value) > CDate(A_BeforeDateTxt.Value) Then
        MsgBox "You have selected invalid dates. Try again."
        GetSQLForActiveRecords = False   'this is the function name
        Exit Function  'exit the function'
    Else
        'this takes both fields and appends them to the sql statement'
        strSql = strSql & " AND ([Submitted] >= #" & A_AfterDateTxt.Value & "#  and [Submitted] <= #" & A_BeforeDateTxt.Value & "#)"
    End If
Else
    'one or both of them are blank, select the one thats entered
    If (SF_isNothing(A_AfterDateTxt.Value) And Not SF_isNothing(A_BeforeDateTxt.Value)) Then
        strSql = strSql & " AND ([Submitted] <= #" & A_BeforeDateTxt.Value & "#)"
    ElseIf (SF_isNothing(A_BeforeDateTxt.Value) And Not SF_isNothing(A_AfterDateTxt.Value)) Then
        strSql = strSql & " AND ([Submitted] >= #" & A_AfterDateTxt.Value & "#)"
    End If
End If

note: SI_isnothing is just a function that checks for a null/empty but since they data is from a textbox it will never be null right?

So there are two date textboxes (AfterDate, and BeforeDate). I build my SQL statement depending on what is filled out (ie, one is entered, the other empty)

So how can I modify this to make it more readable.

Answer 1

I cleaned up the code a bit. You can put the values from the textboxes in variables, that makes the code using the values less cumbersome.

'get the dates
Dim before As String = A_BeforeDateTxt.Value
Dim after As String = A_AfterDateTxt.Value
'check if both date fields are filled out
If after.Length > 0 And before.Length > 0 Then
    'check if they are valid (ie, one date is not bigger then the other)
    If CDate(after) > CDate(before) Then
        MsgBox "You have selected invalid dates. Try again."
        GetSQLForActiveRecords = False
        Exit Function
    Else
        'this takes both fields and appends them to the sql statement
        strSql = strSql & " AND ([Submitted] >= #" & after & "#  and [Submitted] <= #" & before & "#)"
    End If
Else
    'one or both of them are blank, select the one thats entered
    If (after.Length = 0 And before.Length > 0 Then
        strSql = strSql & " AND ([Submitted] <= #" & before & "#)"
    ElseIf before.Length = 0 And after.Length > 0 Then
        strSql = strSql & " AND ([Submitted] >= #" & after & "#)"
    End If
End If

You are right that a value that always is a string can't be Nothing. Checking for a condition that can't occur only makes the code more confusing, as it implies that the value could be something that it actually can't.

I used the Length property to check if the strings are empty. Comparing numbers is slightly more efficient than comparing strings, and it's also less prone to typos. You could accidentally write "'" instead of "", and that isn't easy to spot.

I removed some pointless comments. A comment should explain what needs explaining in the code, a comment that just literally tells what the code is doing only clutters up the code, like this one:

Exit Function  'exit the function'

The code could be rewritten to reuse the part where you add the conditions, so that you don't have that in three places. It would make the code a bit more complicated though, so it's questionable if it's worth it.

Answer 2

There is only ever going to be 4 possible 'states':

• both empty
• a valid
• b valid
• both are valid

With this in mind you could reduce your logic thus:

    Dim dx As Integer = 0

    If Not String.IsNullOrEmpty(txtBefore.Text) Then
        If IsDate(txtBefore.Text) Then
            dx += 1
        End If
    End If

    If Not String.IsNullOrEmpty(txtAfter.Text) Then
        If IsDate(txtAfter.Text) Then
            dx += 2
        End If
    End If

    Select Case dx
        Case 1
            'only before date is not empty and a valid date
        Case 2
            'only after date is not empty and a valid date
        Case 3
            'both are valid and not empty
    End Select

Please note this is vb.NET, I'm not sure how much of that translates to VBA

Answer 3

In general, combing multiple boolean evaluations into a single variable often improves readability.

If A_AfterDateTxt.Value <> "" And A_BeforeDateTxt.Value <> "" Then .....

becomes

Dim dateValuesPresent as Boolean = A_AfterDateTxt.Value <> "" And A_BeforeDateTxt.Value <> ""

If dateValuesPresent Then ....





If CDate(A_AfterDateTxt.Value) > CDate(A_BeforeDateTxt.Value) Then ....

becomes

Dim areValidDates as Boolean = CDate(A_AfterDateTxt.Value) > CDate(A_BeforeDateTxt.Value)

If areValidDates Then ....