Poor Coding?

[Brian P]

I have a try block that attempts to call a web service. The web service
requires a token that can expire at any time, so I am using a catch block
to refresh the token and then try the call to the web service again. This
seems a little poor the way i'm doing it, so just wanted to get a second
opinion:

is there a better way than using a goto statement?


public static string GetData(string Criteria)
{

tryagain:
try
{
CallWebService();
}
catch (SoapException SoapEx)
{
if (Soap.Ex.Message = "Bad Token")
{
RefreshLogon();
goto tryagain;
}
}


}

 

[Paul E Collins]

[...]
is there a better way than using a goto statement?

I think "goto" is the least of your problems here. Throwing and
catching exceptions incurs a significant performance hit, and you
definitely don't want to be doing it in a busy loop like this. You
should never use them for control flow if you can avoid it.

You didn't explain exactly what CallWebService does. Does it have to
throw an exception? And is it really meaningful to call it every few
milliseconds, or could you use a timer and perhaps raise some kind of
event when you get new data?

P.

 

[Jon Skeet [C# MVP]]

I have a try block that attempts to call a web service. The web service
requires a token that can expire at any time, so I am using a catch block
to refresh the token and then try the call to the web service again. This
seems a little poor the way i'm doing it, so just wanted to get a second
opinion:

is there a better way than using a goto statement?

Yes - use a while loop. You could either keep a boolean flag, or
"break" on the success case. Here's your code in each way:

bool success=false;
while (!success)
{
try
{
CallWebService();
success = true;
}
catch (SoapException ex)
{
if (ex.Message=="Bad Token")
{
RefreshLogin();
}
// Your code didn't have this, but I doubt you want to keep
// going if a different SoapException has been thrown...
else
{
throw;
}
}
}

and

while (true)
{
try
{
CallWebService();
break;
}
catch (SoapException ex)
{
if (ex.Message=="Bad Token")
{
RefreshLogin();
}
// Your code didn't have this, but I doubt you want to keep
// going if a different SoapException has been thrown...
else
{
throw;
}
}
}

Alternatively, you could use a do/while, with a continue:

while (true)
{
try
{
CallWebService();
}
catch (SoapException ex)
{
if (ex.Message=="Bad Token")
{
RefreshLogin();
continue;
}
// Your code didn't have this, but I doubt you want to keep
// going if a different SoapException has been thrown...
else
{
throw;
}
}
} while (false);

 

[Brian P]

Hello Paul,

Yes, it will throw an exception if the security token is too old, but i have
no idea when is "too old."

This is generic on purpose, since there are about 12 or so methods exposed
by the web service that I will be using, some will be called several times
a minute, others only once a day. The key here was that i need to be able
to handle the "too old" token exception and retry.

If it were up to me, there'd be a way to see if the token was expired, but
i didn't write the web service.=)

--Brian

[...]
is there a better way than using a goto statement?

I think "goto" is the least of your problems here. Throwing and
catching exceptions incurs a significant performance hit, and you
definitely don't want to be doing it in a busy loop like this. You
should never use them for control flow if you can avoid it.

You didn't explain exactly what CallWebService does. Does it have to
throw an exception? And is it really meaningful to call it every few
milliseconds, or could you use a timer and perhaps raise some kind of
event when you get new data?

P.

 

[Brian P]

Yes - use a while loop. You could either keep a boolean flag, or
"break" on the success case. Here's your code in each way:

Ahh, a while loop makes perfect sense! Sheesh!!!

--Bria

 

[Jon Skeet [C# MVP]]

[...]
is there a better way than using a goto statement?

I think "goto" is the least of your problems here. Throwing and
catching exceptions incurs a significant performance hit, and you
definitely don't want to be doing it in a busy loop like this.

It's a web service call. Somehow, I don't think the exception is going
to be the bottleneck, even if the web service call is to the local
machine.

Exceptions aren't nearly as expensive as people seem to think. See
http://www.pobox.com/~skeet/csharp/exceptions.html

You should never use them for control flow if you can avoid it.

In this case it seems fairly reasonable though. A call failed because
of a condition which presumably couldn't be easily tested; there's an
action which can remedy the situation so that the logic can proceed.

 

[Jon Skeet [C# MVP]]

If it were up to me, there'd be a way to see if the token was expired, but
i didn't write the web service.=)

Even if you could, you'd still need to have the same kind of trap,
wouldn't you? Just because a token is valid when you check it doesn't
mean it's going to be valid by the time you make the call to the web
service immediately afterwards...

 

[Ollie Riches]

If the web service is throwing an exception because a value has expired
becuase of some business rule - this is bad design. You should only throw
exceptions because of unexpected behaviour not because of a business rule.
You would be better of returning some sort of flag (bool, enum, int etc)
indicating that the token has expired and that a new token is required.

HTH

Ollie Riches

 

[Brian P]

Even if you could, you'd still need to have the same kind of trap,
wouldn't you? Just because a token is valid when you check it doesn't
mean it's going to be valid by the time you make the call to the web
service immediately afterwards...

I guess that would depend on how the check was implemented.

If it were up to me, it would compare the current time to the expiration
time, and as long as there was say 30 seconds before it was expired, then
return "not expired" otherwise return "expired". Yes, i see your point that
there is a pitfall there too.

Oh well, I appreciate the loop idea, its far better than goto!

--Brian

 

[Guest]

[...]
is there a better way than using a goto statement?

I think "goto" is the least of your problems here. Throwing and
catching exceptions incurs a significant performance hit, and you
definitely don't want to be doing it in a busy loop like this. You
should never use them for control flow if you can avoid it.

actually I think this is a fairly sensible way of handling it. remember,
the goal is not to totally avoid using exceptions. they are in the language
for a reason. especially that's the only way he can recover and continue
processing.

I also think the use of "goto" is probably fine here. especially the while
loop alternative isn't exactly that much "superior".

 

[Ollie Riches]

[...]
is there a better way than using a goto statement?

I think "goto" is the least of your problems here. Throwing and
catching exceptions incurs a significant performance hit, and you
definitely don't want to be doing it in a busy loop like this. You
should never use them for control flow if you can avoid it.

actually I think this is a fairly sensible way of handling it. remember,
the goal is not to totally avoid using exceptions. they are in the
language
for a reason. especially that's the only way he can recover and continue
processing.

No - Exceptions are for UNEXPECTED behaviour not for implementing business
logic\rule.

I also think the use of "goto" is probably fine here. especially the
while
loop alternative isn't exactly that much "superior".

A goto is not very OO is it And yes I do know that the try catch
mechanism does make use of the goto statement under the covers.

HTH

Ollie Riches

 

[Guest]

The dreaded status code / return code. I think it's time we do away with
those.

throwing exception due to expired access token is fine in my opinion. You
can also say that well, when attempting to open a file, we shouldn't always
assume that file will be there, so we should use a return code rather than
FileNotFoundException.

 

[Guest]

No - Exceptions are for UNEXPECTED behaviour not for implementing business
logic\rule.

I can easily argue that failed access token is not part of the business
rule. the business rule controls how data is obtained and processed, but
whether you can see it or not is something entirely different.

A goto is not very OO is it And yes I do know that the try catch
mechanism does make use of the goto statement under the covers.

and there's nothing OO about a while loop either. the reason I am
advocating the use of "goto" here is that it really doesn't add any potential
confusion or fitfall here. it doesn't destroy readability, or make the
program flow hard to follow. compare it against the while loop, I really
dont' see one being easier to follow than the other. why avoid using it just
because it doesn't feel "right".

 

[William Stacey [MVP]]

I would also check the IsExpired property and get a new token if expired,
instead of just relying on the exception. If you know it is expired to
begin with, no since waiting for the network call and resulting exception.

 

[William Stacey [MVP]]

I think WSE does this. TMK, there is no way to override that behavior.
Then again, maybe there is some override. On the wire, however, the
exception is nothing more then an soap reply that says there was an
exception.

 

[Jon Skeet [C# MVP]]

If the web service is throwing an exception because a value has expired
becuase of some business rule - this is bad design. You should only throw
exceptions because of unexpected behaviour not because of a business rule.
You would be better of returning some sort of flag (bool, enum, int etc)
indicating that the token has expired and that a new token is required.

Euch, no - I would say that relying on clients picking up on a status
code is worse design here. You could easily phrase this in terms of a
business rule to do with security, after all. Exceptions don't have to
be that unexpected - it's not entirely unexpected for a network
connection to drop sometimes, but a network-oriented stream would still
generate an exception if the connection went down without a deliberate
close from one side or the other.

Do you have any *practical* objection against this being an exception?
Performance isn't going to be significant, and the meaning of "I can't
do what you've really asked me to do (the main success story of the
method)" is upheld.

 

[Bruce Wood]

You should only throw exceptions because of unexpected behaviour not because of a business rule.

That depends upon your perspective. From the point of view of the web
service, it may be an exception. From the point of view of the calling
application, it may be a recoverable situation.

The web service has no idea of in what context it's being called. Given
this, throwing an exception for an expired token is reasonable
behaviour.

 

[Jon Skeet [C# MVP]]

Euch, no - I would say that relying on clients picking up on a status
code is worse design here. You could easily phrase this in terms of a
business rule to do with security, after all.

Whoops, of course, you were already viewing it as a business rule.
Violating a business rule is still something which should generate an
exception in my view though - and clearly in the view of others. For
instance, trying to insert a null value into a non-nullable column is
likely to be "just" violating a business rule, but lo and behold,
you'll get an exception if you try to do it.

 

[Christof Nordiek]

I have a try block that attempts to call a web service. The web service
requires a token that can expire at any time, so I am using a catch block
to refresh the token and then try the call to the web service again. This
seems a little poor the way i'm doing it, so just wanted to get a second
opinion:

is there a better way than using a goto statement?


public static string GetData(string Criteria)
{

tryagain:
try
{
CallWebService();
}
catch (SoapException SoapEx)
{
if (Soap.Ex.Message = "Bad Token")
{
RefreshLogon();
goto tryagain;
}
}


}

Besides what others did say until now:
How many times do you want to retry when a the token is old?
If for some odd reason the token appears to be old directly after
RefreshLogon() you'll and up in an endless loop.
Supposing that it would be very odd, I'd simply let the exception
fall through and not retry again.

So I would propose:
try
{
CallWebService();
}
catch (SoaException SoapEx)
{
if (SoapEx.Message == "Bad Token")
{
RefreshLogon();
}
else
throw;
}

 

[Chris Dunaway]

No - Exceptions are for UNEXPECTED behaviour not for implementing business
logic\rule.

I have to disagree with this. Exceptions are for EXPECTED behavior.
You provide handlers for situation you anticipate might happen. When
you write a routine that reads/writes files and you reach the end of
file prematurely, wouldn't you EXPECT an exception? You code your
exception handling accordingly by providing a handler for that
possibility.

When you code something like this:

try {
// some code here
}
catch (OverflowException) {
//handler code here
}


By definition, you EXPECT that an overflow might occur.

Perhaps it's just semantics.

Anyway, my 2 cents (probably worth only 1 in this day and age).

Cheers