Using lambda expressions and linq

Question

So I've just started working with linq as well as using lambda expressions. I've run into a small hiccup while trying to get some data that I want. This method should return a list of all projects that are open or in progress from Jira

Here's the code

    public static List<string> getOpenIssuesListByProject(string _projectName)
    {
        JiraSoapServiceService jiraSoapService = new JiraSoapServiceService();
        string token = jiraSoapService.login(DEFAULT_UN, DEFAULT_PW);
        string[] keys = { getProjectKey(_projectName) };

        RemoteStatus[] statuses = jiraSoapService.getStatuses(token);
        var desiredStatuses = statuses.Where(x => x.name == "Open" || x.name == "In Progress")
            .Select(x=>x.id);

        RemoteIssue[] AllIssues = jiraSoapService.getIssuesFromTextSearchWithProject(token, keys, "", 99);
        IEnumerable<RemoteIssue> openIssues = AllIssues.Where(x=>
            {
                foreach (var v in desiredStatuses)
                {
                    if (x.status == v)
                        return true;
                    else
                        return false;
                }
                return false;
            });
        return openIssues.Select(x => x.key).ToList();
    }

Right now this only select issues that are "Open", and seems to skip those that are "In Progress".

My question: First, why am I only getting the "Open" Issues, and second is there a better way to do this?

The reason I get all the statuses first is that the issue only stores that statuses ID, so I get all the statuses, get the ID's that match "Open" and "In Progress", and then match those ID numbers to the issues status field.

Answer 1

Well, you can change

IEnumerable<RemoteIssue> openIssues = AllIssues.Where(x=>
{
    foreach (var v in desiredStatuses)
    {
        if (x.status == v)
            return true;
        else
            return false;
    }
    return false;
});

to

IEnumerable<RemoteIssue> openIssues =
      AllIssues.Where(x=> desiredStatuses.Contains(x.status));

As for why you aren't getting both statuses - Stephan has answered that. My code change above will fix that problem too.

Answer 2

IEnumerable<RemoteIssue> openIssues = AllIssues.Where(x=>
        {
            foreach (var v in desiredStatuses)
            {
                if (x.status == v)
                    return true;
            }
            return false;
        });

The code you had was only checking the first state and returning false. You need to iterate all states and return false only if it's not in the list at all.

Answer 3

The reason that you are only getting one status, is that you are always returning out of the loop after the first check. If the first item doesn't match, no more items are checked. If you remove the return in the else, it will work:

foreach (var v in desiredStatuses) {
  if (x.status == v) {
    return true;
  }
}
return false;

You should make sure to realise the collection of desired statuses, so that you don't rerun the query that creates it every time that you use it:

var desiredStatuses =
  statuses
  .Where(x => x.name == "Open" || x.name == "In Progress")
  .Select(x=>x.id)
  .ToList();

If there are only a few statuses that you want to check for, there is no need to make it more efficient than that. If there are a lot of statuses, you could realise the statuses into a HashSet and use it's Contains method which is a lot faster than looping through the items.

Answer 4

The code looks right to me, although there are ways to do it with less written code...

Regarding:

Right now this only select issues that are "Open", and seems to skip those that are "In Progress".

Can you confirm that both are in the desiredStatuses though?

Also I assume that RemoteIssue.status property is indeed referring to the id of the status rather than name, since that's what you're comparing it to?

Then for the code, as per Martin Harris's answer: I would use Contains operator rather than your inner loop...

Answer 5

Change this:

IEnumerable<RemoteIssue> openIssues = AllIssues.Where(x=>
            {
                foreach (var v in desiredStatuses)
                {
                    if (x.status == v)
                        return true;
                    else
                        return false;
                }
                return false;
            });

To this:

IEnumerable<RemoteIssue> openIssues = AllIssues.Where(x=>
            {
                foreach (var v in desiredStatuses)
                {
                    if (x.status == v)
                        return true;
                    //else
                        //return false;
                }
                return false;
            });

Answer 6

The other answers are right, but you can do this a little more concisely with a straight Lambda instead of an anonymous delegate.

IEnumerable<RemoteIssue> openIssues = AllIssues.Where(x=> 
        desiredStatuses.Contains(x.status)

So your whole method would look like:

public static List<string> getOpenIssuesListByProject(string _projectName)
{
    JiraSoapServiceService jiraSoapService = new JiraSoapServiceService();
    string token = jiraSoapService.login(DEFAULT_UN, DEFAULT_PW);
    string[] keys = { getProjectKey(_projectName) };

    RemoteStatus[] statuses = jiraSoapService.getStatuses(token);
    var desiredStatuses = statuses.Where(x => x.name == "Open" || x.name == "In Progress")
        .Select(x=>x.id);

    RemoteIssue[] AllIssues = jiraSoapService.getIssuesFromTextSearchWithProject(token, keys, "", 99);
    IEnumerable<RemoteIssue> openIssues = AllIssues.Where(x => desiredStatuses.Contains(x.status));
    return openIssues.Select(x => x.key).ToList();
}

This basically emits the equivalent of a SQL "IN" clause. So your statement reads:

SELECT <RemoteIssue> FROM AllIssues AS x WHERE x.status IN <desiredStatuses>