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
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.
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
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
How about returning a flag enum that contains all validation errors? (not sure of VB syntax here, my background is C#):
Then the function can return a
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.
|show 5 more comments|
Now your code will look like this
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
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