This LINQ Model Query seems extremely inefficient

Question

I'm new to LINQ, but I have some background in T-SQL. I know there are probably 100 different ways to design a T-SQL statement that would run this much more efficiently, but I'm not sure how I would do it in LINQ and I would like to stick with LINQ.

This works perfectly well, I just hate the way it looks and am unsure of how to fix it.

I am using MVC3, C#, LINQ, Entity Framework 4.3

public class CollectionRepository : ICollectionRepository {
 private CollectionEntities db = new CollectionEntities();
 public IEnumerable<Collection> GetCollectionByUid(long id) {
 var _collection = (from c in db.UserCollections
 where c.uid == id
 orderby c.CreateDate descending
 select new Collection {
 Name = c.CollectionName,
 Type = (from t in db.CollectionTypes
 where t.ctypeid == c.Type
 select t.CollectionTypeName).FirstOrDefault(),
 Created = c.CreateDate,
 Count = (from f in db.Figures
 where f.CollectionID == c.cid
 select f).Count()
 }).ToList();
 return _collection;
 }
}

The constructors it points to:

public class Collection {
 public string Name { get; set; }
 public string Type { get; set; }
 public DateTime Created { get; set; }
 public int Count { get; set; }
}

Answer 1

As this is EF code, assuming you have all the associations set up correctly, you should be able to write it something like this:

public IEnumerable<Collection> GetCollectionByUid(long uid) {
 var result =
 (from collection in db.UserCollections
 where collection.uid == uid
 orderby collection.CreateDate descending
 select new Collection {
 Name = collection.CollectionName,
 Type = collection.CollectionType.CollectionTypeName,
 Created = collection.CreateDate,
 Count = collection.Figures.Count(),
 }).ToList();
 return result;
}

My only real criticism is the variable names.

Local variables or parameters should not be prefixed with an underscore. IMHO, it should only be allowed for private instance fields. So rather than using _collection, it would be better off as collection (though I used result for the name here instead).

Parameter names are very much part of the documentation as well (especially so in C# and .NET in general). The method is called GetCollectionByUid which suggests you provide a uid. The parameter name should reflect that. Call it uid since that's what it is.

Personal preference but I also prefer to write out the full/reasonable-length variable names in queries. I reserve significantly shortened variable names (i.e., one character variable names) in lambda expressions.

• mawburn: Great! Thanks. I was wondering how I could better use those associations!
• vgru: +1 But I would also ditch the "Collection" prefix out of every property which belongs to a Collection class. Collection.Name and Collection.Type should be enough. On the other hand, the class itself should probably be renamed to FigureCollection (if that's what it's used for) or something similar which would explain what data it contains.
• Jeff Mercado: @Groo: You know, I didn't even notice the names of those properties on the entities. I definitely agree with that. Though it'd be moot if they can't be renamed in the DB. Even though the names of the entities and their properties doesn't necessarily have to match that of in the DB, they should always match IMHO.