My code is very inneficient, mind taking a look for this simple Linq usage?

Question

I have the following method that is supposed to parse information from an XML response and return a collection of users.

I've opted into creating a Friend class and returning a List to the calling method.

Here's what I have so far, but I noticed that the ids.ToList().Count method parses every single id element to a List, then does it again in the for conditional. It's just super ineffective. I need some help solving this because I'm not that great with Linq. Thank you!

        public List<Friend> FindFriends()
        {
            List<Friend> friendList = new List<Friend>();

            var friends = doc.Element("ipb").Element("profile").Element("friends").Elements("user");
            var ids = from fr in friends
                      select fr.Element("id").Value;

            var names = from fr in friends
                        select fr.Element("name").Value;

            var urls = from fr in friends
                        select fr.Element("url").Value;

            var photos = from fr in friends
                        select fr.Element("photo").Value;

            if (ids.ToList().Count > 0)
            {
                for (int i = 0; i < ids.ToList().Count; i++)
                {
                    Friend buddy = new Friend();
                    buddy.ID = ids.ToList()[i];
                    buddy.Name = names.ToList()[i];
                    buddy.URL = urls.ToList()[i];
                    buddy.Photo = photos.ToList()[i];

                    friendList.Add(buddy);
                }
            }            

            return friendList;
        }

Answer 1

First question - do you have to return a List<Friend>? Can you return an IEnumerable<Friend> instead? If so, performance gets a lot better:

IEnumerable<Friend> FindFriends()
{
    return doc.Descendants("user").Select(user => new Friend {
        ID = user.Element("id").Value,
        Name = user.Element("name").Value,
        Url = user.Element("url").Value,
        Photo = user.Element("photo").Value
    });
}

Rather than actually creating new buckets and stuffing values into them, this creates a projection, or a new object that simply contains all of the logic for how to create the new Friend objects without actually creating them. They get created when the caller eventually starts to foreach over the IEnumerable. This is called "deferred execution".

This also makes one assumption - All the <user> nodes in your XML fragment are friends. If that isn't true, the first part of the XML selection might need to be a little more complex.

And as @anon points out, even if you do need to return a List<Friend> for some reason not obvious from the information you've provided, you can just call .ToList() at the end of the return statement. This will just execute the projection I described above straight into a new bucket, so you only ever create one.

Answer 2

Why do you need the separate ids/names/urls/photos variables? Combine it all. You can eliminate the ToList() call if you don't need a List.

List<Friend> friendList = (from f in doc.Element("ipb").Element("profile").Element("friends").Elements("user")
                          select new Friend() { 
                                                 ID = f.Element("id").Value, 
                                                 Name = f.Element("name").Value,
                                                 URL = f.Element("url").Value,
                                                 Photo = f.Element("photo").Value
                                 }).ToList();

return friendList;