foreach loop stopping after first iteration in

Question

I have a function that accepts a list of objects (in my case, comments on a blog post) and a user. Then the function should iterate over each comment in the list and set the IsCommenter boolean property to true or false depending on if the comments author id is equal to that of the user which was passed in. The concept being the picture below:

If I'm logged in as Taylor Swift, and the below list is sent to this function as well as Taylor Swift the user, then this boolean function should return false (because the first comment was made by Happy Gilmore), true, true, true.

But it is not working. It is doing the first comment, setting it to true or false, then exiting out of the foreach loop setting everything that follows the first object in the list of comments to false.

public bool IsCommenter(List<Comment> comments, ApplicationUser user)
{
    if (user == null) throw new ArgumentNullException("User can't be null");
    if (comments.Count() <= 0) throw new ArgumentException("Must have more than one comment.");

    foreach(var comment in comments)
    {
        if (comment.AuthorId == user.Id)
        {
            return comment.IsCommenter = true;

        } else 
        {
            return comment.IsCommenter = false;
        }
    }

    return false;
}

I suspect it may be because of the final return false in the function, however, I put that in there because without it, I get an error that not all code paths return a value (which I don't see how that could be the case when it's an if/else, not an if/elseif. Any thoughts on making this work?

“I get an error that not all code paths return a value”: the foreach doesn't know if comments contains zero comments. Why would that be an ArgumentException? — Dour High Arch
– Dour High Arch, Commented Oct 25, 2018 at 16:40
Shouldn't IsCommenter be renamed to IsAuthor? Technically all comments are made by a commenter... :) — littlerufe
– littlerufe, Commented Oct 25, 2018 at 17:41
Also, as @DourHighArch mentioned, throwing an ArgumentException for an empty comments list puts a little more burden on the caller (they either have to verify the list is empty before calling the method, or write code to handle the exception). Why not just do nothing and return false? — littlerufe
– littlerufe, Commented Oct 25, 2018 at 17:42
The name of your method is pretty poor. IsCommenter() sounds like it determines a value (is some value some other value) not induces a side affect (changes an objects value). It doesn't seem valuable in your case to even return a value, so you may want to consider changing the signature to void UpdateIsCommenter(...). — Erik Philips
– Erik Philips, Commented Oct 25, 2018 at 18:05

Guy · Accepted Answer · 2018-10-25 16:38:28Z

3

Because you have return in the loop. Remove it

foreach(var comment in comments)
{
    if (comment.AuthorId == user.Id)
    {
        comment.IsCommenter = true;
    }
    else 
    {
        comment.IsCommenter = false;
    }
}

Or simplified

foreach(var comment in comments)
{
    comment.IsCommenter = comment.AuthorId == user.Id;
}

edited Oct 25, 2018 at 16:38

answered Oct 25, 2018 at 16:37

Guy

51.2k10 gold badges49 silver badges96 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

J.G.Sable Over a year ago

and change the return type to void in the method signature? (if I keep it to bool it will still tell me not all code paths return a value)

Guy Over a year ago

@J.G.Sable It shouldn't, but since now you just return false you can make the method void.

Collectives™ on Stack Overflow

foreach loop stopping after first iteration in

1 Answer 1

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related