Is this smelly error handling? [closed]

Closed. This question is opinion-based. It is not currently accepting answers.

---

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 3 years ago.

Improve this question

This seems like a pretty efficient way to do error handling, but I want to know how to do it correctly if this is smelly:

Class Widget
    ...
    Public Function IsValid() As Boolean
        If (some condition isnt met) Then
            Throw New ApplicationException("Error message")
        ElseIf (some other condition isnt met) Then
            Throw New ApplicationException("Another error message")
        End If
        Return True
    End Function
    ...
End Class

... (somewhere else)...
Public Function DoAwesomeStuff(id As Integer) As String
    Dim w As Widget() = Widget.GetWidget(id)
    If w.IsValid Then
        Do Awesome Things
    End If
    Return a string of some sort
End Sub

... (somewhere elser)...
<WebMethod(EnableSession:=True)>
<ScriptMethod(ResponseFormat:=ResponseFormat.Json)>
Public Function Add(ByVal id As Integer) As String
    Try
       //.ToJson is an Extension Method that serializes an obj to JSON
       Return New With {.Message = DoAwesomeStuff(id)}.ToJson
    Catch ex 开发者_StackOverflowAs Exception
        Return New With {.Message = "Error: " & ex.Message, .Error = True}.ToJson
    End Try
End Function

And then in the javascript I check for an Error property in the response and handle accordingly. It seems like it works ok for me, but I am not sure if this is smelly. I know you're not supposed to use Try/Catch blocks for control flow, I'm just not sure if this qualifies as control flow or not.

---

Yes, it has a vexing exception smell.

Users of the IsValid function would expect it to return True or False.

Exceptions should be used in unexpected cases, for example, if some variable needed to evaluate the validity in your IsValid function is not properly initialized.

If you want to have a validation failure message along with the validation status, consider using a ByRef parameter for example, something like:

Public Function IsValid(ByRef invalidReason as String) As Boolean
    If (some condition isnt met) Then
        invalidReason = "Error message"
        return False
    ElseIf (some other condition isnt met) Then
        invalidReason = "Another error message"
        return False
    End If
    Return True
End Function

---

Looks perfectly valid to me.

Since this is on a system boundary (web service), it is perfectly OK to try and return an error if there was an exception.

The only smell here is in the IsValid method - it really shouldn't be the one throwing exceptions. The calling method should (when a false is returned), or even the lower level methods that you call in the If clauses (assuming you have encapsulated them in their own methods).