CODEWITHSUNDEEP
c#.netlinqrefactoringresharper
Sean Anderson
Sean Anderson

Reputation: 29281

Resharper reworked a for loop. Is this LINQ statement doing too much in one line?

[WebMethod(Description = "Return all activities by Task.")]
public IList<ActivityDto> GetActivitiesByTaskID(int taskID)
{
    IList<Activity> activities = ActivityDao.GetByTaskID(taskID);
    IList<ActivityDto> activityDtos = new List<ActivityDto>(activities.Count);

    foreach(Activity activity in activities)
        activityDtos.Add(ActivityDto.Create(activity));

    return activityDtos;
}

shortened to:

[WebMethod(Description = "Return all activities by Task.")]
public IList<ActivityDto> GetActivitiesByTaskID(int taskID)
{
    return ActivityDao.GetByTaskID(taskID).Select(ActivityDto.Create).ToList();
}

I feel like because I have to ask -- it probably is doing a bit too much. The only part that causes me any concern, though, is passing the function to the Select statement. I think that I have just not used that syntax enough, though, and that it's actually quite a nifty way of expressing a lot succinctly.

I'd like to know if fellow programmers would be upset seeing the shortened version of this method.

EDIT: Also, comments on efficiency comparison would be appreciated. I know LINQ is notorious for working slower on large data sets. I would say 10,000-20,000 records could be enumerated in some instances.

Upvotes: 3

Views: 351

Answers (2)

Peter Ritchie
Peter Ritchie

Reputation: 35881

For what it's worth, that line is the equivalent of :

var activity =  ActivityDao.GetByTaskID(taskID);
var q = activity.Select(ActivityDto.Create);
return q.ToList();

You could do the above if you think what R# reworked it to could be more readable. If what you're concerned about is violating Law of Demeter then the above isn't going to help much--you'd have to redesign and separate concerns more/differently.

Upvotes: 1

Adam
Adam

Reputation: 15805

No, it is expressive and concise.

The first version is all about how to iterate over Lists and transform the elements.

The second version is all about what the result should be.

LINQ was created to give us powerful, readable tools for working with collections and transforming them. That's exactly what you're using it for in the second example.

For people unfamiliar with the method group syntax, another option might be to use a query expression:

var result = from task in ActivityDao.GetByTaskID(taskID)
             select ActivityDto.Create(task);
return result.ToList(); // if you really need it as a list

Upvotes: 10

Related Questions