C# class method. How to fail and return why?

Question

I am sure there is a "good" way of solving this, but it's something that has always bugged me. I have a method that is supposed to return an object, but has certain preconditions for its parameters. These are outside my control and can fail for "business logic" reasons (if you'll forgive the stale term).

The return value of the object would be null but I'd also like to pass back why, so the calling code can essentially say "I didn't get my object back because there wasn't enough information to build it".

I don't feel like try catch is the right approach, but have been using it in this situation for want of a better approach. All my reading here on stackoverflow and textbooks and MSDN seem to focus either on when or how to use exceptions, but I am somehow failing to come up with an approach for this case.

Can anyone suggest a pattern that would be more appropriate? (first post so... pls forgive any faux pas)

Here's a sample I have been playing with by way of example: (note the throw new Exception line below the // TODO comment)

public static Packet Parse(string packetString)
{
    Packet returnPacket = new Packet();
    StringBuilder output = new StringBuilder();

    try
    {
        using (XmlReader reader = XmlReader.Create(new StringReader(packetString)))
        {
            XmlWriterSettings ws = new XmlWriterSettings();
            ws.Indent = true;
            using (XmlWriter writer = XmlWriter.Create(output, ws))
            {
                string rootNodeString = string.Empty;

                // Parse the packet string and capture each of the nodes.
                while (reader.Read())
                {
                    //test root node is the correct opening node name
                    if (rootNodeString == string.Empty)
                    {
                        if (reader.NodeType != XmlNodeType.Element || reader.Name != PACKETROOT)
                        {
                            // TODO: I don't really think this should be an exception, but going with it for now for expediency, since XmlReader is doing the same anyway
                            throw new Exception(string.Format("The root node of a Packet must be <{0}>", PACKETROOT));
                        }
                        else
                        {
                            rootNodeString = reader.Name;
                        }
                    }

                    switch (reader.NodeType)
                    {
                        case XmlNodeType.Element:
                            Console.WriteLine(string.Format("start element = {0}", reader.Name));
                            break;
                        case XmlNodeType.Text:
                            Console.WriteLine(string.Format("text = {0}", reader.Value));
                            break;
                        case XmlNodeType.XmlDeclaration:
                        case XmlNodeType.ProcessingInstruction:
                            Console.WriteLine(string.Format("XmlDeclaration/ProcessingInstruction = {0},{1}", reader.Name, reader.Value));
                            break;
                        case XmlNodeType.Comment:
                            Console.WriteLine(string.Format("comment = {0}", reader.Value));
                            break;
                        case XmlNodeType.EndElement:
                            Console.WriteLine(string.Format("end element = {0}", reader.Name));
                            break;
                    }
                }

            }
        }

    }
    catch (XmlException xem)
    {
        Console.WriteLine(xem.Message);
        throw;
    }

    return returnPacket;
}

Answer 1

I have a method that is supposed to return an object, but has certain preconditions for its parameters. These are outside my control and can fail for "business logic" reasons

If the precondition is truly a precondition then a caller who fails to honour the precondition has a bug. The right thing to do is for the callee to throw an exception, ideally one that aggressively crashes the entire process. That will strongly encourage the caller to fix their bug.

But based on your description, it sounds like what you have is not actually a precondition; preconditions are the things that must be true and if they are not always true then the caller is buggy. It sounds like instead that what you have is a method that is doing too much; it is both validating that the arguments supplied are classified as valid by certain business policies, and computing a result if the arguments are valid.

I would therefore be inclined to split this up into two methods.

The first method analyzes its arguments to see if the business policy classifies them as valid, and returns a reporting object that describes in detail why the arguments are valid or invalid.

The second method computes a result. The second method can have as its precondition that the first method would have validated the arguments.

The return value of the object would be null but I'd also like to pass back why, so the calling code can essentially say "I didn't get my object back because there wasn't enough information to build it".

Again, this is more evidence that you are trying to do too much. You want the returned value of the method to be both an analysis describing why a business rule was not met, and the result of a business process. Those are two very different things, and therefore should be computed by two different methods.

Answer 2

The approach I use in Noda Time is to have a ParseResult<T> type, which is the result of a parse operation. It knows whether or not it succeeded, and if you ask a parse failure for the result it will throw an exception, but it doesn't throw an exception otherwise. (You can't currently get the exception which would be thrown without it being thrown, but I may add that later.)

This is nicer than throwing an exception because failure is expected here - it's not really exceptional.

It's nicer than using the pattern of int.TryParse etc because it gives you a single result value encapsulating everything about the parse operation - no more messing with out parameters, and much more detail available in the case of failure.

Now it's not clear to me whether this is appropriate in your particular case - but it is appropriate (IMO) when you're basically dealing with data which may well be invalid in a way which doesn't indicate anything being wrong with any part of your system: there's nothing surprising or exceptional going on here, just someone providing you with bad input :(