3 ms·

catch if(Log()) Doesn't look great. It's not easy to understand what this code does. I'd better rewrite it this way: catch if(IsExceptionLogged()) So we can

catch if(Log()) Doesn't look great. It's not easy to understand what this code does. I'd better rewrite it this way: catch if(IsExceptionLogged()) So we can see here "Is" keyword, it gives us a hint that this function returns boolean. We can see the main operation, and it's more human-readable. And we can easily understand what do we mean here. Because in case of Log() we have to think - should we catch if it was logged or if it was not logged. Maybe it is useful in some cases. But unfortunately the code quality doesn't depend on the tools one uses, and it's nothing about language features. 99% of developers don't know how to use C# 1.0. There are so still a lot for them to learn. Maybe even catch if(IsExceptionHandled()) is better. Because I don't see any reasons why should we catch exceptions if it was logged. Yep, it was logged - is it a good decision factor to catch? Logging often can be disabled in configuration.

RomanPushkin 12y ago
PS It's also a bad idea to compare exception messages. Well, I know, it's a sample, but just wanna point that it's not the best one. First - exception messages could be internationalized. Second - general string comparison should be done with InvariantCulture, and case-ignorant (and usually strings are trimmed). And even in that case it's not a good idea. It's better to compare types, or message codes.
DougBTX 12y ago
Calling the Log function there is a hack to work around the stack being unwound inside catch blocks, it isn't being used to control whether the catch block is run based on whether logging is turned on or not, so those suggested alternative names seem confusing. "IsExceptionLogged" implies that the function is being called for its return value, to work out whether the exception is being looked or not, but in this usecase the Log function is being called for its side affects, and the return value is a boolean simply to appease the type checker.

RomanPushkin 12y ago

PS It's also a bad idea to compare exception messages. Well, I know, it's a sample, but just wanna point that it's not the best one. First - exception messages could be internationalized. Second - general string comparison should be done with InvariantCulture, and case-ignorant (and usually strings are trimmed). And even in that case it's not a good idea. It's better to compare types, or message codes.

DougBTX 12y ago

Calling the Log function there is a hack to work around the stack being unwound inside catch blocks, it isn't being used to control whether the catch block is run based on whether logging is turned on or not, so those suggested alternative names seem confusing. "IsExceptionLogged" implies that the function is being called for its return value, to work out whether the exception is being looked or not, but in this usecase the Log function is being called for its side affects, and the return value is a boolean simply to appease the type checker.