5
\$\begingroup\$

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; }
}
\$\endgroup\$

1 Answer 1

7
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Great! Thanks. I was wondering how I could better use those associations! \$\endgroup\$
    – mawburn
    Commented Jul 13, 2012 at 1:31
  • \$\begingroup\$ +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. \$\endgroup\$
    – vgru
    Commented Jul 13, 2012 at 7:51
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Jul 13, 2012 at 13:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.