use more exception when statements

[SimonCropp]

No description provided.

[jodydonetti]

The 2 lines pattern (if + do log) is used everywhere in the codebase, and that (at least, to me) brings consistency and less mental burden while scanning the code.

Changing that in a couple of places only because - as of today - the "do log" is the only instruction remaining in the catch block does not seem to be worth it.

What is the reason for doing it?

[SimonCropp]

from my understanding of exception when statements, they are more efficient since if the when is false, the runtime doesnt need to build the stack trace.

[jodydonetti]

Hi @SimonCropp

from my understanding of exception when statements, they are more efficient since if the when is false, the runtime doesnt need to build the stack trace.

Found it, here.

So it looks like you were right, thanks for letting me learn something new!

Now I need to balance readability/maintainability VS the perf gain, lemme think a bit about it.

[SimonCropp]

if u put a newline before the when, then IMO the readability is the same

catch (Exception exc)
    when (logger?.IsEnabled(LogLevel.Warning) ?? false)

versus

catch (Exception exc)
{
    if (logger?.IsEnabled(LogLevel.Warning) ?? false)

[Inok]

This changes the existing behavior, though. The original implementation not only logs but also always suppresses exceptions. With when, exception will or will not be suppressed depending on whether logging is enabled.

[jodydonetti]

Damn, you're right @Inok
Now I'm off, but I'll revert this asap.
Thanks!

ps: maybe an extra catch with no when right after that, to keep the small optimization + still having the old behavior?

[Inok]

I'll revert this asap.

It's not merged yet, it seems, so don't worry. I'm just warning about consequences 🙂

ps: maybe an extra catch with no when right after that, to keep the small optimization + still having the old behavior?

If the second catch is always executed anyway, I believe there's no benefit in moving logging to a separate block.

In the context of logging, using when makes sense if you want to log exceptions but don't want to suppress them. Then, instead of rethrowing them after logging, you can use this approach:

try { ... }
catch (Exception ex) when (LogException(ex)) 
{  
   /* empty, never executed */ 
}

bool LogException(Exception ex) 
{
    if (logger.IsEnabled(...)) 
        logger.Log(ex); 

    return false;
}