Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
    
No, you don't want to expand your question. We're here to do code reviews, not answer questions. Your question is already borderline because of the wording (hence you already have 3 close-votes on it). –  Bobby Feb 17 at 11:24
    
The question I added was in the context of this code review, if you wanted me to change the format so it would be a code review and not a question you could have just comment me... I still would be happy to hear suggestions of how to handle a case of return type can't be initialized, like @Knagis exception suggestion. –  Moshisho Feb 17 at 12:34

3 Answers 3

up vote 6 down vote accepted

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.
share|improve this answer

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.

share|improve this answer
    
Thanks for the answer, right after I posted the question I thought of the same solution and now I'm embarrassed in my question... but your answer has given me the certificate it's the correct way to go. About the 'a' prefix I have to disagree since to me it's really readable that it's 'a' general var and not THE var or the 'current' var. And for the documentation - I would start implementing right away! –  Moshisho Feb 17 at 10:15
    
I agree with this advice for handling a non-existent aID. Another justification: that's how jQuery would behave, and jQuery is awesome. –  200_success Feb 17 at 10:29
    
@200_success: jQuery is always the answer.. –  Bobby Feb 17 at 10:45

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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