Which one is more tend to throw error?

Question

I am reviewing an application and I am wondering which one is more error-prone?

    Public Function ToBool(ByVal vsValue As String) As Boolean

        If IsNothing(vsValue) OrElse vsValue = "" Then
            Return False
        End If
        If UCase(vsValue) = "TRUE" Or vsValue = "1" Or UCase(vsValue) = "Y" Or UCase(vsValue) = "YES" Or UCase(vsValue) = "T" Then
            Return True
        Else
            Return False
        End If
    End Function

or

Public Function ToBool(ByVal vsValue As String) As Boolean

            If IsNothing(vsValue) OrElse vsValue = "" Then
                Return False

            ElseIf UCase(vsValue) = "TRUE" Or vsValue = "1" Or UCase(vsValue) = "Y" Or UCase(vsValue) = "YES" Or UCase(vsValue) = "T" Then
                Return True
            Else
                Return False
            End If
        End Function

Why not just use `CBool` instead of writing your own function? — JaredPar, Aug 09 '11 at 15:16
Because there are another values considered as True such as "T", 1 etc. That's why it is checking whether any of those values are assigned, it is considered as True. — Tarik, Aug 09 '11 at 15:28
Just as a comment unrelated to the answer to this question, you should consider a couple of things for the above code: 1) Use `String.Equals(vsValue, "True", true)` instead of actually changing the case of the variable. 2) If you'd rather do it as you are above, store the result of `UCase` in a local variable so that you only do it once, and 3) Rather than use the VB-specific `UCase` function, use `vsValue.ToUpper()`. — Adam Robinson, Aug 09 '11 at 15:32

score 2 · Accepted Answer · answered Aug 09 '11 at 15:14

There is no difference as to which one is more error prone, as the two code examples are semantically identical. If the code enters the body of the first If block, then in both cases it will exit the function. The code that executes if it does not enter the body of the first If block is also identical.

score 1 · Answer 2 · answered Aug 09 '11 at 15:15

1

first one, because you won't be going through everything if vsValue is nothing, otherwise if it is something and the second uCase value's criterias are not there, then it will go to the final else. Which will return false when you may only be missing a specific state.

It just makes it so the program wouldn't need to go through two extra if's in the end.

answered Aug 09 '11 at 15:15

Kevin

3,509
4
31
45

The program exits the function if it enters the first `If`. There is no difference between the two in the actual steps that are executed. – Adam Robinson Aug 09 '11 at 15:19

score 1 · Answer 3 · answered Aug 09 '11 at 15:15

1

I believe none is more error-prone. 2nd one might be a little better since it uses else if instead of 2 ifs ...

answered Aug 09 '11 at 15:15

David

429
1
4
10

1

In what way is either better from a performance perspective? The two examples are semantically identical. I'll agree on readability, though, but that isn't what the OP asked about. – Adam Robinson Aug 09 '11 at 15:24

score 1 · Answer 4 · answered Aug 09 '11 at 16:44

An observation with your function: If you pass in a boolean True value that has been converted to an integer it will fail because it evaluates to -1 not 1.

When Visual Basic converts numeric data type values to Boolean, 0 becomes False and all other values become True

I wrote something very similar to this as follows which evaluates the False side first:

Public Shared Function ConvertBool(ByVal Value As String) As Boolean
    If Value Is Nothing Then Return False
    Select Case Value.ToUpper
        Case "", "0", "N", "FALSE", "F" 'etc
            Return False
        Case Else
            Return True
    End Select
End Function

score 0 · Answer 5 · edited May 23 '17 at 09:58

The end result is going to be the same in either case. May I suggest the following refactoring technique which makes the code a little more readable and is easier to expand to include other matches?

Public Function ToBool(ByVal vsValue As String) As Boolean
  If Not String.IsNullOrEmpty(vsValue) Then
    Dim value As Boolean = False
    vsValue = vsValue.ToUpper()
    value = value OrElse vsValue = "TRUE"
    value = value OrElse vsValue = "1"
    value = value OrElse vsValue = "Y"
    value = value OrElse vsValue = "YES"
    value = value OrElse vsValue = "T"
    Return value
  End If
  Return False
End If

Here is another possibility.

Public Function ToBool(ByVal vsValue As String) As Boolean
  If Not String.IsNullOrEmpty(vsValue) Then
    vsValue = vsValue.ToUpper()
    Select Case vsValue
      Case "TRUE": Return True
      Case "1":    Return True
      Case "Y":    Return True
      Case "YES":  Return True
      Case "T":    Return True
    End Select
  End If
  Return False
End If

In the above the example the compiler will emit a sequencial lookup. If the number of case statements hits a certain threshold it may convert the entire Select construct into a Dictionary lookup like what C# does. If this is a function that you expect to be called then you could experiment with both ways to see which one is faster.

Which one is more tend to throw error?

5 Answers5