HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Return value when the requested variable returns null

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thereturnnullvaluereturnswhenvariablerequested

Problem

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

private List GetRelevantNodes(HtmlDocument aHtmlDoc, string aID)
{
    List nodes = new List();

    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.

Solution

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

private List GetNodes(HtmlDocument aHtmlDoc, string aID)
{
    List nodes = new List();

    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 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 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 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.

Code Snippets

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;
}
List<HtmlNode> nodes = GetNodes(htmlDoc, id);
if (nodes != null)
{
    foreach (HtmlNode node in nodes)
    {
        // Super secret business code
    }
}
foreach (HtmlNode node in GetNodes(htmlDoc, id))
{
    // Still secret...
}
List<HtmlNode> nodes = GetNodes(htmlDoc, id);
if (nodes.Length > 0)
{
    foreach (HtmlNode node in nodes)
    {
        // Super secret business code
    }
}
private List<HtmlNodes> GetNodes(HtmlDocument aHtmlDoc, string aID)

Context

StackExchange Code Review Q#41867, answer score: 8

Revisions (0)

No revisions yet.