5
\$\begingroup\$

I have a series of text boxes in a table to gather input as below:

enter image description here

The user will input a target and actual value for each measurement point they require. I would then like to validate the actual values against the target values based upon the tolerance inputted in the tolerance textbox. The tolerance will be the same for all measurement points inputted, however the user will not always input all 10 measurement points.

I have also created a very basic class containing a function that accepts the target, actual and tolerance values then returns a Boolean depending on whether the actual value is within tolerance. I realise I could use this with a ruck load of if statements to check each textbox for input then use the class to perform the validation, however this seems like a lot of code repetition and a bit crude. My question being is there a better way I can perform this validation?

Class content

Public Class TolerenceHelper

Public Function IsInTolerence(ByVal target As Integer, ByVal actual As Integer, ByVal tolerence As Integer) As Boolean

    Dim upper As Integer = target + tolerence
    Dim lower As Integer = target - tolerence

    If actual < lower OrElse actual > upper Then
        Return False
    Else
        Return True
    End If
End Function

Calling the function as below:

Dim m1 As TolerenceHelper
Dim flag As Boolean = True

m1 = New TolerenceHelper

If m1.IsInTolerence(Integer.Parse(txtT1.Text), Integer.Parse(txtA1.Text), Integer.Parse(txtTolerance.Text)) = False Then
    flag = False
End If

If flag = False Then
    lblTest.Text = "Out of tolerance"
Else
    lblTest.Text = "In tolerance"
End If
\$\endgroup\$
  • \$\begingroup\$ Do you have a separate lblTest for each check, or are you reusing the same field? Are you only checking 1 at a time? \$\endgroup\$ – Daniel Nov 25 '13 at 17:11
  • 1
    \$\begingroup\$ Isn't it spelled "tolerance"? \$\endgroup\$ – Mathieu Guindon Nov 26 '13 at 3:10
  • \$\begingroup\$ Jimsan, has anyone appropriately answered your question? if not please provide some specifics on what more you would like \$\endgroup\$ – Malachi Nov 27 '13 at 19:05
  • \$\begingroup\$ Malachi, I didn't mark an answer as everyone has valid points. I ended up using the code that I added in the edit of my post. Thank you all for your help though \$\endgroup\$ – Jimsan Nov 28 '13 at 9:00
  • \$\begingroup\$ @Jimsan if you chose that as a solution you should post it as an answer and accept it as the answer \$\endgroup\$ – Malachi Dec 10 '13 at 21:05
7
\$\begingroup\$

I would write it like this

Dim m1 As TolerenceHelper
Dim flag As Boolean = True

m1 = New TolerenceHelper

flag = m1.IsInTolerence(Integer.Parse(txtT1.Text), Integer.Parse(txtA1.Text), Integer.Parse(txtTolerance.Text))

If flag = False Then
    lblTest.Text = "Out of tolerance"
Else
    lblTest.Text = "In tolerance"
End If

that function returns a boolean so you can just assign it the flag or you could do it like this and avoid creating the flag boolean as well

Dim m1 As TolerenceHelper
m1 = New TolerenceHelper

If m1.IsInTolerence(Integer.Parse(txtT1.Text), Integer.Parse(txtA1.Text), Integer.Parse(txtTolerance.Text)) Then
    lblTest.Text = "In tolerance"
Else
    lblTest.Text = "Out of tolerance"
End If

In your Function IsInTolerence you could eliminate some variables by writing it like this:

Public Function IsInTolerence(ByVal target As Integer, ByVal actual As Integer, ByVal tolerence As Integer) As Boolean

    If (target - tolerence <= actual AND actual =< target + tolerence)
        Return True
    Else
        Return False
    End If
End Function

Straight to the point.

check if it is true first, and then dump out in all other cases, this would cover a null or something else.


Remove Code Duplication with a list of TextBox Objects

I am thinking that you create a list of the text boxes, then you should be able to loop through that list, checking each text box for the following.

  1. if it has a value entered
  2. if the tolerance is met
  3. some kind of highlighting to show that the tolerance was not met.

I am not sure on the syntax, but this would be a good way to do it, because you would only have to type out the logic once and then loop through the list of TextBoxes, and it would be more maintainable because you could just add TextBoxes any time you like and it shouldn't affect the any of the other code.

dim TextBoxListActual as New List(Of TextBox)
TextBoxListActual.Add(txtT1)
TextBoxListActual.Add(txtT2)
TextBoxListActual.Add(txtT3)

Dim TextBoxListTarget as New List(Of TextBox)
TextBoxListTarget.Add(txtA1)
TextBoxListTarget.Add(txtA2)
TextBoxListTarget.Add(txtA3)

dim m1 As TolerenceHelper
m1 = New TolerenceHelper

For index As Integer = 0 To TextBoxListActual.Count
    If m1.IsInTolerence(Integer.Parse(TextBoxListTarget(index).value), Integer.Parse(TextBoxListActual(index).value), Integer.Parse(txtTolerance.Text))
        lblTest.Text = "In Tolerance"
    Else
        lblTest.Text = "Out of Tolerance"
    End If
Next

little bit of an example. probably not perfect.

\$\endgroup\$
  • 1
    \$\begingroup\$ I'd definitely do the 2nd revision, or the first revision but change If flag = False to simply If flag and then change the true clause to being first. \$\endgroup\$ – Daniel Nov 25 '13 at 16:56
4
\$\begingroup\$

Instead of performing each test via a series of if statements or needing to add your textboxes to a list, you can perform a simple for loop and utilize the FindControl command of the page.

This would allow you to write code something like this:

For i As Integer = 1 To 10
  Dim TargetBox As TextBox = Me.FindControl("txtT" & i)
  Dim ActualBox As TextBox = Me.FindControl("txtA" & i)
  If IsNothing(TargetBox) OrElse IsNothing(ActualBox) Then
    'Couldn't find TextBox so just try the next number
    Continue For
  Else
    If TolerenceHelper.IsInTolerence(Integer.Parse(TargetBox.Text), Integer.Parse(ActualBox.Text), Tolerence) Then
      lblText.Text += String.Format("Trial {0} In tolerance",i)
    Else
      lblText.Text += String.Format("Trial {0} Out of Tolerence",i)
    End If
  End If
Next
\$\endgroup\$
  • \$\begingroup\$ if they change the amount of Textboxes they would have to change the for loop. with the List of Text boxes the code won't change all they would have to do is in the declaration, another textboxList.add(textbox43) or they could find a better way of doing it like that. there has to be a collection collection or something where you could select all the textboxes in said group or something, this doesn't seem maintainable. \$\endgroup\$ – Malachi Nov 26 '13 at 2:32
2
\$\begingroup\$

As written, there is no reason for IsInTolerence not to be a shared function because TolerenceHelper has no benefit to actually being instantiated.

To make it shared, you would instead declare it like so:

Public Shared Function IsInTolerence(...

This will give you the advantage of having your initial calls instead of being:

Dim m1 As TolerenceHelper
m1 = New TolerenceHelper
If m1.IsInTolerence(...

You would use:

If TolerenceHelper.IsInTolerence(...

\$\endgroup\$
1
\$\begingroup\$

Instead of parsing the tolerence each time, you'd want to set it to a variable or possibly make a property. I like the property method simply because it allows for a lot of flexibility.

Here's an example:

Protected Property Tolerence As Integer
    Get
      Static _Tolerence As Integer = -1 'Static variable so Tolerence parsed only once per postback
      If _Tolerence = -1 AndAlso Not Integer.TryParse(txtTolerence.Text, _Tolerence) Then
        _Tolerence = 0 ' or your default value or whatever you like
      End If
      Return _Tolerence
    End Get
    Set(value As Integer)
      txtTolerence.Text = value
    End Set
End Property

You may note that I used this Property in one of my other answers.

\$\endgroup\$
1
\$\begingroup\$

You could shorten IsInTolerence to being a 1-liner like this:

Function IsInTolerence(ByVal target As Integer, ByVal actual As Integer, ByVal tolerence As Integer) As Boolean
    Return target - tolerence <= actual And actual <= target + tolerence
End Function

A micro optimization note... I'd initially used AndAlso in my comment, but that runs slower even if the first check is always false.

\$\endgroup\$

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.