Noob: Input Validation help with inputbox

[lovekeiiiy]

I need to some assistance in creating some input validation for a homework assignment. The program is creating an application that shows, based in inventory and order request, how many spools of wire are ready to ship, backed ordered, shipping costs, and total cost. The picture below is the form designed given to use.



This chapter is covering creating procuedures and functions. The assignment also said to include input validation; must order at least 1 spool; I've included inventory cannot be less than zero. The assignment also to use an inputbox to get current inventory in stock

I have everything working, almost. The problem is when I give bad data for current inventory or spools ordered, say -100, I get the error messagebox I create, but the program continues, and then gives bad output. I haven't figured how to prevent bad output. Something like

Function input()
Do
    MessageBox.Show("supply must be 0 (zero) or greater", "Current Supply Input Error")
            If intCurrentSupply >= 0 Then
            Else
                MessageBox.Show("supply must be 0 (zero) or greater", "Current Supply Input Error")
            End If

Loop Unit Integer.TryParse(InputBox("How many spools are currently in stock?", "Current Spools in Stock"), intCurrentSupply) And intCurrentSupply >= 0

My thinking was to create some kind of Loop to test the input and repeat till I got input that was is correct.


Another option I could do is create another function for output, and have it compared the output values. If they values are less zero, have it change the labels' text property to an empty string. But I don't like this option because I'm not really learning a way to force to user to give good input.

Here's the code the assignment. I realize some is probably not needed, but wasn't fully certain what could be removed without making more difficult to help me.

Public Class Form1

    Private Sub Form1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
        txtNumberOfSpools.Focus()
    End Sub

    Function GetInStock()
        'get current number of spools in stock
        Dim intCurrentSupply As Integer

        If Integer.TryParse(InputBox("How many spools are currently in stock?", "Current Spools in Stock"), intCurrentSupply) Then
            If intCurrentSupply >= 0 Then
            Else
                MessageBox.Show("supply must be 0 (zero) or greater", "Current Supply Input Error")
            End If
        Else
            MessageBox.Show("must enter a numerical value for current supply of spools", "Current Stock Input Error")
        End If

        Return intCurrentSupply
    End Function

    Function SpoolsOrdered()
        'get order and validate input
        Dim intSpoolsOrdered As Integer

        If Integer.TryParse(txtNumberOfSpools.Text, intSpoolsOrdered) Then
            If intSpoolsOrdered > 0 Then
            Else
                MessageBox.Show("must have at least one order", "Order Input Error")
                txtNumberOfSpools.Text = String.Empty
                txtNumberOfSpools.Focus()
            End If
        Else
            MessageBox.Show("must enter a numerical value for amount ordered", "Order Input Error")
            txtNumberOfSpools.Text = String.Empty
            txtNumberOfSpools.Focus()
        End If

        Return intSpoolsOrdered
    End Function

    Function ReadyToShip(ByVal intCurrentSupply As Integer, ByVal intSpoolsOrdered As Integer) As Integer
        'determine how many spools ready to ship now

        Dim intReadyToShip As Integer

        If intCurrentSupply >= intSpoolsOrdered Then
            intReadyToShip = intSpoolsOrdered
        Else
            intReadyToShip = intCurrentSupply
        End If

        Return intReadyToShip
    End Function

    Function BackOrdered(ByVal intCurrentSupply As Integer, ByVal intSpoolsOrdered As Integer) As Integer
        'determine how many, if any, spools to place on backorder

        Dim intBackOrdered As Integer

        If intCurrentSupply <= intSpoolsOrdered Then
            intBackOrdered = (intSpoolsOrdered - intCurrentSupply)
        Else
            intBackOrdered = 0
        End If

        Return intBackOrdered
    End Function

    Function ShippingRate()
        'determine shipping rate

        Const decREGULAR_SHIPPING As Decimal = 10
        Const decRUSH_SHIPPING As Decimal = 15
        Dim intShippingRate As Integer

        If chkRush.Checked = True Then
            intShippingRate = decRUSH_SHIPPING
        Else
            intShippingRate = decREGULAR_SHIPPING
        End If

        Return intShippingRate
    End Function
    Function ShippingCharges(ByVal intReadyToShip As Integer, ByVal intShippingRate As Integer) As Integer
        'calculate shipping chargers

        Dim decShippingCharges As Decimal

        decShippingCharges = intReadyToShip * intShippingRate

        Return decShippingCharges
    End Function

    Private Sub btnCalculate_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnCalculate.Click
        'needs to call GetInStock, ReadyToShip, BackOrdered, ShippingCharges
        Const intSPOOL_COST As Integer = 100

        Dim intReadyToShip As Integer
        Dim intSpoolsOrdered As Integer
        Dim intCurrentSupply As Integer
        Dim intBackOrdered As Integer
        Dim intShippingRate As Integer
        Dim intShippingCharges As Integer
        Dim intTotal As Integer

        intCurrentSupply = GetInStock()
        intSpoolsOrdered = SpoolsOrdered()

        intReadyToShip = ReadyToShip(intCurrentSupply, intSpoolsOrdered)
        intBackOrdered = BackOrdered(intCurrentSupply, intSpoolsOrdered)
        intShippingRate = ShippingRate()
        intShippingCharges = ShippingCharges(intReadyToShip, intShippingRate)

        intTotal = (intReadyToShip * intSPOOL_COST) + intShippingCharges

        lblReadyShip.Text = intReadyToShip.ToString()
        lblBackOrder.Text = intBackOrdered.ToString()
        lblShippingHandling.Text = intShippingCharges.ToString("c")
        lblTotal.Text = intTotal.ToString("c")
    End Sub

    Private Sub btnClear_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnClear.Click
        'needs to call ResetSpools and ResetDelivery
        Call ResetDelivery()
        Call ResetSpools()
    End Sub

    Private Sub btnExit_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnExit.Click
        Me.Close()
    End Sub
    Sub ResetSpools()
        'clears the text box and check box
        txtNumberOfSpools.Clear()
        chkRush.Checked = False
    End Sub
    Sub ResetDelivery()
        'clears the display delivery information labels
        lblBackOrder.Text = String.Empty
        lblReadyShip.Text = String.Empty
        lblShippingHandling.Text = String.Empty
        lblTotal.Text = String.Empty
    End Sub
End Class

As for the code, I believe I have some unnecessary redundancy. I'll bother my teacher tomorrow about where I declared some of my variables and, if and when, I should use Static variables, or Class. I'll check with him about some of my Return statements. Some I know I could have just used formula right after it rather than a variable. Needless to say, I'm trying to wrap my mind around variable declarations and the Return statement in regards to functions and procedures.

As always, thanks for any help or guidance.

Rich

[4x2y]

The form already has text box to input number of spools, so why you need InputBox?

A solution could be enabling/disabling btnCalculate according to the value of txtNumberOfSpools, e.g.

    Private Sub txtNumberOfSpools_TextChanged(sender As Object, e As EventArgs) Handles txtNumberOfSpools.TextChanged
        btnCalculate.Enabled = IsNumeric(txtNumberOfSpools.Text) AndAlso CInt(txtNumberOfSpools.Text) >= 0
    End Sub

Another option is using the control NumericUpDown instead of textbox, it is more suitable for entering numeric inputs.

[lovekeiiiy]

the input box was just to get the how much current supply of spools there are. The textbox is to get how many spools are being ordered. I didn't design the form. It's what the book gave. The design wasn't by my choice.

The book hasn't discussed NumericUpDown yet. Although, sounds like that would be an good choice.

[dunfiddlin]

It's what the book gave.

The book hasn't discussed NumericUpDown yet.

Of course it hasn't. I mean that would make sense and have some real world application!

Name and shame! We can add it to the list for the next book burning!

[lovekeiiiy]

Well, apparently the book doesn't discuss the NumericalUpDown. At least I wasn't able find anything in the book's index. The book is Starting Out With Visual Basic 2010 by Tony Gaddis and Kip Irvine. Personally, I don't think the book is that bad. It's not a book that goes into advance topics. I think it's more about teaching programming fundamentals. They have a second book, Advance Visual Basic 2010. I don't know if it covers stuff like NumericUpDown. Overall, the book is fairly easy to understand. I wish it had a few more tutorials to show more ways on how things can be used. It think it would be nice to show more general conventions used as well; it does go over some.

Probably most students wouldn't give a hoot about the bad output once the program was working with good data in. I just assume that making sure you have good data in never really goes away the farther along you get in programming. And the class I'm taking, an introduction to computers, is designed for students taking for various majors beyond computers such as business.

Actually, I've completed all the homework and reading for the class already. Just waiting to do the final, which is in about three weeks, so this is more persuit of my own knowledge and any follow up class I choose to take.

Nonetheless, given what was assigned, any ideas on how to solve my bad input or bad output situation? I assume there is a way, it MAY be beyond my skill set at the moment, or I'm just not making a connection for what I do know.

[jmcilhinney]

You should be doing a pre-test rather than a post-test because you don't want to show the user a MessageBox chastising them for doing the wrong thing until they've actually done the wrong thing.

Private Function GetSpoolsInStockCount() As Integer
    Dim spoolsInStockCount As Integer

    Do Until Integer.TryParse(InputBox("How many spools are currently in stock?", "Current Spools in Stock"),
                              spoolsInStockCount) AndAlso
             spoolsInStockCount >= 0
        MessageBox.Show("Stock count must be a number equal to or greater than zero.", "Current Stock Input Error")
    Loop

    Return spoolsInStockCount
End Function

Notice a few other things there:

1. The function has an access modifier, i.e. Private in this case. While it is not essential, it is good practice to provide an access modifier for every type and member.
2. The function has a return type, i.e. Integer in this case. While it is not essential with Option Strict Off, it is good practice to provide a return type for every function. Also, any self-respecting developer should turn Option Strict On as soon as they learn about it.
3. The function has a descriptive name that starts with an upper-case letter. 'input' is a very poor function name.
4. The Boolean expression uses AndAlso rather than And. You should always use AndAlso and OrElse in preference to And and Or unless you have a specific need to avoid short-circuiting, which should almost never happen.

[lovekeiiiy]

thanks. I'll try experimenting with the code you provided tomorrow. Some of what you talked I need to reread to gain a better understanding, such as the access modifiers. I think VB just defaults to Public in this case.

I don't recall if the book discussed AndAlso or OrElse. I need to look it up again if it was covered.

I wasn't aware it was good practice to provide a data type for every function. The book only showed examples where it did not unless there was some kind of ByVal statement. Easy fix.

The as for naming convention, that was more me trying to be generic. At the time, I was thinking I could do a loop statement for both the GetInStock() and SpoolsOrdered() functions. Thus, if I had the general format, I plug and tweak for both functions. The problem, I forget the SpoolsOrdered uses a textbox, which requires to use another form is using a loop; I haven't learned how to do another form, yet.

Along the same lines, I just realized I didn't include Return and End Function statements in my proposed code. I think I was trying to focus on the loop statements. My apologize. Nonetheless, since I'm so new to programming, nothing wrong with more reinforcement on proper code.

In the mean time, I did create function to hide bad code. Actually, I create a function for the output to labels. It basically says if there is bad data, leave the labels empty.

     Private Sub btnCalculate_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles btnCalculate.Click
removed code

        Output(intCurrentSupply, intSpoolsOrdered, intReadyToShip, intBackOrdered, intShippingCharges, intTotal)

    End Sub   

Sub Output(ByVal intCurrentSupply As Integer, ByVal intSpoolsOrdered As Integer, ByVal intReadyToShip As Integer, ByVal intBackOrdered As Integer, ByVal intShippingCharges As Integer, ByVal intTotal As Integer)
        If intCurrentSupply >= 0 And intSpoolsOrdered > 0 Then
            lblReadyShip.Text = intReadyToShip.ToString()
            lblBackOrder.Text = intBackOrdered.ToString()
            lblShippingHandling.Text = intShippingCharges.ToString("c")
            lblTotal.Text = intTotal.ToString("c")
        Else
            lblReadyShip.Text = String.Empty
            lblBackOrder.Text = String.Empty
            lblShippingHandling.Text = String.Empty
            lblTotal.Text = String.Empty
        End If

    End Sub

The trickiest part was the BackOrdered is the supply was given a negative value. I basically added test to check if current supply is zero or more. If true, then proceed, otherwise it returns no value, and thus nothing to convert later for the label display. I also had to a line that if intCurrentSupply didn't pass the Try.Parse test, had a False return, to give intCurrentSupply a -1 value so the output test would return an empty string, but still work if 0 was entered. Overall, I think this later solution is jenky. But on the user end, it would show similar to more complete input validation.

[lovekeiiiy]

You should be doing a pre-test rather than a post-test ....
1. The function has an access modifier, i.e. Private in this case. While it is not essential, it is good practice to provide an access modifier for every type and member.
2. The function has a return type, i.e. Integer in this case. While it is not essential with Option Strict Off, it is good practice to provide a return type for every function. Also, any self-respecting developer should turn Option Strict On as soon as they learn about it.
3. The function has a descriptive name that starts with an upper-case letter. 'input' is a very poor function name.
4. The Boolean expression uses AndAlso rather than And. You should always use AndAlso and OrElse in preference to And and Or unless you have a specific need to avoid short-circuiting, which should almost never happen.

I tried the code given, and work very well. Thank you. I should probably go back and do more practice on loops in using for other items than just comparing numbers. Although, I thought I had to do call the inputbox before the loop. Didn't realize I just call in the loop test expression.

The book did cover AndAlso and OrElse, but I forgot them.

The access modifiers were only covered briefly thus far; about two paragraphs total. The book says it'll cover them more later; I assume this is because I haven't gotten to create application that use more than one form, which probably means no difference for public, private and public. And didn't discuss Friend, Protected, Protected Friend beyond that they exist.