C# not all code paths return a value data collector class project

Question

I am trying to find out where the issue is in this code and I just can't find it. beginning of statement....

    private Units unitsToUse;
    private int[] dataCaptured = new int[30];
    private int mostRecentMeasure;

method at end of statement....

    public int Measure(int v, Units u)
    {
        if (v != 0 && v != null)
        {
            mostRecentMeasure = v;
            if (u == Units.Metric)
            {
                unitsToUse = u;
                for (int i = 0; i < 30; i++)
                {
                    if (dataCaptured[i] != 0 && dataCaptured[i] != null && i < 29)
                    {
                        continue;
                    }
                    if (i == 29 && dataCaptured[i] != null)
                    {
                        i = 0;
                        dataCaptured[i] = mostRecentMeasure;
                        return mostRecentMeasure;
                    }
                    dataCaptured[i] = mostRecentMeasure;
                    return mostRecentMeasure;
                }
            }
            else if (u == Units.Imperial)
            {
                unitsToUse = u;
                for (int i = 0; i < 30; i++)
                {
                    if (dataCaptured[i] != 0 && dataCaptured[i] != null && i < 29)
                    {
                        continue;
                    }
                    if (i == 29 && dataCaptured[i] != null)
                    {
                        i = 0;
                        dataCaptured[i] = mostRecentMeasure;
                        return mostRecentMeasure;
                    }
                    dataCaptured[i] = mostRecentMeasure;
                    return mostRecentMeasure;
                }
            }
            else
            {
                throw new ArgumentOutOfRangeException("Your units were neither of the available values.");
            }
        }
        else
        {
            throw new ArgumentOutOfRangeException("Your value of measuring was not in the specified range.");
        }
    }

Now The method is going to receive a valid Enum value for Units and a valid value from the int V through a random 1-10 method elsewhere in the code. What i don't understand is where the code isn't returning a value or throwing an exception to handle any erroneous cases of the method executing outside of the parameters. I've been stuck here for a while and any help would be appreciated.

You don't return from the two final `else` statements - you simply throw. Maybe you need an actual return? (Not sure on that). — Tim, Apr 27 '16 at 18:24
this means that you need to have a `return value of int` if you do not plan on returning any value change the method signature to `void` you need the return on both the else blocks — MethodMan, Apr 27 '16 at 18:24
@Tim you do not, that is incorrect. Throw is a valid logical path control statement. — David L, Apr 27 '16 at 18:24
Throwing is acceptable. The error message should not be generated if a code path throws an exception. — Eric J., Apr 27 '16 at 18:24
@DavidL - Ok. That's why I said I wasn't sure if that was the case or not :) — Tim, Apr 27 '16 at 18:25
You don't have a return at the very end of the method. Your code can reach that point if you hit the `continue` case in every iteration of either `for` loop. The compiler isn't smart enough to realize that you won't hit that `continue` on the last loop. — juharr, Apr 27 '16 at 18:27
initialize the `mostRecentMeasure = 0;` outside the if and outside the if statement use only one return and inside the for loop add a break; there are a few ways to handle this .. — MethodMan, Apr 27 '16 at 18:28
I do want to return a value, but there is no case for that to logically happen. I'm just covering my bases with exception handling. — Daniel Scuba, Apr 27 '16 at 18:28
Is the compiler smart enough to know that the loops are using static values and will always execute? If not, it may be complaining about not returning a value after 0 loop iterations. In any event, this method is a *prime* candidate for extracting smaller methods. One of those should tell you more specifically where the problem is. — David, Apr 27 '16 at 18:28
refactor your code it can be done much cleaner and you will have less confusion — MethodMan, Apr 27 '16 at 18:29
@MethodMan So more like ` if (dataCaptured[i] != 0 && dataCaptured[i] != null && i < 29) { continue; } if (i == 29 && dataCaptured[i] != null) { i = 0; break; } dataCaptured[i] = mostRecentMeasure; return mostRecentMeasure; }` — Daniel Scuba, Apr 27 '16 at 18:33
you have an acceptable answer on how you could or should refactor the code.. if you want to do something different I would suggest that you logically think about your own process and do not use the `stream of conscious coding approaching` meaning coding as you go without trying to streamline the excessive code. — MethodMan, Apr 27 '16 at 18:36

score 2 · Accepted Answer · answered Apr 27 '16 at 18:27

First off, consider restructuring your logical control statements to prevent less nesting. That makes it easier to detect paths that do not return.

Second, you are checking for null on value types, this is incorrect and the evaluation will never be true, since value types cannot be null.

Third, you should break out of loops when conditions are met and return from outside of the loop instead of trying to return from inside the loop.

public int Measure(int v, Units u)
{
    if (v == 0)
        throw new ArgumentOutOfRangeException(
            "Your value of measuring was not in the specified range.");

    mostRecentMeasure = v;
    if (u == Units.Metric)
    {
        unitsToUse = u;
        for (int i = 0; i < 30; i++)
        {
            if (dataCaptured[i] != 0 && i < 29)
            {
                continue;
            }
            if (i == 29)
            {
                i = 0;
                dataCaptured[i] = mostRecentMeasure;
                return mostRecentMeasure;
            }
            dataCaptured[i] = mostRecentMeasure;
            break;
        }

        return mostRecentMeasure;
    }
    else if (u == Units.Imperial)
    {
        unitsToUse = u;
        for (int i = 0; i < 30; i++)
        {
            if (dataCaptured[i] != 0 && i < 29)
            {
                continue;
            }
            if (i == 29)
            {
                i = 0;
                dataCaptured[i] = mostRecentMeasure;
                break;
            }
            dataCaptured[i] = mostRecentMeasure;
            break;
        }

        return mostRecentMeasure;
    }
    else
    {
        throw new ArgumentOutOfRangeException(
            "Your units were neither of the available values.");
    }
}

thank you. I know I was overcomplicating it, but it's been a rough week and you really helped me. I was stuck on this for a day straight and after a while lines begin to blur. I sincerely appreciate the help, both in my critical thinking and my code. — Daniel Scuba, Apr 27 '16 at 19:16
@DanielScuba happy to help! Sometimes it is useful to take a step back and break the code down. If my answer helped you, please consider marking it as the accepted answer in order to help others who might be faced with a similar issue. — David L, Apr 27 '16 at 20:41

score 0 · Answer 2 · answered Apr 27 '16 at 18:37

To Build On @DavidL's answer

Code to have the least nesting possible. That makes it easier.
You don't need to check for null on non nullable types (int v)
you should break out of loops when conditions are met and return from outside of the loop instead of trying to return from inside the loop.
Don't repeat the same code for your if statement, they can be combined
No need to explicitly set i = 0 before returning

There are still some more issues with your code:

// How Can i ever be greater than 29 but also less than 30?
for (int i = 0; i < 30; i++)
{
    if (dataCaptured[i] != 0 && i < 29)
    {
        continue;
    }
}



    public int Measure(int v, Units u)
    {
        if (v == 0)
        {
            throw new ArgumentOutOfRangeException("Your value of measuring was not in the specified range.");
        }

        if (false == (u == Units.Metric || u == Units.Imperial))
        {
            throw new ArgumentOutOfRangeException("Proper unit type not provided.");
        }
        mostRecentMeasure = v;
        if (u == Units.Metric || u == Units.Imperial)
        {
            unitsToUse = u;
            for (int i = 0; i < 30; i++)
            {
                if (dataCaptured[i] != 0 && i < 29)
                {
                    continue;
                }
                if (i == 29)
                {
                    dataCaptured[i] = mostRecentMeasure;
                    return mostRecentMeasure;
                }
                dataCaptured[i] = mostRecentMeasure;
                break;
            }

            return mostRecentMeasure;
        }
    }

C# not all code paths return a value data collector class project

2 Answers2