Improving a piece of code

[Trueblue862]

Hi, I'm currently learning VBA, I've created this piece of code below. I'm looking for advice on ways to improve it. This is the first piece of code that I've written entirely without copying or modifying sections from someone else. This code works perfectly for my purposes, I'm just looking for advice on how to go about making it better for future reference. I appreciate any input on this.

Private Sub LabEL_upDate()

If tbxSloppy.Value = "" And tbxJade.Value = "" Then
    LabelE.Visible = True
    LabelE.Caption = "YES!!!"
    LabelE.BackColor = vbGreen
  Else
    LabelE.Visible = False
End If
    labeL_DaTe

End Sub
Private Sub labeL_DaTe()

If LabelE.Visible = True Then Exit Sub
If tbxSloppy.Value = "PERHAPS" And tbxJade.Value = "" Then
    LabelE.Visible = True
    LabelE.Caption = "MAYBE"
    LabelE.BackColor = vbYellow
  Else
    LabelE.Visible = False
End If
    labeL_DaTe2
End Sub
Private Sub labeL_DaTe2()

If LabelE.Visible = True Then Exit Sub
If tbxSloppy.Value <> "" Or tbxJade.Value <> "" Then
    LabelE.Visible = True
    LabelE.Caption = "NO :(  "
    LabelE.BackColor = vbRed
  Else
    LabelE.Visible = False
End If

End Sub

 

[CephasOz]

This is how I would code your subs:

Private Sub LabEL_upDate()
    With LabelE
        If ((tbxSloppy.Value = vbNullString) And (tbxJade.Value = vbNullString)) Then
            .Visible = True
            .Caption = "YES!!!"
            .BackColor = vbGreen
          Else
            .Visible = False
        End If
    End With
    labeL_DaTe
End Sub

Private Sub labeL_DaTe()
    With LabelE
        If (Not .Visible) Then
            If ((tbxSloppy.Value = "PERHAPS") And (tbxJade.Value = vbNullString)) Then
                .Visible = True
                .Caption = "MAYBE"
                .BackColor = vbYellow
              Else
                .Visible = False
            End If
            labeL_DaTe2
        End If
    End With
End Sub

Private Sub labeL_DaTe2()
    With LabelE
        If (Not .Visible) Then
            If ((tbxSloppy.Value <> vbNullString) Or (tbxJade.Value <> vbNullString)) Then
                .Visible = True
                .Caption = "NO :(  "
                .BackColor = vbRed
              Else
                .Visible = False
            End If
        End If
    End With
End Sub

1) Always use vbNullString instead of an empty string "" because each empty string requires space and reference overhead, while vbNullString is already declared.
2) Always use "With" - it both makes code clearer and faster (as the system doesn't have to work out what object it's dealing with now), as well as faster to code.
3) No need to test for "= True", for example "If A = True Then" is the same (but longer) as "If A Then".
4) A personal thing - I use blank lines between subs, but not within them, so it's visually easier to see the unity of a sub.
5) And another personal thing - I always use brackets around my tests, because I have coded in languages where assessment went from left to right, so "If A = B or C = D Then" would evaluate as "if A = B, then or that value with C, then test that for equality with D". By using brackets - no matter the language - I can be sure that the end result of the evaluation will be what I want.

Hope that helps.

 

[Trueblue862]

CephasOz, thank you for taking the time to reply, that all makes a lot of sense. Your code works perfectly, I'll try and make use of your tips going forward.

 

[CephasOz]

Glad to help.

 

[Fluff]

personally I'd keep the whole thing in one sub like

Private Sub LabEL_upDate()
   With LabelE
      .Visible = False
      If tbxSloppy.Value = "" And tbxJade.Value = "" Then
         .Visible = True
         .Caption = "YES!!!"
         .BackColor = vbGreen
      ElseIf tbxSloppy.Value = "PERHAPS" And tbxJade.Value = "" Then
         .Visible = True
         .Caption = "MAYBE"
         .BackColor = vbYellow
      ElseIf tbxSloppy.Value <> "" Or tbxJade.Value <> "" Then
         .Visible = True
         .Caption = "NO :(  "
         .BackColor = vbRed
      End If
   End With
End Sub

 

[Trueblue862]

Thanks Fluff, I originally tried to keep it all in the one sub, but I just couldn't get it to work, which is why I split it up. As soon I separated them out it worked for me, I never even thought of the "ElseIf" statement.

 

[Fluff]

You're welcome & thanks for the feedback

 

[CephasOz]

@Fluff does excellent work. When I looked at your code, I only really looked at the how, not the what as Fluff has done. If I might take a liberty with Fluff's code, you can also use Select like this:

Private Sub LabEL_upDate()
    With LabelE
        .Visible = False
        Select Case True
            Case ((tbxSloppy.Value = vbNullString) And (tbxJade.Value = vbNullString))
                .Visible = True
                .Caption = "YES!!!"
                .BackColor = vbGreen
            Case ((tbxSloppy.Value = "PERHAPS") And (tbxJade.Value = vbNullString))
                .Visible = True
                .Caption = "MAYBE"
                .BackColor = vbYellow
            Case ((tbxSloppy.Value <> vbNullString) Or (tbxJade.Value <> vbNullString))
                .Visible = True
                .Caption = "NO :(  "
                .BackColor = vbRed
        End Select
    End With
End Sub

 

[Fazza]

consider instead of somestring = vbnullstring
using len(somestring) = 0

 

[CephasOz]

@Fazza, that was an interesting comment. I would like to understand your thinking. Personally, I wouldn't do things that way, because it would mean that there would be a lot of literal values - the 0's - throughout my code that would have to be managed (see my comments about using ""). What I always do is to add a function to my projects like so:

' Is a string blank?
Function IsBlank(ByVal strVal As String) As Boolean
    IsBlank = (Trim(strVal) = vbNullString)
End Function

And then throughout my code I use it like so:

If IsBlank(strMyVar) Then

It has the benefit of enhancing readability and intent (always useful for maintenance).