CSharp: Not all code paths return a value.

Question

I am writing a simple WinForms application in which I allow the user to drag around TreeNodes in a TreeView control. One of the rules that I enforce is that the user is not allowed to drag a TreeNode into one of its own children. I wrote the following function in a recursive style to check for the parenthood of the destination node. Upon compilation, I get the error that not all code paths return a value for this function. As far as I can tell, I do have a return statement on every possible branch of this logic... but I'm obviously wrong. Can someone point out my error, please.

    private bool IsDestinationNodeAChildOfDraggingNode(TreeNode draggingNode, TreeNode destinationNode) {
        if (draggingNode.Nodes.Count == 0) 
            return false;
        else {
            if (draggingNode.Nodes.Contains(destinationNode)) 
                return true;
            else {
                foreach (TreeNode node in draggingNode.Nodes) 
                    return IsDestinationNodeAChildOfDraggingNode(node, destinationNode);
            }
        }
    }

Answer 1

It's probably referring to the fact that if draggingNode.Nodes has no items in it then it will fall out of the else and exit the function without returning anything.

Maybe do this:

foreach (TreeNode node in draggingNode.Nodes) 
     return IsDestinationNodeAChildOfDraggingNode(node, destinationNode);

return false

Answer 2

My expectation is that the foreach loop is not guaranteed to return a value, if all elements in the foreach fail to return anything, or if there are no nodes. After the final if/else/foreach, add a default return value.

Answer 3

I'm guessing that the C# compiler assumes that the foreach loop terminates normally, as opposed to returning out across about three levels of control.

Answer 4

The compiler thinks you need to add a return after the foreach loop as it could possibly be that there are no nodes in draggingNode.Nodes so the loop wouldn't actually execute.

private bool IsDestinationNodeAChildOfDraggingNode(TreeNode draggingNode, TreeNode destinationNode) {
    if (draggingNode.Nodes.Count == 0) 
        return false;
    else {
        if (draggingNode.Nodes.Contains(destinationNode)) 
            return true;
        else {
            foreach (TreeNode node in draggingNode.Nodes) 
            {
                return IsDestinationNodeAChildOfDraggingNode(node, destinationNode);
            }
            return false; // <-- here for example
        }
    }
}

Now this might not be the correct place for your algorithm, but the compiler thinks that there is a return missing here. Reworking your code along the lines Jherico suggests will make the situation clearer.

Answer 5

What if there are no items in draggingNodes.Nodes? Then you will not return a value.

Answer 6

Outside of the outer Else you must return something.

For instance:

private bool IsDestinationNodeAChildOfDraggingNode(TreeNode draggingNode, TreeNode destinationNode) {
if (draggingNode.Nodes.Count == 0) 
        return false;
else {
       if (draggingNode.Nodes.Contains(destinationNode)) 
            return true;
       else {
            foreach (TreeNode node in draggingNode.Nodes) 
            return IsDestinationNodeAChildOfDraggingNode(node, destinationNode);
            }
    }
//Return something here.
}

Answer 7

Actually I think your algorithm is wrong. Why would you bother with a foreach statement if you're only going to check the first value? Also, all the else's after the returns are kind of redundant and make the code harder to follow.

I suspect what you're trying to do is something more like this:

private bool IsDestinationNodeAChildOfDraggingNode(TreeNode draggingNode, TreeNode destinationNode) {
  // special case, no children
  if (draggingNode.Nodes.Count == 0) 
    return false;

  // check if the target is one of my children
  if (draggingNode.Nodes.Contains(destinationNode)) 
     return true;

  // recursively check each of my children to see if one of their descendants is the target
  foreach (TreeNode node in draggingNode.Nodes) 
    if (IsDestinationNodeAChildOfDraggingNode(node, destinationNode)) 
        return true;

  // didn't find anything
  return false;
}

Some people will insist that early return is evil which would result in this version of the function:

private bool IsDestinationNodeAChildOfDraggingNode(TreeNode draggingNode, TreeNode destinationNode) {
  bool retVal = false;

  if (draggingNode.Nodes.Count != 0) {

    // check if the target is one of my children
    if (draggingNode.Nodes.Contains(destinationNode)) {
      retVal = true;
    } else {

      // recursively check each of my children to see if one of their descendants is the target
      foreach (TreeNode node in draggingNode.Nodes) 
        if (IsDestinationNodeAChildOfDraggingNode(node, destinationNode)) {
          retVal = true;
          break;
        }
    }
  }

  return retVal;
}