4 ms·

I really dislike this pattern: try { id = int.Parse(inputId); } catch (Exception ex) when (ex is FormatException or OverflowException)

by xnorswap 10mo ago

I really dislike this pattern: try { id = int.Parse(inputId); } catch (Exception ex) when (ex is FormatException or OverflowException) { throw new InvalidOperationException("DeactivateUser failed at: parse id", ex); } Where all you're doing when you catch an exception is throwing it in a more generic way. You could just let the FormatException or OverflowException bubble up, so the parent can handle those differently if needed. If you want to hide that implementation detail, then you should still consider throwing more specific types of exception for different errors. Just throwing InvalidOperationException for everything feels lazy. You've gone out your way to destroy the information being passed up when using exceptions, to demonstrate the value in having error types. It would be far more conventional to also provide a `TryDeactivateUser` function that cannot throw an exception. The article does note this, but hand-waves it away. I'm not against Result types, but I don't find this article entirely convincing.

magicalhippo 10mo ago
The original exception is available[1] in the InnerException though, so upstream can handle those differently. [1]: https://learn.microsoft.com/en-us/dotnet/api/system.invalidoperationexception.-ctor?view=net-10.0#system-invalidoperationexception-ctor(system-string-system-exception) https://learn.microsoft.com/en-us/dotnet/api/system.invalido...
- xnorswap 10mo ago
  Ok, "destroy" was too strong, "hide" would have been a better choice of words. You've still made it harder to handle.
- moogly 10mo ago
  That works somewhat for 2 levels of exceptions. When you start having dig into several levels of inner exceptions, gods help you.
deleted 10mo ago
[deleted]
bob1029 10mo ago
This code path leaves entire classes of unhandled parse exceptions on the table. I have a hard time believing this could be intentional. The safest and most concise approach is to use int.TryParse on the inputId and throw if it returns false.
- xnorswap 10mo ago
  Does it? Int.Parse says it can only return those 2 exceptions or ArgumentNullException, but nulls have been handled already. https://learn.microsoft.com/en-us/dotnet/api/system.int32.parse?view=net-10.0 https://learn.microsoft.com/en-us/dotnet/api/system.int32.pa...
  - bob1029 10mo ago
    Fine but with all that code we are implying there is some case we don't want to catch for some reason. If any effective parse error should always throw we should simply do that instead of playing games.
    - bonesss 10mo ago
      You’re not accounting for what the example actually does. The example provides exception conformance at the API level and specific logging information for tracing and debugging. It’s not playing games, it’s simplifying details for upstream consumers and explaining their contribution to the issue, while being explicit in its intentioned failure modes and failure behaviour. This code cannot tell callers why the callers have sent it garbage or what to do about that, it is failing and explaining why. Throwing “invalid : bad user id” is substantively different than rethrowing “index out of bounds : no string character at index -1”. The wrapped exception has all the original detail, its just unified and more descriptive.
fuzzy2 10mo ago
Doing it like this (or preferably with a custom exception) translates the technical problem into a domain problem. Without doing this, callers can't properly handle it. FormatException or OverflowException could be thrown at multiple locations, not just in parsing the user ID. This here is an InvalidUserIdException. It could be derived from ArgumentException, but IMHO InvalidOperationException is not appropriate.
- reactordev 10mo ago
  Translating into a custom exception is the way to go here. Bubbling up exceptions from your abstractions is fine for development but not a good experience for users of your API. I would rather see custom exceptions thrown than rethrowing.
- xnorswap 10mo ago
  You're right that domain specific exceptions would be much better. As an aside, generating domain specific exceptions is precisely the kind of busywork that traditionally it is hard to find motivation to do but that LLMs excel at.
  - bonesss 10mo ago
    Re: Domain Specific Exceptions Code snippets in IDEs like Visual Studio and refactoring tools like Resharper offer shortcut accessible auto generation of those kinds of entities with deterministic results. They also have routines to extract and move entities to their own files afterwards in a keyboard based workflow. They are far less work than a prompt, faster, can be shared in the project or team, and are guarantee-able to confirm to coding standards, logging specifics and desired inheritance chain for your root domain exception. It works on-prem, offline, for zero tokens.
  - fuzzy2 10mo ago
    I mean, at least on modern C#, it could be as simple as public class UserIdInvalidException(Exception innerException) : Exception("Invalid User ID", innerException); Even easier than most data objects you’d have to define anyway. And then, Exceptions are part of the contract. I’d rather not have an LLM write that up, but that’s just personal preference.
naasking 10mo ago
You're leaking implementation details if you let exceptions bubble. Sometimes this is ok if all of the callers are aware of the implementation details anyway, but it can make refactoring or changing implementations more difficult otherwise.
- wvenable 10mo ago
  You should leak implementation details on exceptions -- if an operation fails because of a network timeout or file access issue, that's useful information. Most exceptions cannot be meaningfully caught anyway so let me log with a good stack trace and be done with it.
  - naasking 10mo ago
    The inner, wrapped exception is logged. Leaking exception details can also leak privileged information, and if you're not careful this can leak information to attackers too. More information is not necessarily better.
    - wvenable 10mo ago
      If your error logging is leaking privileged information to attackers that's a completely different problem from what you should do in code when throwing exceptions. Wrapping exceptions to remove information is mostly a pointless exercise. You should be doing it only to add additional context.
      - naasking 10mo ago
        It's not a different problem, my whole point was that letting exceptions bubble is not a universally acceptable policy. Sometimes you want to bubble, sometimes you want to wrap, and sometimes you want to wrap with information hiding to avoid leaking information.
        wvenable 10mo ago
torginus 10mo ago
That's not how you're supposed to handle this kind of errors (according to .NET designers) - there's 2 kinds of errors in concept, everyday errors that are to be expected, such as a dictionary not containing a key, or in this case, a user supplying a badly formatted integer. For this you have the Try.. methods with TryGetValue, TryParse etc. Go for example, allows for multiple return values, so it allows more elegant handling of this exact cass. Then there's the serious kind of error, when something you didn't expect goes wrong. That's what exceptions are for. If this distinction is followed, then you don't want to handle specific exceptions (with very few notable distinctions, like TaskCanceledException), you just either pick a recoverable function scope (like a HTTP handler), and let the exception bubble to its top, at which point you report an error to the user, and log what happened. If such a thing is not possible, just let the program crash.

magicalhippo 10mo ago

The original exception is available[1] in the InnerException though, so upstream can handle those differently. [1]: https://learn.microsoft.com/en-us/dotnet/api/system.invalidoperationexception.-ctor?view=net-10.0#system-invalidoperationexception-ctor(system-string-system-exception) https://learn.microsoft.com/en-us/dotnet/api/system.invalido...

xnorswap 10mo ago
Ok, "destroy" was too strong, "hide" would have been a better choice of words. You've still made it harder to handle.
moogly 10mo ago
That works somewhat for 2 levels of exceptions. When you start having dig into several levels of inner exceptions, gods help you.

deleted 10mo ago

[deleted]

bob1029 10mo ago

This code path leaves entire classes of unhandled parse exceptions on the table. I have a hard time believing this could be intentional. The safest and most concise approach is to use int.TryParse on the inputId and throw if it returns false.

xnorswap 10mo ago
Does it? Int.Parse says it can only return those 2 exceptions or ArgumentNullException, but nulls have been handled already. https://learn.microsoft.com/en-us/dotnet/api/system.int32.parse?view=net-10.0 https://learn.microsoft.com/en-us/dotnet/api/system.int32.pa...
- bob1029 10mo ago
  Fine but with all that code we are implying there is some case we don't want to catch for some reason. If any effective parse error should always throw we should simply do that instead of playing games.
  - bonesss 10mo ago
    You’re not accounting for what the example actually does. The example provides exception conformance at the API level and specific logging information for tracing and debugging. It’s not playing games, it’s simplifying details for upstream consumers and explaining their contribution to the issue, while being explicit in its intentioned failure modes and failure behaviour. This code cannot tell callers why the callers have sent it garbage or what to do about that, it is failing and explaining why. Throwing “invalid : bad user id” is substantively different than rethrowing “index out of bounds : no string character at index -1”. The wrapped exception has all the original detail, its just unified and more descriptive.

fuzzy2 10mo ago

Doing it like this (or preferably with a custom exception) translates the technical problem into a domain problem. Without doing this, callers can't properly handle it. FormatException or OverflowException could be thrown at multiple locations, not just in parsing the user ID. This here is an InvalidUserIdException. It could be derived from ArgumentException, but IMHO InvalidOperationException is not appropriate.

reactordev 10mo ago
Translating into a custom exception is the way to go here. Bubbling up exceptions from your abstractions is fine for development but not a good experience for users of your API. I would rather see custom exceptions thrown than rethrowing.
xnorswap 10mo ago
You're right that domain specific exceptions would be much better. As an aside, generating domain specific exceptions is precisely the kind of busywork that traditionally it is hard to find motivation to do but that LLMs excel at.
- bonesss 10mo ago
  Re: Domain Specific Exceptions Code snippets in IDEs like Visual Studio and refactoring tools like Resharper offer shortcut accessible auto generation of those kinds of entities with deterministic results. They also have routines to extract and move entities to their own files afterwards in a keyboard based workflow. They are far less work than a prompt, faster, can be shared in the project or team, and are guarantee-able to confirm to coding standards, logging specifics and desired inheritance chain for your root domain exception. It works on-prem, offline, for zero tokens.
- fuzzy2 10mo ago
  I mean, at least on modern C#, it could be as simple as public class UserIdInvalidException(Exception innerException) : Exception("Invalid User ID", innerException); Even easier than most data objects you’d have to define anyway. And then, Exceptions are part of the contract. I’d rather not have an LLM write that up, but that’s just personal preference.

naasking 10mo ago

You're leaking implementation details if you let exceptions bubble. Sometimes this is ok if all of the callers are aware of the implementation details anyway, but it can make refactoring or changing implementations more difficult otherwise.

wvenable 10mo ago
You should leak implementation details on exceptions -- if an operation fails because of a network timeout or file access issue, that's useful information. Most exceptions cannot be meaningfully caught anyway so let me log with a good stack trace and be done with it.
- naasking 10mo ago
  The inner, wrapped exception is logged. Leaking exception details can also leak privileged information, and if you're not careful this can leak information to attackers too. More information is not necessarily better.
  - wvenable 10mo ago
    If your error logging is leaking privileged information to attackers that's a completely different problem from what you should do in code when throwing exceptions. Wrapping exceptions to remove information is mostly a pointless exercise. You should be doing it only to add additional context.
    - naasking 10mo ago
      It's not a different problem, my whole point was that letting exceptions bubble is not a universally acceptable policy. Sometimes you want to bubble, sometimes you want to wrap, and sometimes you want to wrap with information hiding to avoid leaking information.
      - wvenable 10mo ago

torginus 10mo ago

That's not how you're supposed to handle this kind of errors (according to .NET designers) - there's 2 kinds of errors in concept, everyday errors that are to be expected, such as a dictionary not containing a key, or in this case, a user supplying a badly formatted integer. For this you have the Try.. methods with TryGetValue, TryParse etc. Go for example, allows for multiple return values, so it allows more elegant handling of this exact cass. Then there's the serious kind of error, when something you didn't expect goes wrong. That's what exceptions are for. If this distinction is followed, then you don't want to handle specific exceptions (with very few notable distinctions, like TaskCanceledException), you just either pick a recoverable function scope (like a HTTP handler), and let the exception bubble to its top, at which point you report an error to the user, and log what happened. If such a thing is not possible, just let the program crash.