isNumeric - Program Crashes

[bradz1993]

1. Public Class Form1
3. Private Sub Button1cmdRun_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1cmdRun.Click
5. ' Variable declaration & initilization
6. Dim hour As Integer
7. Dim minute As Integer
8. Dim second As Integer
11. ' Validation
12. If IsNumeric(txbTotal.Text) And txbTotal.Text Then >= 0 Then
14. ' Total number of seconds
15. Dim totalSeconds As Integer = CInt(txbTotal.Text)
19. ' Calculation
20. hour = totalSeconds \ 3600
21. minute = (totalSeconds Mod 3600) \ 60
22. second = totalSeconds Mod 60
24. ' Output result
25. lblResult.Text = "H: " & CStr(hour) & " M: " & CStr(minute) & " S: " & CStr(second)
27. Else
28. MessageBox.Show("Invalid input.", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error)
29. txbTotal.Clear()
30. End If
32. End Sub
34. Private Sub TextBox1_TextChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles txbTotal.TextChanged
36. End Sub
38. Private Sub lblResult_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles lblResult.Click
40. End Sub
41. End Class

Whenever I run the program and I enter text into the label the program crashes. When I type in a negative number the message box works.
A number like -45 worked and a message show appeared, whilst a string like 'bob made the program crashed.

I think line 12 is where the problem is.

How do I fix this?

Thanks.

 

[jmcilhinney]

For future reference, please just post your code as plain text inside

 tags and let the forum format it, including indenting.  That's the best way to make it as readable as possible for us.

With regards to the code, presumably this:[xcode=vb]If IsNumeric(txbTotal.Text) And txbTotal.Text Then >= 0 Then

is actually this:

If IsNumeric(txbTotal.Text) And txbTotal.Text >= 0 Then

The code you posted wouldn't even compile. In that case, there are several ways to improve the code. First, you should pretty much always use AndAlso and OrElse in preference to And and Or:

If IsNumeric(txbTotal.Text) [B]And[U]Also[/U][/B] txbTotal.Text >= 0 Then

The difference is that AndAlso and OrElse short-circuit while And and Or do not. In your case, your code will still compare the text to zero, even if it has already determined that it's not numeric. With AndAlso, if the first condition is False then the second condition doesn't get evaluated, which is exactly what you want.

Let's look closer at your code, even with that improvement:

If IsNumeric(txbTotal.Text) AndAlso txbTotal.Text >= 0 Then
    Dim totalSeconds As Integer = CInt(txbTotal.Text)

So, first you check whether the value in numeric. In order to do that, the IsNumeric function must actually convert the text to a number, but discards the result. If it is numeric, the code then proceeds to compare the text, a String, to zero. In order to do that, the system must implicitly convert the text to a number. That's twice the text has been converted now. Next, you explicitly convert the text to a number. That's three times the text is converted to a number. Not only that, if you are going to explicitly convert the text to a number then surely you should be doing that before comparing it to zero, another number. Finally, IsNumeric tells you nothing about the type of number it is. "12.34" would return True when passed to IsNumeric yet when passing it to CInt you would get 12, so you aren't even using the value the user entered.

The proper way to do this is to use the TryParse method of the numeric type you want to convert to. You want an Integer so you should be using Integer.TryParse:

Dim totalSeconds As Integer

If Integer.TryParse(txbTotal.Text, totalSeconds) AndAlso totalSeconds >= 0 Then

TryParse validates and converts the text, so you are only converting once. FYI, the IsNumeric function actually calls Double.TryParse internally. In fact, Double.TryParse was created in the first place specifically to improve the performance of IsNumeric.

With regards to the rest of your code, you're going about your time calculations suboptimally also.

Dim time = TimeSpan.FromSeconds(totalSeconds)

lblResult.Text = time.ToString("'H: 'h' M: 'm' S: 's")

 

[bradz1993]

Thanks for your replies. jmcilhinney your answer worked Just a quick question. Why don't I need to assign a value to totalSeconds when I declare it? Is it because this is done when you do the tryparse - If Integer.TryParse(txbTotal.Text, totalSeconds)?

 

[jmcilhinney]

That is correct. The second parameter to TryParse is declared ByRef, which means that a value will/can be assigned within the method and passed back out. If TryParse returns True then the String passed in is a valid integer representation and the corresponding Integer is passed out via the second parameter. If it returns False then the conversion failed and the second parameter will be zero.