# Userform that calculates gas mileage

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
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
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


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.

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.

• Comments are supposed to make plain what the code does not tell us already.
• Good code seldom needs comments

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