оптимизировать этот код в VBA

Question

Привет, ребята, так что я просматривал SO, и одной из тем было «как узнать, кодировал ли это студент» ... ну, я студент, работающий в довольно большой компании в качестве стажера. Недавно я написал для них инструмент отчетности в Access, и у меня возник вопрос о куске кода

    '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

примечание: SI_isnothing - это просто функция, которая проверяет наличие нулевого / пустого значения, но поскольку данные получены из текстового поля, оно никогда не будет равно нулю, верно?

Итак, есть два текстовых поля даты (AfterDate и BeforeDate). Я строю свой SQL-запрос в зависимости от того, что заполнено (т. Е. Один введен, а другой пуст)

Так, как я могу изменить это, чтобы сделать его более читабельным.

Answer 1

Существует только 4 возможных состояния:

• оба пустые
• действительный
• b действует
• оба действительны

Имея это в виду, вы можете уменьшить свою логику таким образом:

    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

Обратите внимание, что это vb.NET, я не уверен, сколько из этого переводится на VBA

Answer 2

Как правило, объединение нескольких логических оценок в одну переменную часто улучшает читабельность.

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

Answer 3

Я немного почистил код. Вы можете поместить значения из текстовых полей в переменные, что делает код, использующий значения, менее громоздким.

'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

Вы правы, что значение, которое всегда является строкой, не может быть Nothing. Проверка на условие, которое не может произойти, только делает код более запутанным, поскольку подразумевает, что значение может быть чем-то, чего он на самом деле не может.

Я использовал свойство Length, чтобы проверить, являются ли строки пустыми. Сравнение чисел немного более эффективно, чем сравнение строк, и оно также менее подвержено опечаткам. Вы можете случайно написать «» вместо «», и это не так легко определить.

Я удалил несколько бессмысленных комментариев. Комментарий должен объяснять, что нужно объяснять в коде, комментарий, который буквально говорит о том, что код делает, только загромождает код, как этот:

Exit Function  'exit the function'

Код можно переписать для повторного использования части, в которую вы добавляете условия, чтобы в трех местах этого не было. Хотя это немного усложнит код, поэтому сомнительно, стоит ли оно того.

Answer 4

Вы хотите читабельность?Как это:

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

Answer 5

Используя следующую функцию:

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

Я бы предложил следующее:

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

Некоторые примечания:

• Это обрабатывает возможностьнедействительная дата
• Это в VBA, а не в VB.NET
• Как только обнаружена ошибка, выйдите из функции.Это помогает избежать вложенного If-Then
• Переменная Date, которая не была инициализирована, имеет значение 0, что соответствует 30 декабря 1899 года в 12:00.Если эта дата введена в текстовое поле A_AfterDateTxt, код будет обрабатывать ее как пустую.
• Должен быть какой-то способ избежать повторения сообщения о разборе даты.