5
\$\begingroup\$

I created a project for a programming class that asks you to make a userform in Visual Studio that calculates mileage, given the user inputs of gallons of gas consumed and distance traveled. The code does its job fine, but I can't help but feel that my use of Do...While and Try...Catch statements are less than elegant. If anyone has suggestions or tips or cryptic hints about what would improve my code, I would be grateful!

Private Sub btnMileage_Click(sender As Object, e As EventArgs) Handles btnMileage.Click
    Dim sngGas As Single, sngDistance As Single, sngMileage As Single

    Do While sngGas <= 0 'force user to input positive number for gallons of gas consumed
        Try 'reject all inputs that are not real numbers
            sngGas = CSng(InputBox("Please enter gasoline consumed in gallons:"))
            If sngGas <= 0 Then 'remind user to input number greater than 0
                MsgBox("Please make sure your input is greater than 0.")
            End If
        Catch ex As Exception
            MsgBox("Error: Please enter a valid number.") 'remind user that all inputs that aren't numbers are rejected
        End Try
    Loop

    Do While sngDistance <= 0
        Try 'reject all inputs that are not real numbers
            sngDistance = CSng(InputBox("Please enter distance travelled in miles:")) 'force user to enter positive value for distance
            If sngDistance <= 0 Then
                MsgBox("Please make sure your input is greater than 0.")
            End If
        Catch ex As Exception
            MsgBox("Error: Please enter a valid number.") 'reminder user that all inputs that aren't numbers are rejected
        End Try
    Loop

    sngMileage = Math.Round((sngDistance / sngGas), 1, MidpointRounding.AwayFromZero) 'calculate mileage

    PopulateForm(CStr(sngGas), CStr(sngDistance), CStr(sngMileage)) 'update form with gas, distance & mileage


    MsgBox("All done!")
End Sub

The sub I use to populate the form follows:

Private Sub PopulateForm(Optional ByVal strGas As String = "", Optional ByVal strDistance As String = "", Optional ByVal strMileage As String = "") 'wipes form or populates with new values

    lblGas.Text = "Gas Consumed: " & strGas
    lblDistance.Text = "Distance Travelled: " & strDistance
    lblMileage.Text = "Mileage: " & strMileage

End Sub
\$\endgroup\$
6
\$\begingroup\$

You have a form -- by definition, a window in which text boxes and such can appear. Why are you using MsgBox and InputBox to communicate with the user?

If you instead put elements on the form and used those (particularly NumericUpDown), the code part would be so much simpler -- and far more like, well, a Windows app.

Watch this. Create a form, put three elements on it -- two NumericUpDown controls named GasAmountBox and DistanceBox, and one text control (text box or label, your choice really) named MileageOutput. Add other elements (like labels for the controls) to suit your taste.

Then, inside the form class, put something like:

Private Sub InputsChanged(sender As Object, e As EventArgs) _
  Handles GasAmountBox.ValueChanged, DistanceBox.ValueChanged
    If (GasAmountBox.Value = 0) Then
        MileageOutput.Text = "---"
    Else
        Dim mileage = DistanceBox.Value / GasAmountBox.Value
        MileageOutput.Text = mileage.ToString() & " MPG"
    End If
End Sub

Aaaaand done.

That's the entirety of the code you have to write to make the form work. The controls take care of ensuring that the user inputs a number. You can even set the allowable range of values, so you don't have to care about negative numbers. You can add some code to, say, round the output, if you want.

\$\endgroup\$
4
\$\begingroup\$

This is , but it's very close to be compilable in or - the reason for this is the Imports Microsoft.VisualBasic that you haven't shown but that InputBox and MsgBox are giving away.

Maybe it's just my own personal opinion, but I firmly believe that the Microsoft.VisualBasic namespace only exists to facilitate rewriting legacy applications in ; new developments shouldn't use that namespace, as the rest of the .net framework contains much better tooling.

For example, I'd use CSng in to convert a value to a Single. In .net though, I'd use a DirectCast if I know the value can be cast to a Single. In your case, you don't know that - all you know is that you have a String that may or may not be numeric; instead of wrapping the archaic conversion construct with a Try/Catch block, you should use the TryParse method of the type you're converting to:

Dim userInput As String, gallons As Single
Do While Not Single.TryParse(userInput, gallons)
    '...
Loop

Notice the identifier names: avoid using Hungarian-style prefixes, prefer meaningful names that tell you what the variable contains, not what its type is.

Instead of a legacy Microsoft.VisualBasic.InputBox, I would have a simple TextBox on the form for the user to provide input - if you wanted to push it you could even implement the validation in the textbox itself.

Instead of Microsoft.VisualBasic.MsgBox, I would use a System.Windows.Forms.MessageBox, which is the .net WinForms replacement for it.

Other than that.. you've implemented the entire logic inside a button click handler; that can get easily get messy. You need to break the problem down into several small steps, and implement each step in its own method - that way you can reuse code instead of Copy+Paste'ing it. PopulateForm is a good step forward.

Lastly, your comments are way overkill. Here is a very good canonical CR answer about comments - it's , but the concept is the same:

  • Comments are supposed to make plain what the code does not tell us already.
  • Good code seldom needs comments
\$\endgroup\$
  • 1
    \$\begingroup\$ Cool comment about comments ;-) \$\endgroup\$ – rolfl May 14 '15 at 4:51
2
\$\begingroup\$

I would use the Single.TryParse instead of a Try/Catch. Since you are doing the same thing twice, I would put it in a function.

Public Function GetSingle(ByVal message As String) As Single

    Dim enteredValue As String
    Dim returnValue As Single

    Do
        enteredValue = InputBox(message)

        If Not Single.TryParse(enteredValue, returnValue) Then
            MsgBox("Error: Please enter a valid number.")
        ElseIf returnValue <= 0 Then
            MsgBox("Please make sure your input is greater than 0.")
        Else
            Exit Do
        End If
    Loop

    Return returnValue
End Function

Your function would look like this

gallons = GetSingle("Please enter gasoline consumed in gallons:")
distance = GetSingle("Please enter distance travelled in miles:")

mileage = Math.Round((distance / gallons), 1, MidpointRounding.AwayFromZero)

Personally, I would send single instead of strings and let PopulateForm do the conversion.

PopulateForm(gallons, distance, mileage)

The others have good suggestion. I would stay away from Microsoft.VisualBasic.

\$\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.