C#: 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);
            }
        }
    }

everyone is correcting the compiler error without noting the algorithm only explores one leg of the graph. This is almost certainly wrong. — Jherico, Oct 27 '09 at 23:39

score 12 · Answer 1 · answered Oct 27 '09 at 23:23

12

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

answered Oct 27 '09 at 23:23

Karim

18,347
13
61
70

Well, if draggingNode.Nodes has no items then its Count property would equal zero, in which case it returns false. Is that not correct? – λ Jonas Gorauskas Oct 27 '09 at 23:26
3

The code checker doesn't know that... you are giving it credit for understanding algorithms. It only understands code paths (one of which was left out as pointed out by Karim). – Godeke Oct 27 '09 at 23:28
1

Might add something about why this is because one would think the count check in the beginning should suffice... which in a threaded and/or event driven environment can't be guaranteed to be true (though if this is what's actually in the compiler's mind or if it's just picky/simple-minded I have no idea?) – Oskar Duveborn Oct 27 '09 at 23:29
This solution doesn't actually work, as it returns a false if the destination is more than 2 levels deep ... – λ Jonas Gorauskas Oct 27 '09 at 23:41
1

@Jonas, incorrect, it returns false if the destination is not a node that is a sibling to a node on the left hand side of the graph – Jherico Oct 27 '09 at 23:44
"Well, if draggingNode.Nodes has no items then its Count property would equal zero [...]" True - IF (!) the Count property does anything like you would expect it to..... So, in this case conservative checking has its reasons. – danielschemmel Oct 28 '09 at 00:31
@Jherico - Yes, i realize that now.. Thanks! @Karim - I am accepting the answer by Jherico because he addresses the compiler error as well as the flaw in my algorithm. – λ Jonas Gorauskas Oct 28 '09 at 01:31

Jherico · Accepted Answer · 2009-10-27T23:42:57.373

8

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;
}

edited Oct 27 '09 at 23:42

answered Oct 27 '09 at 23:36

Jherico

28,584
8
61
87

1

My logic is as follows: 1. If draggingNode has no nodes then destinationNode cannot be one of its children. 2. Otherwise check if the immediate children of draggingNode contains the destinationNode. 3. If not, then start descending into the draggingNode's grandchildren, until you run out of possibilities. BTW, your alternative where you create *bool retVal = false;* works and created the desired effect. I thought that the *early returns* version was cleaner. Why do you say that *early returns are evil*? – λ Jonas Gorauskas Oct 27 '09 at 23:53
2

Step 1 and 2 are fine, but step 3 is broken, because the code will always return true or false based on the children of the first child node, not *all* the grandchildren. If your second child's first child is the destination node, your code will still return false. Also, *I* didn't say early returns are evil. I said some people might say that. I've been places where the coding guidelines prohibited them. I don't know the justification off the top of my head. – Jherico Oct 27 '09 at 23:57
1

re: early return, its the subject of this question: http://stackoverflow.com/questions/36707 – Jherico Oct 28 '09 at 00:12
Whenever an early return equals simpler, easier to follow code, I'll gladly have spaghetti for dinner. – snarf Oct 28 '09 at 00:26
1

@Jherico - Thanks for the pointer to the question on Early Return... Interesting commentary going on there. I am accepting this answer by Jherico because he addresses the compiler error as well as the flaw in my algorithm. – λ Jonas Gorauskas Oct 28 '09 at 01:34

ChrisF · Answer 3 · 2009-10-27T23:47:59.230

2

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.

edited Oct 27 '09 at 23:47

answered Oct 27 '09 at 23:24

ChrisF

134,786
31
255
325

I am actually stablishing if draggingNode.Nodes has any nodes when I check *if (draggingNode.Nodes.Count == 0)* ... so when I get to the foreach loop I already know that I have nodes to loop through... Is that not correct? – λ Jonas Gorauskas Oct 27 '09 at 23:35
@Jonas - I see your point, but adding the return at the point indicated stops the compiler complaining. – ChrisF Oct 27 '09 at 23:40
Adding the *return false* statement where you propose doesn't work... It returns false when the destination node is more than 2 levels deep... – λ Jonas Gorauskas Oct 27 '09 at 23:43

score 1 · Answer 4 · answered Oct 27 '09 at 23:25

1

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

answered Oct 27 '09 at 23:25

Ed S.

122,712
22
185
265

score 0 · Answer 5 · answered Oct 27 '09 at 23:23

0

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.

answered Oct 27 '09 at 23:23

JYelton

35,664
27
132
191

score 0 · Answer 6 · answered Oct 27 '09 at 23:24

0

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.

answered Oct 27 '09 at 23:24

John R. Strohm

7,547
2
28
33

Karim's answer is much better than mine. – John R. Strohm Oct 27 '09 at 23:27

score 0 · Answer 7 · answered Oct 27 '09 at 23:25

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

Incorrect: If the problem with the `foreach` loop is addressed, both branches of the inner if/else will return values and the compiler will be happy. — Bevan, Oct 27 '09 at 23:50

C#: Not all code paths return a value

7 Answers7