[xnorswap]

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.

[fuzzy2]

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]

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]

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]

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]

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.

[magicalhippo]

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.invalido...

[xnorswap]

Ok, "destroy" was too strong, "hide" would have been a better choice of words.

You've still made it harder to handle.

[moogly]

That works somewhat for 2 levels of exceptions. When you start having dig into several levels of inner exceptions, gods help you.

[bob1029]

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]

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.pa...

[bob1029]

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]

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.

[torginus]

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.

[naasking]

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]

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]

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]

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]

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]

> my whole point was that letting exceptions bubble is not a universally acceptable policy.

It should be. It should bubble to whatever boundary you have (web API, event loop, etc). At that boundary, if it's not supposed to leak information then don't. Do whatever sanitation you need at one point only. Good use of exceptions should have as few "catch" blocks as possible.