11
\$\begingroup\$

I am using a public validating sub to help validate the other subs. Is this the most efficient way to do this?

Public Sub Validator(sender As Object, e As EventArgs) Handles GrossRevenueButton.Click, CPTCodesButton.Click, EstablishedVisitsButton.Click
    Dim validater As Integer = 0 'this is the variable that gets sent out from this sub
    If MonthSelectBox.Text = "" Then
        Label1.Text = "You didn't select a month!"
        validater = 1
    Else
        Label1.Text = ""
    End If
    If YearSelectBox.Text = "" Then
        Label2.Text = "You didn't select a year!"
        validater = 2
    Else
        Label2.Text = ""
    End If
    If FilepathTextBox.Text = "" Then
        Label3.Text = "You didn't select a file!"
        validater = 3
    Else
        Label3.Text = ""
    End If

    If MonthSelectBox.Text.Length > 1 And YearSelectBox.Text.Length > 1 And FilepathTextBox.Text.Length > 1 Then
        validater = 0
    End If
End Sub



'GROSS REVENUE
Sub Button1_Click(sender As System.Object, e As System.EventArgs) Handles GrossRevenueButton.Click
    Dim validater As Integer 'validater comes from the validate sub
    If validater > 0 Then
        Call Gross_Revenue()
    Else
        Label4.Text = "Something went wrong!"
    End If
End Sub

'CPT CODES
Sub Button3_Click(sender As System.Object, e As System.EventArgs) Handles CPTCodesButton.Click
    Dim validater As Integer 'validater comes from the validate sub
    If validater > 0 Then
        Call CPT_Totals()
    Else
        Label4.Text = "Something went wrong!"
    End If
End Sub

'ESTABLISHED VISTS
Sub Button4_Click(sender As System.Object, e As System.EventArgs) Handles EstablishedVisitsButton.Click
    Dim validater As Integer 'validater comes from the validate sub
    If validater > 0 Then
        Call Established_Visits()
    Else
        Label4.Text = "Something went wrong!"
    End If
End Sub
\$\endgroup\$
  • \$\begingroup\$ How does the validater variable come from the Validator Sub? I don't see where it gets "sent out". The validater variable is deimensioned as a local within the Validator method so it is not accessible outside of it. Also, the Click handlers also dimension their own instance of a validater variable and don't re-use it. Also, is the Established_Visits method important to have listed? I don't see it in the above code. \$\endgroup\$ – xDaevax Oct 9 '14 at 13:41
  • \$\begingroup\$ Ah yes, so validater is always 0 in this instance. I was under the impression that it was either 1, 2 or 3 if the fields were blank. Yes, Established_Visits is in the rest of the code, but it's the same as the above. Just with a different name. So how do I send the value of validater? \$\endgroup\$ – FoxMcCloud Oct 9 '14 at 13:47
11
\$\begingroup\$

In your case it seems like you want to have a re-usable validation method that validates a common part of your form.

Step 1

I wouldn't bind the clicks of the buttons to multiple handlers. This makes the code difficult to debug and can make it unpredictable (which fires first?). Instead, bind the click event to one handler per button, and perform all of the actions you want in that handler.

Step 2

Sometimes you may want to have some functionality shared among event handlers. In this case, I would create a separate function that returns the value based on the common validation routine, and call this from the separate event handlers.

Here is an example:

    Public Function ValidateCommonFields() As Integer
        Dim returnValue As Integer = -1

        If String.IsNullOrWhiteSpace(MonthSelectBox.Text) Then
            Label1.Text = "You didn't select a month!"
            returnValue = 1
        Else
            Label1.Text = ""
        End If

        If String.IsNullOrWhiteSpace(YearSelectBox.Text) Then
            Label2.Text = "You didn't select a year!"
            returnValue = 2
        Else
            Label2.Text = ""
        End If
        If String.IsNullOrWhiteSpace(FilepathTextBox.Text) Then
            Label3.Text = "You didn't select a file!"
            returnValue = 3
        End If

        Return returnValue
    End Function

    Sub Button1_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles GrossRevenuButton.Click
        Dim validationStatus As Integer = Me.ValidateCommonFields()
        If validationStatus < 0 Then
            Gross_Revenue()
        Else
            Label4.Text = "Something went wrong!"
        End If
    End Sub

A couple of other style points:

At the end of your Validator method, you check all of the fields again. This seems unnecessary, so I omitted it from my version.

If you need to return values, use a function or an instance variable that all functions can access.

You'll notice I use the String.IsNullOrWhiteSpace() method (introduced in .NET 4) to check strings for null. This checks for an actual Nothing value, or a string: "" or a string with only whitespace " ", so it is a clean way to check all of the above. Some documentation here: http://msdn.microsoft.com/en-us/library/system.string.isnullorwhitespace(v=vs.110).aspx

Using call while it is valid syntax, is pretty "old-style" VBA syntax and can be excluded. I believe this makes the code easier to read.

All this being said, the way you are doing the validation seems strange.

.NET comes built-in with form field validators so there isn't a reason to write your own (unless you require highly customized functionality), but in your case it seems to be all required fields, which is supported out of the box. See these links for details:

\$\endgroup\$
  • \$\begingroup\$ wow thats brilliant. thank you very much. just one change, at the end...if validationstatus < 0 then \$\endgroup\$ – FoxMcCloud Oct 9 '14 at 13:59
11
\$\begingroup\$

Your validation has a "major" flaw. You are validating each thing by itself, so If you user "creates" multiple validation errors, this provides a seriously bad UX. While we're at UX, your error messages could use some love: here's a little help from UX.SE

The easiest fix for this is to use binary flags, similar to the following:

[Flags]
Enum ValidatorFlags As Integer
   MonthSelect = 1 ' 0001
   YearSelect  = 2 ' 0010
   FilepathBox = 4 ' 0100
End Enum

Any combination of these flags is guaranteed to be backtraceable to the flags that are composing it (Sidenote: this is how linux permissions work).

As a sidenote: the Flags attribute as used in this enum and described more in depth in this SO question allows nice toString() magic (not that you'd need it). In the (currently) accepted answer there you also get a good lead on how you'd backtrace the original errors to the flags ;) The code also works fine without it though.

In addition to the mentined points and the very nice review by xDaevax (who also led me to the Flags question on SO), I want to mention that the names of your error message containers are gruesome.

Don't number your variables

Instead of Label1,2,3,... use descriptive names, just as you did in your InputFields

your code would then become somewhat like this:

Public Sub Validator 'leaving rest for brevity
    Dim Validator As ValidatorFlags
    If MonthSelectBox.Text = "" Then
       Validator |= ValidatorFlags.MonthSelect
       MonthErrorLabel.Text = "Please select a month."
    Else 
       MonthErrorLabel.Text = ""
    End If

    If YearSelectBox.Text = "" Then
        Validator |= ValidatorFlags.YearSelect
        YearErrorLabel.Text = "Please select a year."
    Else
        YearErrorLabel.Text = ""
    End If

    If FilePathTextBox = "" Then
        Validator |= ValidatorFlags.FilepathBox
        FilepathErrorLabel.Text = "Please select a valid filepath."
    Else
        FilepathErrorLabel.Text = ""
    End If
    Return Validator
End Sub
\$\endgroup\$
  • \$\begingroup\$ The Flags is an awesome idea! Is the [Flags] attribute required for it to be processed correctly or is it superfluous? \$\endgroup\$ – xDaevax Oct 10 '14 at 15:46
  • \$\begingroup\$ @xDaevax I dont't quite understand... Sure you could leave the flag adding out, but then you could just return false instead of explicit error codes. The problem begins when you want to know what errors exactly occured... \$\endgroup\$ – Vogel612 Oct 10 '14 at 15:49
  • \$\begingroup\$ @Vogel612 To clarify, I'm wondering if decorating the Enum with the <Flags> attribute is necessary, or if using the ValidatorFlags enum without the attribute is equally valid. But, I think I answered my own question with this answer: stackoverflow.com/a/8480/937012 \$\endgroup\$ – xDaevax Oct 10 '14 at 15:53
  • \$\begingroup\$ +1 for validating everything at the same time, but Flags should never be added or subtracted like Validator += ValidatorFlags.MonthSelect. They are bit flags, so you need to use bitwise operators with them: Validator = Validator And ValidatorFlags.MonthSelect. If you add MonthSelect twice, you unset it and set YearSelect instead. Also, unless there is a compelling reason to Dim it as an Integer, it should be Dim Validator As ValidatorFlags. \$\endgroup\$ – Comintern Oct 11 '14 at 15:35
5
\$\begingroup\$

A really small style point to add to the other answers: I personally prefer String.Empty over empty quotes. I think it makes the intention of the code more clear. At a glance, it can be difficult to tell if it's empty quotes, a single space, or if the original dev simply forgot to fill in the text.

Label1.Text = ""

Using String.Empty clears up this ambiguity.

Label1.Text = String.Empty
\$\endgroup\$
  • 2
    \$\begingroup\$ I wasn't aware that you could use String.Empty instead of "". Thanks, I completely agree that this is 10000 x better! \$\endgroup\$ – FoxMcCloud Oct 13 '14 at 12:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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