1
\$\begingroup\$

I've asked a question receiving excessive (in a good sense) feedback. After I've incorporated it into the code, it changed substantially and I felt that I'd need opinion on the new version. Posting revised code is against the rules, so I'm asking a new, follow-up question.

  1. Choice of exception names. Are those informative enough?.
  2. Introduction of tiered exceptions. Are more levels of inner exceptions needed?).
  3. Omission of the values. Should I provide the faulty ones?

Code

private static void Validate(Guid userId, EntityState state, object data)
{
  if (userId == Guid.Empty)
    throw new IdentityNotMappedException("Token unrecognized!",
      new ArgumentException("Can't be Guid.Empty or null!", nameof(userId)));
  if (state == EntityState.Detached)
    throw new InvalidOperationException("Action unrecognized!",
      new ArgumentException("Must follow C_UD!", nameof(state)));
  if (data == null)
    throw new MappingException("Data unrecognized!",
      new ArgumentException("Must be deserializeable!", nameof(data)));
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I'm generally strongly against unthrown exceptions. An exception without its stack trace is simply a named container for an error message. You're using this kind of exceptions for innerException parameter of the exceptions your really throw:

throw new IdentityNotMappedException("", new ArgumentException("", nameof(userId)));

There is no need to have an inner exception if outer one has not been originated by that. When you feel that needing you are, probably, using wrong exception.

There is no need for ArgumentException and userId cannot be null because it is a value type. IdentityNotMappedException alone itself is sufficient to "describe" your error condition (but note that, according to your true code, I'd use a more descriptive error message).

if (userId == Guid.Empty)
    throw new IdentityNotMappedException("User identity is unknown.");

"Action unrecognized" is not a very useful error message (same as "Must follow C_UD!"). Your error messages must quickly describe what's wrong (hints to solve the problem are welcome but optional).

if (state == EntityState.Detached)
    throw new ArgumentException("Entity can not be in Detached state.", nameof(state));

If state is a function argument then you should throw ArgumentException, if it's object state then you may throw InvalidOperationException:

if (state == EntityState.Detached)
    throw new InvalidOperationException("Entity can not be in Detached state.");

Let me stress again to use a better error description. You're checking for a parameter vs null; there may be multiple reasons for this (caller programming error, invalid object state), unless you're sure about what's going on then limit yourself to say what you actually know (argument is null).

if (data == null)
    throw new ArgumentNullException(nameof(data));

To answer your questions directly:

Choice of exception names. Are those informative enough?.

Last exception is not right one (IMO) because you can be more specific. First one can be right if (and only if) you're actually working with System.Security.Principal stuff. If not then you'd better to do not use it because it carries extra context that it's not applicable in your case.

Introduction of tiered exceptions. Are more levels of inner exceptions needed?).

No, you do not even need these inner exceptions, for sure you don't need more. As I already said an exception without stack trace is merely a named container for an error message and moreover you give a false information: IdentityNotMappedException has been originated because of a catched ArgumentException. If you want to give more information don't forget that each exception has a Data dictionary for this purpose:

if (state == EntityState.Detached) {
    var e = new ArgumentException("Cannot use detached entity state.", nameof(state));
    e.Data.Add("Hint", "Must follow C_UD");

    throw e;
}

Omission of the values. Should I provide the faulty ones?

When value is implicit you don't need to provide it (for example if you're throwing because state is EntityState.Detached then you don't need to repeat, same when you throw ArgumentNullException). You have this doubt just because your error messages are not actually describing what is wrong.

<edit>
As you already noted by yourself these error messages may be as much cryptic as original ones. Context will help to clarify them when required, if - for example - entity state is an unknown implementation detail (from caller point of view) then it has no use to expose it and a more abstract error message is preferable.
</edit>

In your example there are no need to specify any faulty value but, for example, in this situation you should do it:

if (entity.State != EntityState.Unchanged)
    throw ArgumentException($"Invalid entity state {entity.State} (state must be Unchanged).", nameof(entity));
\$\endgroup\$
2
  • \$\begingroup\$ Very good points. As I read it from your angle, I realized that C_UD might be very confusing outside the context. In fact, I refer to CRUD operations, a very canonical term, but since there's no retrieval (only accepting Create-_-Update-Delete). But boy - I was a bit confused myself by that string when I saw you post it! \$\endgroup\$ Jan 12, 2016 at 11:24
  • \$\begingroup\$ Absolutely! Nice (in the sense of useful) error messages need some knowledge of context, entity state (for example) may be a completely unknown implementation detail (from caller point of view). \$\endgroup\$ Jan 12, 2016 at 11:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.