Optimizing reporting tool

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开发者_JS百科 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.

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

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.

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 ....

Using the following function:

Private Function IsNullOrZLS(toCheck As Variant) As Boolean
IsNullOrZLS = True
If TypeName(toCheck) = "String" Then
    If Len(toCheck) > 0 Then IsNullOrZLS = False
End If
End Function

I would suggest the following:

Public Function GetSQLForActiveRecords() As Boolean
Dim whereClause As String, badDate As Boolean
Dim before As Date, after As Date

If Not IsNullOrZLS(A_BeforeDateTxt) Then
    If Not IsDate(A_BeforeDateTxt) Then
        MsgBox "Unable to parse date!"
        GetSQLForActiveRecords = False
        Exit Function
    End If
    before = CDate(A_BeforeDateTxt)
    whereClause = "[Submitted] <= #" & A_BeforeDateTxt.value & "#"
End If

If Not IsNullOrZLS(A_AfterDateTxt) Then
    If Not IsDate(A_AfterDateTxt) Then
        MsgBox "Unable to parse date!"
        GetSQLForActiveRecords = False
        Exit Function
    End If
    after = CDate(A_AfterDateTxt)
    If Len(whereClause) > 0 Then whereClause = whereClause & " AND "
    whereClause = "[Submitted] >= #" & A_AfterDateTxt.value & "#"
End If

If after <> 0 And before > after Then
    MsgBox "Invalid dates!"
    GetSQLForActiveRecords = False
    Exit Function
End If

GetSQLForActiveRecords = True

If Len(whereClause) = 0 Then Exit Function

strsql = strsql & " AND (" & whereClause & ")"
End Function

Some notes:

• This handles the possibility of an invalid date
• It's in VBA, not VB.NET
• As soon as an error is detected, exit the function. This helps avoid nested If-Then
• A Date variable which hasn't been initialized has a value of 0, which corresponds to December 30, 1899 12:00 AM. If that date is entered into the A_AfterDateTxt textbox, the code will treat it as empty.
• There must be some way to avoid repeating the date-parsing message.

You want readability? Like this:

Select Case True
Case Not IsDate(A_BeforeDateTxt.Value) And Not IsDate(A_AfterDateTxt.Value)
    MsgBox "You have selected invalid dates. Try again."
    GetSQLForActiveRecords = False   'this is the function name

Case A_AfterDateTxt.Value = ""
    strSql = strSql & " AND ([Submitted] <= #" & A_BeforeDateTxt.Value & "#)"

Case A_BeforeDateTxt.Value = ""
    strSql = strSql & " AND ([Submitted] <= #" & A_BeforeDateTxt.Value & "#)"

Case CDate(A_AfterDateTxt.Value) > CDate(A_BeforeDateTxt.Value)
    MsgBox "You have selected invalid dates. Try again."
    GetSQLForActiveRecords = False   'this is the function name

Case Else
    strSql = strSql & " AND ([Submitted] >= #" & A_AfterDateTxt.Value & "# and
      [Submitted] <= #" & A_BeforeDateTxt.Value & "#)"

End Select