C# error - "Not all code paths return a value"

Question

This is fairly simple method. I use entity framework to get some data and then check some values in a if statement. However right now the method is marked with red.

This is my method:

private bool IsSoleInProduction(long? shoeLastID)
{
    if (shoeLastID == null)
    {
        MessageBox.Show(Resources.ERROR_SAVE, 
                        "Error", 
                        MessageBoxButtons.OK, 
                        MessageBoxIcon.Error);
        return false;
    }

    ISoleService soleService = 
        UnityDependencyResolver.Instance.GetService<ISoleService>();

    List<Sole> entity = 
        soleService.All().Where(s => s.ShoeLastID == shoeLastID).ToList();

    if (entity.Count() != 0)
    {
        foreach (var items in entity)
        {
            if (items.Status == 20)
            {
                return true;
            }
            else
            {
                return false;
            }
        }
    }
    else
    {
        return false;
    }    
}

What am I missing?

Did you modify the code after the answers? The above code should work. — Emil Lundin, Apr 09 '13 at 07:06
wouldn't the if else status check only ever check the first element of the list? I'd think you just need the one return false afterwards — Sayse, Apr 09 '13 at 07:10

cuongle · Accepted Answer · 2013-04-09T07:29:43.517

7

You need to take advantage of LINQ with Any, replace your code:

if (entity.Count() != 0)
{
    foreach (var items in entity)
    {
        if (items.Status == 20)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
}
else
{
    return false;
}

with simpler code:

 return entity.Any(item => item.Status == 20);

Or even better performance:

 return soleService.All()
              .Any(s => s.ShoeLastID == shoeLastID
                     && s.Status == 20);

Edit: With you comment, below code is what you need:

  List<Sole> entity =  soleService.All()
                            .FirstOrDefault(s => s.ShoeLastID == shoeLastID);

  return entity == null ? false : entity.Status == 20;

edited Apr 09 '13 at 07:29

answered Apr 09 '13 at 07:09

cuongle

74,024
28
151
206

1

The version using linq is not equivalent to the original, which may return false if there are items with Status != 20 at the beginning of the list. – josef.axa Apr 09 '13 at 07:16
@josef.axa: No, it returns if one item which has Status == 20, if not, will search until found. You need to run this code to see – cuongle Apr 09 '13 at 07:19
@josef.axa That is a good abservation. The equivalent version would be something like: return (soleService.All().First(s => s.ShoeLastID == shoeLastID).Status == 20) – Emil Lundin Apr 09 '13 at 07:21
@josef.axa: I have included the link on this answer – cuongle Apr 09 '13 at 07:22
1

What I meant is the linq version will return true if the list contains any element with status == 20, while the original will only return true if the first item has status == 20. – josef.axa Apr 09 '13 at 07:24
I suspect the linq version is what the author wanted though, I just wanted to point out that there was a difference. – josef.axa Apr 09 '13 at 07:25

score 3 · Answer 2 · answered Apr 09 '13 at 07:01

3

If there's no item in your entity collection, then neither of the containing if/else branches will be executed. In this case there's no return statement anymore, because the else part won't be executed, and outside your foreach you have no return statement.

answered Apr 09 '13 at 07:01

JustAnotherUserYouMayKnow

2,554
16
14

score 3 · Answer 3 · answered Apr 09 '13 at 07:15

The compiler does not "see" that if

entity.Count() != 0

then your loop

foreach (var items in entity)

will run at least once. Therefore it sees a possibility of running the forech zero times, and not running the else block.

Suppose first time the entity is enumerated, it yields some (finite number of) items. Then the Count will be non-zero. Then suppose next time the same entity is enumerated then it yields no items! That would cause your code to "fall through" without returning.

It is very probably that you can guarantee that the source yields the same number of items each time it is re-enumerated. But the compiler cannot.

Solution: Just skip if (entity.Count() != 0) and do foreach right away.

score 2 · Answer 4 · answered Apr 09 '13 at 07:02

You haven't retrun anything from this code block

if (entity.Count() != 0)
            {
                foreach (var items in entity)
                {
                    if (items.Status == 20)
                    {
                        return true;
                    }
                    else
                    {
                        return false;
                    }
                }
                // return someting
            }

Soner Gönül · Answer 5 · 2013-04-09T07:15:47.207

What will be your entity.Count() is not 0 and your entity doesn't have any items?

That means your if block will work but foreach part will not work. Since your if part doesn't have any return statement, that's why you get an error.

You should put return statement in your if part.

if (entity.Count() != 0)
{
    foreach (var items in entity)
    {
        if (items.Status == 20)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
    //return true or false
}

score 1 · Answer 6 · edited May 23 '17 at 11:49

You might consider doing the following. This will adhere to the "single exit-point" principle (which can sometimes help improve code clarity), and ensure you have a default value in any case:

private bool IsSoleInProduction(long? shoeLastID)
{
    // The main change: A default value, assuming "no":
    var isSoleInProduction = false; 

    if (shoeLastID == null)
    {
        MessageBox.Show(Resources.ERROR_SAVE, 
                        "Error", 
                        MessageBoxButtons.OK, 
                        MessageBoxIcon.Error);
        isSoleInProduction = false;
    }

    ISoleService soleService = 
        UnityDependencyResolver.Instance.GetService<ISoleService>();

    List<Sole> entity = 
        soleService.All().Where(s => s.ShoeLastID == shoeLastID).ToList();

    if (entity.Count() != 0)
    {
        foreach (var items in entity)
        {
            if (items.Status == 20)
            {
                isSoleInProduction = true;
            }
            else
            {
                isSoleInProduction = false;
            }
        }
    }
    else
    {
        isSoleInProduction = false;
    }    

    return isSoleInProduction;
}

score 0 · Answer 7 · answered Apr 09 '13 at 07:04

0

If your entity collection has no elements you will not reach a return statement - you need to add a return false forexample as last statement

answered Apr 09 '13 at 07:04

Rune Andersen

1,650
12
15

score 0 · Answer 8 · answered Apr 09 '13 at 07:05

As the error states there can be cases in which none of your return clause is evaluated (e.g. if there are no elements in your list). To quickly solve it you can put a default return statement, for example by moving the last return clause outside the else statement. But it really depends on the behavior you'd expect.

 private bool IsSoleInProduction(long? shoeLastID)
        {
            if (shoeLastID == null)
            {
                MessageBox.Show(Resources.ERROR_SAVE, "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                return false;
            }
            ISoleService soleService = UnityDependencyResolver.Instance.GetService<ISoleService>();
            List<Sole> entity = soleService.All().Where(s => s.ShoeLastID == shoeLastID).ToList();

            if (entity.Count() != 0)
            {
                foreach (var items in entity)
                {
                    if (items.Status == 20)
                    {
                        return true;
                    }
                    else
                    {
                        return false;
                    }
                }
            }
            return false;
        }

Matthew Strawbridge · Answer 9 · 2013-04-09T07:19:50.833

0

The compiler can't guarantee that the first call to Count means that the foreach will loop at least once (with good reason, because you could, if you wished, create a collection where this wasn't true). You can do this instead (with no need for the outer if):

foreach (var items in entity)
{
    if (items.Status == 20)
    {
        return true;
    }
    else
    {
        return false;
    }
}

return false;

edited Apr 09 '13 at 07:19

answered Apr 09 '13 at 07:07

Matthew Strawbridge

19,940
10
72
93

C# error - "Not all code paths return a value"

9 Answers9