Once the cat is naked, would it prefer a fur coat or latex? (Or, being a geek - Part 2)
Last night I started what ended up being a pretty long discussion on a pretty small piece of code. I talked a bit about things you could do to clean it up a bit, and get it perhaps a little stabler. As a refresher, let's pretend those changes I talked about were implemented, and we can look at the new version.
Dim lastException As Exception
Dim ws As TheWebService
Try
ws = New TheWebService
For retry As Integer = 1 To MAX_RETRY_COUNT
Try
'Use the web-service...and then
lastException = Nothing
Exit For
Catch wex As System.Net.WebException
'a web exception is our likely candidate for a retryable call
lastException = wex
Catch ex As Exception
'generic unhandled case - no point retrying
lastException = ex
Exit For
End Try
Thread.Sleep(SOME_DELAY)
Next
Finally
If Not ws Is Nothing Then
ws.Dispose()
End If
End Try
If Not lastException Is Nothing Then
'We got through all of the retries without succeeding...need to do something here.
End If
What we have here now is something we're a lot more sure of that actually does all the things we want it do. The client proxy isn't recreated on each loop iteration, we've put a delay in give the remote server a chance to finish rebooting, and we've filtered the exceptions to only retry on exceptions that could possibly not happen if we tried again (there's likely more that can be done to filter further, but it's good enough for this discussion).
We're now in free time. The code is 'good enough' to go into some code at work where time is precious and we can't spend hours making sure it's picture perfect. But I'm in my own time now where it's nice to spend the time to really think about it - and yes, I have a life! The kids (that is, my daughter and my wife), and I'm still awake :)
One thing I really don't like is keeping track of whether something went wrong by keeping a reference around to the last exception object. Exceptions can be pretty bulky objects - why should I keep it around any longer than I absolutely have to? If you think about what's going on, we can remove it entirely. While I didn't show it there, in the last if block where there's a test for the last exception existing, all that was being done was a 'Throw lastException'. In this case, we didn't need to keep track of it at all - we already have a way of knowing if it's time to throw a doozy up the call stack - the loop counter.
Dim ws As TheWebService
Try
ws = New TheWebService
For retry As Integer = 1 To MAX_RETRY_COUNT
Try
'Use the web-service...and then
Exit For
Catch wex As System.Net.WebException
If retry = MAX_RETRY_COUNT Then
Throw
End If
Catch ex As Exception
Throw
End Try
Thread.Sleep(SOME_DELAY)
Next
Finally
If Not ws Is Nothing Then
ws.Dispose()
End If
End Try
At the time of the exception, we throw it again - if we can't retry (in the case of the generic catch) or can't retry any more (in the case of the WebException catch).
The best part of this approach is that now the exception is not re-thrown - it's just thrown. What's the difference? If you catch an exception and throw a new one (or the same one) and whole new exception raising event happens. But if you can just 'Throw' with no arguments (only valid in a catch block) then the current exception just continues on as if it was never caught in the first place. Exceptions are expensive, and at least we've made it just that little bit cheaper.
Now, what about that loop? We're going to need some sort of looping construct to retry our call - there's no avoiding that. Well, yes, we right a recursive function I suppose, but happens when some rocket scientist decides to make MAX_RETRY_COUNT 10000 (in an extremely naive attempt to make the call more likely to happen eventually)? We get a pretty damn massive call stack, that's what - probably best not to go down that path :)
And this where the cat that has been (skun? skinned? skan?) is really relevant. There's so many ways to loop - which one is best? Let's have a look at some. You've already seen the For loop, so I won't repeat it. I'm also certainly not going to shape it into a For Each loop - that would be silly.
Do While
Do While retry <= MAX_RETRY_COUNT
Try
'Use the web-service...and then
Exit Do
Catch wex As System.Net.WebException
If retry = MAX_RETRY_COUNT Then
Throw
End If
Catch ex As Exception
Throw
End Try
Thread.Sleep(SOME_DELAY)
retry += 1
Loop
There's a similar loop that can be used just like the Do While loop.
While
While retry <= MAX_RETRY_COUNT
Try
'Use the web-service...and then
Exit While
Catch wex As System.Net.WebException
If retry = MAX_RETRY_COUNT Then
Throw
End If
Catch ex As Exception
Throw
End Try
Thread.Sleep(SOME_DELAY)
retry += 1
End While
This puppy is exactly the same, just different words. I prefer the Do While over the While, but that's a habit from the old days - I hate that shocking word 'Wend'.
Do Until
Do Until retry > MAX_RETRY_COUNT
Try
'Use the web-service...and then
Exit Do
Catch wex As System.Net.WebException
If retry = MAX_RETRY_COUNT Then
Throw
End If
Catch ex As Exception
Throw
End Try
Thread.Sleep(SOME_DELAY)
retry += 1
Loop
Both of the Do While and Do Until work almost the same way - a while loop will loop so long as the while expression is true. An until loop will loop so long as the until expression is false. There's a small difference, but so much more meaning can be gained if you use each in the right context.
In both these cases, there's a test that's performed to see whether the loop should be entered. This test is insignificant in this case - a simple test of an integer is pretty cheap (as far as I'm concerned, one of the cheapest there is...there might be cheaper, but a numeric equality test - especially if that number is a whole number that fits within one single register - is pretty cheap) - but you could as easily call a method that returns a Boolean. This method might cost a lot.
But why should we test the first time? We damn well want it to go through the loop at least once right? If we didn't, there was no point calling this whole function in the first place.
Do
'the same code as before....
retry += 1
Loop While retry <= MAX_RETRY_COUNT
Do
'the same code as before....
retry += 1
Loop Until retry > MAX_RETRY_COUNT
Now it's test at the end of the loop. What's the difference? Well, this is pretty much the same as saying that it tests at the start of each loop - but only from the second iteration onwards. Have we saved much? Not in this case...but in the case of having some while expression that cost a lot, then hell yeah.
It's not actually that likely that we'd need to retry a call to a web service. 99% of the time, we're going to call the web method, it will succeed, and we'll exit the loop. So in the general scheme of things, by moving the while test from the top to the bottom, we've decreased the number of tests performed from 1 to 0. That's a saving of 100% - it's as if we've never even entered the loop at all. There's certainly no real loop code executed when you get down to the IL - the test for the value and the branch to go back to the start of the loop get skipped over, so it's like the loop never even existed.
We can take this even further. Often times, people shy away from infinite loops.
Infinite loops are bad. What's an infinite loop look like? We just remove the while expression, and we loop forever.
Do
'some code in here
Loop
Zoom zoom zoom, round we go. Let me out of here! But these sort of loops are fine to use, so long as you can guarantee that somewhere within the loop you'll eventually reach a condition where you can bail out.
Do
Do
'some code in here
If someExpression Then
Exit Do
End If
'some more code
Loop
So long as you can make absolutely sure that an Exit Do call will be made (or some other way of escaping), we're laughing. So let's take out the while test from before.
Do
Try
'Use the web-service...and then
Exit Do
Catch wex As System.Net.WebException
If retry = MAX_RETRY_COUNT Then
Throw
End If
Catch ex As Exception
Throw
End Try
Thread.Sleep(SOME_DELAY)
retry += 1
Loop
This is exactly the same as the first Do While example from before, but without the While statement. Are we stuck now?
Hell no.
The code never actually had a chance to ever reach a point where the while expression was evaluated and the loop was terminated. The try block, if it works, exits the loop. The generic catch immediately exits the loop by throwing an exception. The WebException catch also exits the loop, just before retry could be incremented for the last time.
In that case, why do we need an official loop construct at all?
GoTo
TryAgain:
Try
'Use the web-service...and then
GoTo Finished
Catch wex As System.Net.WebException
If retry = MAX_RETRY_COUNT Then
Throw
End If
Catch ex As Exception
Throw
End Try
Thread.Sleep(SOME_DELAY)
retry += 1
GoTo TryAgain
Finished:
There's actually no difference (compiled form) between this method and the endless loop above. Even reflector, when showing the disassembly in VB form, writes this code back out again as an endless loop.
But which way is the best way? Using the endless loop method (either a Do with no condition, or GoTo) is certainly the most 'optimal'. Now we have to fall back to thinking of the poor sod who has to maintain this mess is the future.
Which way is more readable? Which way is most understandable when read for the first time? I think the For loop needs to go. By specifying the range of the retry variable like that, it gives the impression that we have more control over this loop than we really have. I want to go with the endless loop version, because I'm like that - I think it's clear enough what's going on...but I've often been proven wrong on this point.
What do you all think? Would you do any more to it? Which loop would you use in the end?
Listening to: too long - daft punk - (10:00)
Update: I've realised a stupid thing I've done. If you're only catching to throw again, why bother catching? Every occurence of 'Catch ex as Exception' should be stricken from the record. I'm leaving it in to show that even I am not perfect :)