3

I have 3 simple classes. A Reference, a Parent, and a Child. The Child knows the Reference and Parent instances it's associated with. Here they are, initialization and other data/methods omitted:

class Reference
{
    public int ID { get; private set; }
}

class Parent
{
    public int ID { get; private set; }
}

class Child
{
    public int ID { get; private set; }
    public Reference Reference {get; private set; }
    public Parent Parent {get; private set; }
}

Here is a class that maintains collections of them:

class SetOfThings
{
    private List<Reference> _refs;
    private List<Parent> _parents;
    private List<Child> _children;

    // ...
    // initialization, etc
    // ...
}

It enforces constraints like

  • cannot add a Child unless its Parent and Reference already exists
  • cannot add a Child if another Child with the same Parent and Reference already exists

and so on. Currently, the AddChild method enforces the constraints with ArgumentExceptions and returns the new Child with a new unique ID:

class SetOfThings  // continued
{
    public AddChild(Reference ref, Parent parent)
    {
        if (!_refs.Contains(ref))
            throw new ArgumentException($"{ref} not yet known");
        if (_children.Any(c => c.Parent == parent && c.Reference == ref))
            throw new ArgumentException($"{ref} and {parent} already referenced by a child");

        int newID = MakeNewChildID();
        var newChild = new Child(newID, ref, parent);

        _children.Add(newChild);
        return newChild;
    }
}

All of my AddChild calls look like this:

Child newChild;
try
{
    newChild = AddChild(someReference, someParent);
}
catch (ArgumentException ex)
{
    UI.Alert(ex.Message);
    return;
}

// ...
// Trigger state change, update UI with new child, etc.
// ...

However, this feels like an abuse of the exception mechanism. It isn't really "exceptional" behavior when those ArgumentExceptions are thrown - it's just normal control flow. But I don't know how to avoid this:

  • I could return a bool that indicates success/failure and use an output parameter to "return" the new child,

    bool AddChild(Reference r, Parent p, out Child c)
    

    but I don't get any indication of why a failure occurred, and output parameters sacrifice readability.

  • I could return null to indicate a failure. The method signature wouldn't change, but I still don't know why failures occur. Moreover, a null reference probably shouldn't be expected behavior any more than exceptions should be.

  • I could return a custom Notification object (or list thereof) that indicates failures with severity and message strings.

    Notification AddChild(Reference r, Parent p, out Child c)
    

    Lots of information, hooray! But I still need an output parameter. D'oh.

  • I could return Notification, but construct the Child before passing it into AddChild, and expose the Child.ID property as a public settable so the SetOfThings can change it appropriately.

    var newChild = new Child(someRef, someParent);
    var notification = set.AddChild(newChild);
    if (/* notification is failure */)
        // change state, discard newChild, display message
    else
        // change state, use newChild, etc.
    

    But the public settable Child.ID property to make this work is confusing at best, and dangerous at worst.

Am I missing some obvious or common alternative? I'm not breaking new ground here, but I can't come up with a clear, readable way for a collection class to have creation and validation in the same method.

  • 2
    If your "normal control flow" for when some arguments are invalid is to show the user a message box with an error message that tells them what they did wrong - that's a perfectly acceptable use for the exception mechanism in my book – Idan Arye Nov 17 '15 at 0:02
3

The alternative that immediately comes to my mind is to simply use more than one method.

Child newChild(someReference, someParent);
if(HasChild(newChild)) {
    // UI.Alert and whatnot
} else {
    AddChild(someReference, someParent);
}

Now you aren't using exceptions for control flow, and if the AddChild method throws, that means something actually has gone quite horribly wrong.

Notice that the example I just gave does not check that someReference and someParent already exist in SetOfThings. That's deliberate. Maybe I already know for certain that they exist and it's only duplicate children I'm worried about. Maybe in some other code snippet it'll be the other way around and over there you'll write this instead:

if(!HasReference(someReference) || !HasParent(someParent)) {
    // UI.Alert and whatnot
} else {
    Child newChild = AddChild(someReference, someParent);
}

In other words, having dedicated methods (as opposed to exceptions) for your validation checks lets you be much more explicit about what checks you believe are necessary. If an exception gets thrown from either of the above snippets, then you know that you've made a mistake somewhere.

  • This is interesting. It essentially transfers the responsibility of validation to the AddChild callers, rather than the class that has the method. Right? – kdbanman Nov 17 '15 at 0:07
  • Actually, I suppose transferring responsibility isn't necessary. The SetOfThings could still validate using the exception mechanism or notification mechanism. – kdbanman Nov 17 '15 at 0:08
  • 2
    @kdbanman It definitely allows (but does not require) the AddChild method to transfer that responsibility, which might be useful. In the C++ code I'm used to, it's particularly common to state that an invalid AddChild call is undefined behavior, then give AddChild some assertions on its arguments that are enabled only in debug mode. This way you get the extra checks in development without any performance loss in production. Whether that sort of thing makes sense in C# I'm not sure, but separate validation methods seems viable in any language. – Ixrec Nov 17 '15 at 0:11
  • I think, for simple business logic validation we should not throw exceptions (because it is not an exceptional case). Rather use different validation methods which return bool result and validate the business logic before actually working with it. It also help you to create different small methods for validating and doing the actual work. It is better in comparison with doing validation and doing the actual working in same method. – DeveloperArnab Nov 17 '15 at 3:07
  • @DeveloperArnab, You suggest "use different validation methods which return bool result and validate the business logic before actually working with it." This is in agreement with Ixrec's answer, right? – kdbanman Nov 17 '15 at 16:56
2

There's nothing wrong in using exceptions for that. Even though it might look like a common scenario it must not be as common as not having any validation errors.

Even if the validation fails more often than not, exceptions are still a desirable solution for your problem. If you're worried about performance just don't as every developer knows that premature optimization is the root of all evil.

There are several ways to improve on it though:

  • Split your method in two, one to validate another to actually do the operation which then moves the responsibility to the caller
  • Have more specific exceptions (example: InputValidationException, FormatException) so it allows the caller to catch only relevant errors
  • Throw an exception that contains more than just the error message
  • Collect all validation errors and throw them as a list inside a ValidationException object allowing the caller to display multiple errors at a time

Based on my experience not using exceptions for validation errors will more likely turn your code into spaghetti in the long run. Specially if you have to handle validation errors that are being generated several levels down the call stack.

Exceptions is the best language tool that allows you to pass information across the entire stack without turning your API design in a huge mess.

  • Sound words. I'm not actually worried about performance in this situation - it's more the cognitive overhead of exceptions for devs that I'm worried about. But more than one person has suggested it's actually the appropriate mechanism here. – kdbanman Nov 17 '15 at 5:01
  • Can you explain your third bullet, "Throw an exception that contains more than just the error message," please? I don't understand what you mean. – kdbanman Nov 17 '15 at 16:13
  • @kdbanman there are situation where only a message is not enough, you want to include an error code or for example a DB error message which will help whoever needs to debug this issue. When that's the case you can't simply return a bool value you need multiple values, potentially a class containing a message, error code, username, sql info and so on. Exceptions are the perfect tool in that situation. – Alex Nov 20 '15 at 14:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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