Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
Public Function MyProjectFuncion(firstObject As Object,
                                 myEnumeration As Enumeration,
                                 theString As String,
                                 someCollection As List(of Object),
                                 anotherObject As Object) _
                            As String

    If IsNothing(firstObject) Then
        Throw New Exception("No data was received in 'firstObject'")
    Else
        If IsNothing(anotherObject) Then
            Throw New Exception("No data was received in 'anotherObject'")
        Else
            If IsNothing(myEnumeration) OrElse myEnumeration = 0 Then
                Throw New Exception("No data was received in 'myEnumeration'")
            Else
                If IsNothing(theString) OrElse theString = String.Empty Then
                    Throw New Exception("No data was received in 'theString'")
                Else
                    If IsNothing(someCollection) OrElse someCollection.Count = 0 Then
                        Throw New Exception("No data was received in 'someCollection'")
                    End If
                End If
            End If
        End If
    End If

    Return "All the variables have value"
End Function
share|improve this question
    
Thanks @SimonAndréForsberg but unfortunately this is a code that someone is actually using in our real project, I just don't wanna use the real name values. I'm posting this code 'cause I think there should be a better way to do this, but its beyond me ATM. –  Luis Apr 11 at 20:22
    
@200_success the only difference with the real project is the string inside de Exception. The values a, b, c[...] are variables and the message inside the exception wants to tell which one is the variable with no value. –  Luis Apr 11 at 20:25
    
@SimonAndréForsberg this is the initial block of a method, and its intention is to verify that all the parameters have value –  Luis Apr 11 at 20:27
1  
We'd like to know more because there's usually a deeper cause why code like this needs to exist in the first place. –  200_success Apr 11 at 20:28

2 Answers 2

up vote 8 down vote accepted

You're throwing System.Exception - that's bad, you'll want to throw a meaningful exception. See this answer for more details.

The main problem with this code, is that you're using exceptions to control the execution flow of your program, which is very expensive - I assume you're going to be catching these exceptions somewhere down the call stack.

If a is Nothing, then accessing it will throw an NullReferenceException. Same for b, c, d and e (say, you also have f in there?).

Let it blow up with whatever exception you get - and catch it. Use exceptions for exceptional cases, and avoid throwing them altogether if you can.


"Exceptions are expensive"

"Throwing and catching exceptions is expensive", you'll read [almost] everywhere. There's truth in that, Jon Skeet has a great article here that correlates performance to the depth of the call stack. The reason for this, is because the thread stops at the throw instruction, and then the exception's StackTrace gets populated - the call stack gets unwinded and the thread walks it until it reaches a catch block. The deeper the stack, the longer it takes.

But the problem isn't about performance - it's about efficiency. Exceptions are not a way to control your execution flow. Regardless of whether or not you care about wasting cycles and populating a stack trace, regardless of how long that takes for the runtime to complete, the point and the matter is, there are ways to make the intent much clearer to whoever is maintaining this code, than by throwing an exception.


The goal is to validate a bunch of objects, and return a message that informs which object is in an invalid state.

By throwing an exception, the first invalid object exits the function and you can't know if other ones were invalid.

By nesting the conditions and returning a String, the last invalid object gets its status reported... if you're lucky - I mean, when anotherObject isn't null, nothing else gets checked and you can't report that theString was NullOrEmpty.

How about returning a flag enum that contains all validation errors? (not sure of VB syntax here, my background is C#):

[Flags]
Public Enum MyValidationError
    None = 0
    FirstObjectIsInvalid = 1
    MyEnumerationIsInvalid = 2
    TheStringIsInvalid = 4
    SomeCollectionIsInvalid = 8
    AnotherObjectIsInvalid = 16
End Enum

Then the function can return a MyValidationError value:

Public Function MyProjectFunction(...) As MyValidationError

    Dim result As MyValidationError
    If IsNothing(firstObject) Then result += MyValidationError.FirstObjectIsInvalid
    If IsNothing(anotherObject) Then result += MyValidationError.AnotherObjectIsInvalid
    If MyEnumeration = 0 Then result += MyValidationError.MyEnumerationIsInvalid 'an enum type shouldn't ever be 'Nothing'...
    If String.IsNullOrEmpty(theString) Then result += MyValidationError.TheStringIsInvalid
    If IsNothing(SomeCollection) OrElse SomeCollection.Count = 0 Then result += MyValidationError.SomeCollectionIsInvalid

    Return result

End Function

Notice all parameters are validated every time now, and the result will contain everything that went wrong. Then another method can use bitwise AND operations on the result and determine the message(s) to log/display, all without throwing an exception, and without nesting anything.

share|improve this answer
    
I know that throwing exceptions are bad in this case, no one wants to throw a new exception just because an object is null, the problem is, that I need to show why are they expensive. (The person who did this, wants it to be an exception). –  Luis Apr 11 at 20:45
    
@Luis added more details, gotta run for now, will get back to this answer later tonight and make it cleaner for sure. –  Mat's Mug Apr 11 at 20:47
1  
Exception means: the function is undefined on a certain input. (In fact, if you throw an exception, then you are not allowed to also return a value.) The exception should be listed in the documentation, and if a client is not aware of it, then it's a bug in the client. If a function is defined on a certain input (MyProjectFunction is defined on all inputs) then an exception is just an ugly way of writing return. The difference in performance can be hardly seen in most cases, the point is exceptions are the wrong tool to be used in this context. –  ignis Apr 11 at 23:30
1  
Yes, it is, in fact I agree with you. –  ignis Apr 11 at 23:35
1  
The only thing I'd add is that I would argue that whilst by and large you should not control flow with exceptions, the use of recoverable exceptions can be justified though this dovetails right into @Mat'sMug 's point about the use of meaningful exceptions. If this is a library then by all means raise a suitable, targeted exception where appropriate (like FileNotFoundException or NotImplementedException) and don't catch it in your library, let the caller deal with it. If it ' a consumer then there's no shame in catching and dealing with recoverable errors like FileNotFoundException –  Steve Pettifer May 8 at 21:02
  1. Lets kill that giant nest of code. When an Exception is thrown, it interrupts the flow of the program. The code does not continue to execute beyond that point, it is like an early return in that regard. This means we do not need to nest our next statement in the Else block, because if an exception was thrown the program has returned.

  2. You should not be throwing Exception, not to say you should throw exceptions, but just not throw the base Exception class. Instead make your own exception class that inherits from Exception. An incredibly simple way of doing this is just...

    Public Class MissingDataException
        Inherits Exception
        Public Sub New(message As String)
                MyBase.New(message)
        End Sub
        Public Sub New(message As String, innerException As Exception)
                MyBase.New(message, innerException)
        End Sub
    End Class
    
  3. Lets also use some of .Net's built in features, specifically belonging to the string class.

    String.IsNullOrEmpty(theString)
    

    will replace the condition for checking null-ness and emptyness, I would recommend you look at IsNullOrWhiteSpace as well, this is good if you may be getting some blank spaces newlines or tabs with your data.

  4. (Optional) Instead of comparing your Enumeration to 0, implement a MyEnumeration.NoValue value, this way in-case someone ever changes the default values of enum this code will still work. Or if you don't want to change the enum, you could just check to see if the value given is a valid value in your enumeration.

Now your code will look like this

Public Function MyProjectFuncion(firstObject As Object, myEnumeration As Enumeration, theString As String, someCollection As List(Of Object), anotherObject As Object) As String
    If (firstObject Is Nothing) Then
        Throw New MissingDataException("No data was received in 'firstObject'")
    End If
    If (anotherObject Is Nothing) Then
        Throw New MissingDataException("No data was received in 'anotherObject'")
    End If
    If (myEnumeration Is Nothing) OrElse myEnumeration = 0 Then
        Throw New MissingDataException("No data was received in 'myEnumeration'")
    End If
    If String.IsNullOrEmpty(theString) Then
        Throw New MissingDataException("No data was received in 'theString'")
    End If
    If (someCollection Is Nothing) OrElse someCollection.Count = 0 Then
        Throw New MissingDataException("No data was received in 'someCollection'")
    End If
    Return "All the variables have value"
End Function

I do want to address the way this method is functioning. It would appear that you have gotten some data, and now are checking the data. I know you said this is someone else's code, but it should be changed. My assumption is that outside this method call is a try catch statement that is catching the exception and displaying or logging the exception message, otherwise display/logging the "It worked" message. I am guessing this is the case because the person who wrote this code didn't realize how much better it would be to just return two values.

Notice at the end of the method signature is a new parameter called succeeded

Public Function MyProjectFuncion(firstObject As Object, myEnumeration As Enumeration, theString As String, someCollection As List(Of Object), anotherObject As Object, ByRef succeeded As Boolean) As String
    succeeded = False
    If (firstObject Is Nothing) Then
        Return "No data was received in 'firstObject'"
    End If
    If (anotherObject Is Nothing) Then
        Return "No data was received in 'anotherObject'"
    End If
    If (myEnumeration Is Nothing) OrElse myEnumeration = 0 Then
        Return "No data was received in 'myEnumeration'"
    End If
    If String.IsNullOrEmpty(theString) Then
        Return "No data was received in 'theString'"
    End If
    If (someCollection Is Nothing) OrElse someCollection.Count = 0 Then
        Return "No data was received in 'someCollection'"
    End If

    succeeded = True
    Return "All the variables have value"
End Function

Now the succeeded value is available after the method returns, and will only be true if all values past the test, and now there is no nasty exception throwing, which hurts your application's performance and is just bad practice.

You can call the method like this now. WITHOUT a try..catch surrounding it

Dim wasSuccess As Boolean
Dim message As String = MyProjectFuncion(firstObject, myEnumeration, theString, someCollection, anotherObject, wasSuccess)

If wasSuccess Then
    'What you do when it works
Else
    'What you do normally when there is an exception in that method
End If
share|improve this answer
    
Thanks for your answer, it's very complete, but I'm afraid there's no Try Catch on the caller method, which is bad. I've already implemented a MyException Class, but I think it's still a bad practice to raise any of this exceptions, this should be an informative return and only tell which object is invalid. –  Luis Apr 11 at 22:25
    
@Luis That is indeed very bad. I wonder what the original authors purpose behind giving specific messages to each exception if he never catches them. You are indeed correct this is just a simple information gathering method, and you should probably use it as such... perhaps just do what I did above and not worry about when it fails. (as long as that isnt a breaking change) –  BenVlodgi Apr 13 at 16:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.