Problem with a VB function on my form (Visual Studio 2019)-CodePudding

The function I am trying to create takes in an input from a combo box (Small, Medium, Large) and then spits out a price based on what was selected. For example, if the user chooses small the calculated price would be 100, if Medium: 200, and if Large: 300. I want the function to be executed after the user clicks the button I created "Calculate Price". I would then have a message box tell the user "The calculated price is: {result}." I currently just keep getting the output "The calculated price is: 0." upon testing my work. Here is the code I am using for the function and button click. (Note: I don't need to have the dollar amounts be returned as integers, they can just be strings Im not too picky about that)

Private Sub Button1_Click(sender As Object, e As EventArgs) Handles Button1.Click
        Dim result As Integer
        result = calcpriceFunction(cboUnitType.ToString)
        MessageBox.Show("The calculated price is: " & result)

End Sub

Private Function calcpriceFunction(ByVal untype As String) As Integer
    Dim price As String
    untype = ""
    price = ""
    If untype = "Small" Then
        Return price = "100"
    ElseIf untype = "Medium" Then
        Return price = "200"
    ElseIf untype = "Large" Then
        Return price = "300"
    End If


End Function

If anyone can spot a mistake or some sort of convention flaw that would be amazing. I am all out of options right now!

CodePudding user response：

There are many problems with calcpriceFunction

First of all, the code declares that function as returning an integer but instead you try to return a string. This is accepted by the compiler because in the option of your project you have the Option Strict set to Off and this enables automatic type conversion (when possible). While it seems a good thing, in reality this automatic conversion creates lot of problems because you will never know when, for any reason, it doesn't work as expected. So, first thing is to change that option to Option Strict On.

Then it is necessary to remove that untype = "" because it changes the value passed in the parameter.

Finally, don't return at each if/else statement, but just set the price value and use just one final return

Private Function calcpriceFunction(ByVal untype As String) As Integer
    ' Zero is the default, but I like to be explicit
    Dim price As Integer = 0

    If untype = "Small" Then
        price = 100
    ElseIf untype = "Medium" Then
        price = 200
    ElseIf untype = "Large" Then
        price = 300
    End If
    Return price
End Function

This change will remove the many exit points from your method but also remove another subtle bug present in the original return statement

Return price = "100"

doesn't assign the value "100" to the variable price and then return it, instead it ask the compiler to return the result of the comparison between the current value of price and the constant "100". Or in other words. Is the current value of price equals to "100"? If not return 0

CodePudding user response：

Private Function calculatePrice(untype As String) As Integer
    If untype = "Small"  Then Return 100
    If untype = "Medium" Then Return 200
    If untype = "Large"  Then Return 300
    Throw New ArgumentOutOfRangeException()
End Function

One thing to understand early is that it's very bad to conflate integers and strings. "100" and 100 are not the same value! Moreover, thanks to internationalization/localization and other cultural concerns, converting back and forth between those values is so much slower and error-prone than you can know. It's absolutely something to avoid as much as possible.

Therefore, when accepting numeric input from a user, which of course must start as a string, convert to a numeric type like integer or Decimal as soon as possible, and keep it that way as long as possible, only convert it back to a string again when you must show it to a user again.

The same principle applies to DateTime values.

CodePudding user response：

Here are some modifications to your calcpriceFunction:

Private Function CalculatePrice(untype As String) As Integer
    Select Case untype.ToLowerInvariant
        Case "small"
            Return 100
        Case "medium"
            Return 200
        Case "large"
            Return 300
        Case Else
            Return 0
    End Select
End Function

In addition to the issues Steve mentioned in his answer, I suggest you make the function case-insensitive for the untype value. Also, I changed the condition block from If ... ElseIf ... ElseIf ... End If to Select Case for readability. Note that you can return the price directly once you have determined what it is as it is done in this example with Return 100. Finally, I renamed the function to CalculatePrice. There is no reason to abbreviate the name and you should not have Function in a function name.

CodePudding user response：

I made these changes to my Button Click and function and I got my desired outcome! Thank you so much everyone for the help.

Private Sub Button1_Click(sender As Object, e As EventArgs) Handles Button1.Click
        Dim result As Integer
        result = calcpriceFunction(cboUnitType.SelectedValue)
        MessageBox.Show("The calculated price is: $" & result & " per month.")

End Sub

Private Function calcpriceFunction(ByVal untype As String) As Integer

    Dim price As Integer = 0

    If untype = "Small" Then
        price = 100
    ElseIf untype = "Medium" Then
        price = 200
    ElseIf untype = "Large" Then
        price = 300
    End If
    Return price



End Function