# Is there a better way to optimize these conditionals?

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

• 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 '14 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 '14 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 '14 at 20:27
• 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 '14 at 20:28

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.

• 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 '14 at 20:45
• [...]those exceptions weren't significant at all when it came to performance That's the conclussion of the article you gave me, for what I understand, it tells me it would be good to use exceptions cause they are not that expensive. – Luis Apr 11 '14 at 22:12
• 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 '14 at 23:30
• Yes, it is, in fact I agree with you. – ignis Apr 11 '14 at 23:35
• 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 '14 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

• 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 '14 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 '14 at 16:38