How to handle "not all code paths return a value" when the logic of the function does ensure a return

Question

I suppose the easiest way to explain is by a contrived example:

public static int Fail() {
    var b = true;
    if (b) {
        return 0;
    }
}

This code will not compile, and gives the error "not all code paths return a value," while we humans can clearly see that it does. I DO understand why. My question is what should be done to remedy the situation. It could be something like this:

public static int Fail() {
    var b = true;
    if (b) {
        return 0;
    }
    throw new ApplicationException("This code is unreachable... but here we are.");
}

But it all just seems rather silly. Is there a better way? Again, this code is a contrived example (and can be reduced to return 0). My actual code is massive and complex, but does logically (by mathematical proof) return a value before trying to exit.

Constantino

Note that if you declare b as a const local variable (const bool b = true;), it compiles just fine – Hydrus

If your method is so "massive and complex" you should probably refactor it. What hope would the next human programmer have if a compiler gives up on it? – Orsola

github.com/coder0xff/parlex/blob/master/IDE/Nfa.cs based on the Kameda-Weiner algorithm in "On the State Minimization of Nondeterministic Finite Automata". This stuff is not for human programmers. – Constantino

Answer 1

C# code flow analysis is limited and as your sample points out there are cases where all paths do return but the compiler can't detect it. Throwing an exception is an acceptable remedy in those circumstances.

I would not use a return of a default value to fix this error. You're operating under the assumption that the line is never hit against the advice of the compiler. Consider for a second that your analysis is wrong and execution can proceed to the end of the method. If you return a default value then you will have no indication of the problem. The method will just be returning bad data. Throwing an exception will make it very obvious that there is a problem.

However in those cases my preference is to simply rewrite the code such that the compiler can see that all paths terminate. In general I've found that if the method is so complex that the compiler can't follow it then the guy who picks up the code after me won't be able o follow it either.

Publican

Refactoring is my first instinct as well. Unfortunately, in this case (NFA minimization), the problem is intractable (in the mathematical sense) and this can't really be done. I'm going through a loop, testing some conditions, of which I know at least one will be used as the result. – Constantino

I've decided, after a couple days, that I like your answer best, and switched it to you. I want to ask though, what exception do I throw? I could make a TheAssumptionsMadeAboutThisProgramsLogicAreIncorrectException, but as I said, it just all seems a bit silly. – Constantino

@Constantino personally I would just use System.Exception but add in a very specific message about what went wrong. – Publican

Answer 2

The compiler doesn't perform complex mathematical processes to ensure you return a value, it's a relatively dumb beast in that regard.

If you're certain of your assumptions, just add return 0; at the end with a comment stating the reality, that it will never be reached.

Of course, any code where it can be guaranteed the final condition will be true can just have the condition removed, as you've already stated. That's mathematically true even for complex cases. So you could refactor your code to take that into account without a superfluous return.

Heman

See my comment to JaredPar's answer. However, I think I'll have to agree with you. A return null; //this code is unreachable seems like the best thing. – Constantino

paxdiablo is right, the correct way is what he said. But if it will make you happier ;), you can define minNfa outside foreach, then just break in if, and finally return minNfa after froeach. (Based on your code here: github.com/coder0xff/parlex/blob/master/IDE/Nfa.cs) – Postrider

@Postrider can you clarify how that would be done? As far as I can tell, this is tantamount to deciding the halting problem. Also, based on the 'minNfa' approach you describe, I'd still have to initialize minNfa to null, because the compiler still decides that the foreach may not set its value. – Constantino

@Constantino sure it must be initialized. But it will decrease the readability of the code, IMHO. So, I think the approach is not so good in practice. :) – Postrider

Answer 3

I think your current approach is correct. Returning a default value at the end of the method is risky, because if there's a flow in your logic, this instruction could be reached, which could have unintended consequences. If you throw an exception, it gives you a chance to detect the mistake.

Hydrus

Answer 4

There are usually a couple of classes of programmers:

1. They who return all over.
2. They who return only at the end of a funcion/method.

First approach:

public static int Fail() {
  var b = true;
  if (b)
    return 0;

  return 1;
}

I usually prefer the second approach since it's easier to quickly see where a function/method returns.

public static int Fail() {
  var b = true;
  var returnValue = 1:
  if (b)
    returnValue = 0;

  return returnValue;
}

Strep

Answer 5

Really late answer, but I can imagine a scenario in which the function will not know what to return:

1) Run step by step using the debugger

2) Step over var b = true;

3) When if (b) { is reached, put b = false in the watch. It will both return false and assign b = false

4) Step over (over if)

5) End of function is reached, no return is there

From this perspective, return is clearly needed.

Mcdonald

Answer 6

I had code like this and fixed it this way. Looking at your example:

public static int Fail() {
    var b = true;
    if (b) {
        return 0;
    }
    throw new ApplicationException("This code is unreachable... but here we are.");
}

would become

public static int Fail() {
    var b = true;
    if (!b) {
        throw new ApplicationException("This code is unreachable... but here we are.");
    }
    return 0;
}

This will get rid of the compiler warning, still be logically the same, and actually be slightly cleaner code.

Gribble

As other answers mention, having a "default" value isn't clean. If the logic of the function changes, you may suddenly be returning a magic 0. – Constantino

This is not a "default" value. I took the code snippet from the original example. This code will remove the "not all code paths return a value" error from the IDE, which is the original question. We could say that instead of "return 0;" we instead have code which depends on variable "b" being true. – Gribble