Return value when the requested variable returns null

Question

I have a method that returns a list from a HTML page:

private List<HtmlNodes> GetRelevantNodes(HtmlDocument aHtmlDoc, string aID)
{
    List<HtmlNode> nodes = new List<HtmlNode>();

    HtmlNode currentDiv = aHtmlDoc.GetElementbyId(aID);
    if (currentDiv == null)
            return null;
    else
            //Do stuff - fill the nodes list...

    return nodes;
}

I'm not sure what to do with the return value in case the currentDiv variable is null. Obviously I can't continue the method regularly and I need to return from this method with some indication that there's no such div.

If I return null and handle it up the stack it'll work fine but it's less readable code (at list in my opinion... don't you think?). Another thing I can add is an out string with the error message and maybe use it later for the end user output but that still doesn't solve my readable problem.

I'd be happy to hear your ideas and advice.

Answer 1

You have to decide if the caller of your method needs to know if the parent was not found or if there is no difference when the parent was not found or when the parent element is simply empty.

If the second holds true, then the other answer already describes the correct approach - return an empty list.

However, if the caller needs to be informed that the parent element (currentDiv) does not exist:

1. The best and most common one would be to throw an exception saying The HTML node with ID {id} does not exist..
2. If the exception should only be thrown in certain cases, your method could have a signature like GetChildren(HtmlDocument doc, string id, bool throwIfNotExists = false). Now the caller can decide himself if the exception should be thrown. If not, then return an empty list (however, not null).

Please note that the following two options are there just so that the answer is more complete, I personally would recommend option #2 from above.

• This option is more suitable for things like web services where exceptions might not be welcome - create a new type that your method returns: class GetChildrenResult { public bool ParentFound; public List<HtmlNode> Children; } (though use properties).
• (Not recommended) You could use an out parameter. This is generally not a good idea but if you decide to do so, you should provide two overloads - one with the argument and second one without.

Answer 2

You're right, this is not the best thing to do. The correct way would be to return an empty list.

private List<HtmlNodes> GetNodes(HtmlDocument aHtmlDoc, string aID)
{
    List<HtmlNode> nodes = new List<HtmlNode>();

    HtmlNode currentDiv = aHtmlDoc.GetElementbyId(aID);
    if (currentDiv != null)
    {
            //Do stuff - fill the nodes list...
    }

    return nodes;
}

Now why is that better than returning null? Simple, it allows to remove checking from the client code. Imagine your function would return null if there are no nodes, clients always need to write the following code when interacting with it:

List<HtmlNode> nodes = GetNodes(htmlDoc, id);
if (nodes != null)
{
    foreach (HtmlNode node in nodes)
    {
        // Super secret business code
    }
}

Always. Now if you'd return an empty list, the checking becomes optional on the client side:

foreach (HtmlNode node in GetNodes(htmlDoc, id))
{
    // Still secret...
}

Or with checking:

List<HtmlNode> nodes = GetNodes(htmlDoc, id);
if (nodes.Length > 0)
{
    foreach (HtmlNode node in nodes)
    {
        // Super secret business code
    }
}

---

You should always write XML Documentation.

---

private List<HtmlNodes> GetNodes(HtmlDocument aHtmlDoc, string aID)

There's a typo, it should be HtmlNode.

Also your variables are named not optimal. Drop that a prefix, nobody cares if it is an argument or not, it's a variable you have to work with.

The name of the function is also not good, it's called GetNodes yet from what I see (and interpret into it) it does not fetch the nodes with the given ID, it fetches the children of the first node with the given ID. So it should actually be called GetChildren.

Answer 3

I agree with Knais that the key question is whether you really need to indicate failure. If you do, then either a tuple or an out param in the form of a TryGetChildren function would be appropriate. I disagree with him in that I don't think that using an out param is a bad idea, at least not if done in the TryXxx pattern.

I would NOT suggest using an out param for error message, as that is a bad idea - if you need more than the bool returned by the TryXxx pattern, then you should be using an object that is acted on, not a variable that is changed. It's just as easy to do the simple stuff, and it makes it possible to do more complex scenarios if you need to. And you should be doing it as a separate validation check, not an exception.

But in 9 out of 10 cases returning a valid but empty collection, is the right thing to do, as you don't really care why you're not finding what you are looking for, you just want something to use in a for each loop.