How can I refactor my LinqToSql to be more maintainable?

Question

I am using Linq2Sql as my data access, but I have quickly noticed that I am doing something wrong.

I have a list of Posts, and I need to be able to filter it in several different ways. For example:

• GetNewest()
• GetOldest()
• GetForUser(int)
• GetByCategory(int)

Each method returns a Model I have called PostViewModel. It is the expanded version of a post, and some of its foreign keys. (Author name, etc...)

A post looks like:

class Post
{
    int Id;
    DateTime DateCreated;
    int AuthorId;
}

and the PostViewModel would be:

class PostViewModel
{
    int Id;
    DateTime DateCreated;
    string AuthorName;
}

Each Method above is something like

from p in db.Posts
where p.StatusID == (int)PostStatus.Visible //this can be different for each
order by <something>  //this can be different for each
select new PostViewModel() //this is the same for each
{
    //snip
}

I feel like I am doing something wrong. If I make changes to the PostViewModel, I will have to change each of those methods. It may be relevant to note that I expand the object into the PostViewModel so I don't have to go back to the database to get the AuthorName (and a few others) whenever I iterate over the result and display it.

Sp what can I do here? I'm not afraid of making drastic changes if needed.

Answer 1

To start with, if your queries are relatively simple, I would be inclined to avoid creating a set of data access helper methods like GetNewest(), GetOldest(), GetForUser(int), GetByCategory(int), etc. These kinds of methods can be counter-productive as they hide underlying details that may be important to the calling code and they make combining queries difficult.

For example, you might implement GetNewest() like this:

public IQueryable<Post> GetNewest()
{
    return
        from p in db.Posts
        orderby p.DateCreated descending
        select p;
}

But then later include the where p.StatusID == (int)PostStatus.Visible clause like so:

public IQueryable<Post> GetNewest()
{
    return
        from p in db.Posts
        where p.StatusID == (int)PostStatus.Visible
        orderby p.DateCreated descending
        select p;
}

Now, all the calling code still reads as GetNewest() so just by looking at the calling code you don't know if you're getting visible or not or what the ordering is. And in fact, some existing code might expect that GetNewest() returns invisible posts and now you've broken that code without realizing it.

On the other hand though, you don't want queries all smattered throughout your code. If you change your database schema then you could have lots of broken code.

The right way to go is think about queries as series of composible, atomic operations.

So this is what I'd suggest you do for your data access code:

public static IQueryable<Post> WhereStatusVisible(
    this IQueryable<Post> posts)
{
    return
        from p in posts
        where p.StatusID == (int)PostStatus.Visible
        select p;
}

public static IQueryable<Post> OrderByDateCreatedDescending(
    this IQueryable<Post> posts)
{
    return
        from p in posts
        orderby p.DateCreated descending
        select p;
}

Now you can write this kind of calling code:

var query =
    db.Posts
        .WhereStatusVisible()
        .OrderByDateCreatedDescending();

This becomes very clear what is going on and it is not likely that the underlying implementation of these extension methods would ever change and all of the schema specifics are hidden in the extension methods.

You can then extend this to handle the creation of your view models. Now since the view models are not part of your database I would go from IQueryable<Post> to IEnumerable<PostViewModel> and since it seems like you need to join the author (presumably from the User table) I would do something like this:

public static IEnumerable<PostViewModel> ToPostViewModels(
    this IQueryable<Post> posts,
    IQueryable<User> users)
{
    var query =
        from p in posts
        join u in users on p.AuthorId equals u.Id
        select new { p, u };

    return
        query
            .ToArray()
            .Select(q => new PostViewModel()
            {
                Id = q.p.Id,
                DateCreated = q.p.DateCreated,
                AuthorName = q.u.Name,
            })
            .ToArray();
}

Now the calling code looks like this:

var postViewModels =
    db.Posts
        .WhereStatusVisible()
        .OrderByDateCreatedDescending()
        .ToPostViewModels(db.Users);

I would then look at doing this kind of code too:

public static IQueryable<Post> GetByCategories(
    this IQueryable<Post> posts,
    IQueryable<Category> categories)
{
    return
        from p in posts
        join c in categories on p.CategoryId equals c.Id
        select p;
}

public static IQueryable<Category> WhereStartsWith(
    this IQueryable<Category> categories,
    string startsWith)
{
    return
        from c in categories
        where c.Name.StartsWith(startsWith)
        select c;
}

This would enable this kind of calling code:

var postViewModelsInCategoriesStartingWithN =
    db.Posts
        .GetByCategories(db.Categories.WhereStartsWith("N"))
        .WhereStatusVisible()
        .ToPostViewModels(db.Users);

So the bottom-line with this approach is that you:

1. provide atomic, composible operators that act on IQueryable<T> going to IQueryable<T>.
2. use IQueryable<> when in the database and go to IEnumerable<> when you work with non-database classes.
3. avoid making your calling code dependent on database specifics (i.e. don't get by int, do so by User or IQueryable<User>).

If there's anything further let me know. I hope this helps.

Answer 2

For one, you can create a set of extension methods helping you to do common tasks, like:

static class PostQueryExtensions
{
    public static IQueryable<PostViewModel> Materialize(this IQueryable<Post> query)
    {
        return from post in query
               select new PostViewModel()
               {
                   ....
               }
    }
}

class PostDataAccess
{
    public IEnumerable<PostViewModel> GetOldest()
    {
        var query = db.Posts
                      .OrderBy(x => x.DateCreated)
                      .Materialize();
    }

    public IEnumerable<PostViewModel> GetNewest()
    {
        var query = db.Posts
                      .OrderByDescending(x => x.DateCreated)
                      .Materialize();
    }
}

You are free to include any type of extension method you want. including sorting, filtering etc. So repetitive tasks can be transformed in query extensions.

If you are affraid your model might change in the future (or requirements), you can even extend this for each simple basic task. Lets take an example: The requirement is that most methods sort by PosterName. so in most queries you have something like: order by p.Name. Now what if suddenly the Name field is extended with LastName. instead of having to sort only on the Name field, you now have to sort on 2 fields. However, if you have made a Query extension in the beginning like:

public static IQueryable<Post> OrderByName(this IQueryable<Post> query) { ... }

You only have to modify the implementation of OrderByName and each method using this will be automatically affected. (This is called by many the pipeline pattern).

Answer 3

Here would be one way:

public enum PostType
{
    Oldest,
    Newest,
    ForUser,
    ByCat
}
public List<PostViewModel> GetPosts(PostType pt, YourDataContext db, int UserId = 0)
{
    List<PostViewModel> v = db.Posts.Select(i => new PostViewModel() { /* snip */});
    switch(pt)
    {
        case pt.Oldest:
            v = v.Where(i => p.StatusID == (int)PostStatus.Visible).OrderDescendingBy(i => i.DateCreated).ToList();
            break;
        case pt.ByUser:
            v = v.Where(i => p.UserId == UserId).ToList();
            break;
           ...

    }   
    return v;
}