8
\$\begingroup\$

Recently, I've come up with an idea of building a custom query builder combine with FluentApi.

Expected usage:

List<User> users = userQueryBuilder
                        .Active()
                        .IsMale()
                        .AgeGreaterThan(25)
                        .Include(u => u.UserProfile, u => u.Purchase);

User user = userQueryBuilder
                        .Active()
                        .HasIds(new List<int>{1})
                        .Include(u => u.UserProfile);

As you see from the expected behavior, my purpose is to make the code descriptive and easy to understand for all developers and most importantly, the code need to be flexible and welcome-for-changes. For instance, I could add IsMale() to any of existing query without changing any lines of code.

QueryBuilder.cs (base class)

public abstract class QueryBuilder<TEntity> : IDisposable where TEntity : class, new()
{
    protected DbContext Context { get; set; }

    /// <summary>
    /// The query object
    /// </summary>
    protected static IQueryable<TEntity> Query;

    /// <summary>
    /// Initializes a new instance of the <see cref="QueryBuilder{TEntity}"/> class.
    /// </summary>
    protected QueryBuilder(DbContext context)
    {
        Context = context;
        Query = Context.Set<TEntity>();
    }

    /// <summary>
    /// Performs an implicit conversion from <see cref="QueryBuilder{TEntity}"/> to <see cref="List{TEntity}"/>.
    /// </summary>
    /// <param name="queryBuilder">The query builder.</param>
    /// <returns>
    /// The result of the conversion.
    /// </returns>
    public static implicit operator List<TEntity>(QueryBuilder<TEntity> queryBuilder)
    {
        return Query.ToList();
    }

    /// <summary>
    /// Performs an implicit conversion from <see cref="QueryBuilder{TEntity}"/> to <see cref="TEntity"/>.
    /// </summary>
    /// <param name="queryBuilder">The query builder.</param>
    /// <returns>
    /// The result of the conversion.
    /// </returns>
    public static implicit operator TEntity(QueryBuilder<TEntity> queryBuilder)
    {
        return Query.FirstOrDefault();
    }

    /// <summary>
    /// Execute query and return the result as list 
    /// </summary>
    /// <returns></returns>
    public List<TEntity> ToList()
    {
        return Query.ToList();
    }

    /// <summary>
    /// Joins with the specified t key.
    /// </summary>
    /// <typeparam name="TTargetEntity">The type of the target entity.</typeparam>
    /// <typeparam name="TKey">The type of the key.</typeparam>
    /// <param name="tKey">The t key.</param>
    /// <param name="uKey">The u key.</param>
    /// <returns></returns>
    public QueryBuilder<TEntity> Join<TTargetEntity, TKey>(Expression<Func<TEntity, TKey>> tKey, Expression<Func<TTargetEntity, TKey>> uKey) where TTargetEntity : class
    {
        Query = Query.Join(Context.Set<TTargetEntity>(), tKey, uKey, (tblT, tblU) => tblT);
        return this;
    }

    /// <summary>
    /// Joins with predicate.
    /// </summary>
    /// <typeparam name="TTargetEntity">The type of the target entity.</typeparam>
    /// <typeparam name="TKey">The type of the key.</typeparam>
    /// <param name="tKey">The t key.</param>
    /// <param name="uKey">The u key.</param>
    /// <param name="whereExpressions">The where expressions.</param>
    /// <returns></returns>
    public QueryBuilder<TEntity> JoinWithPredicate<TTargetEntity, TKey>(Expression<Func<TEntity, TKey>> tKey, Expression<Func<TTargetEntity, TKey>> uKey, params Expression<Func<TTargetEntity, bool>>[] whereExpressions) where TTargetEntity : class
    {
        if (whereExpressions == null) return this;

        var targetSets = Context.Set<TTargetEntity>().AsQueryable();
        targetSets = whereExpressions.Aggregate(targetSets, (current, expression) => current.Where(expression));

        Query = Query.Join(targetSets, tKey, uKey, (tblT, tblU) => tblT);

        return this;
    }

    /// <summary>
    /// Loop through expressions and include it into the query
    /// </summary>
    /// <param name="includeExpressions">The include expressions.</param>
    /// <returns></returns>
    public QueryBuilder<TEntity> Include(params Expression<Func<TEntity, object>>[] includeExpressions)
    {
        foreach (var includeExpression in includeExpressions)
        {
            Query = Query.Include(includeExpression);
        }

        return this;
    }

    /// <summary>
    /// Execute the query and get the first item.
    /// </summary>
    /// <returns></returns>
    public TEntity FirstOrDefault()
    {
        return Query.FirstOrDefault();
    }

    /// <summary>
    /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.
    /// </summary>
    public void Dispose()
    {
        Context.Dispose();
        Query = null;
    }
}

UserQueryBuilder.cs (sample implementation of a QueryBuilder)

public class UserQueryBuilder : QueryBuilder<User>
{
    public UserQueryBuilder() : base(new UserDbContext())
    {
    }

    public UserQueryBuilder Active()
    {
        Query = Query.Where(user => user.IsActive);
        return this;
    }

    public UserQueryBuilder HasIds(List<int> ids)
    {
        Query = Query.Where(user => ids.Contains(user.Id));
        return this;
    }

    public UserQueryBuilder AgeGreaterThan(int age)
    {
        Query = Query.Where(user => user.Age >= age);
        return this;
    }

    public UserQueryBuilder IsMale()
    {
        Query = Query.Where(user => user.Gender == Gender.Male);
        return this;
    }

    public UserQueryBuilder IsFemale()
    {
        Query = Query.Where(user => user.Gender == Gender.Female);
        return this;
    }
}

Is there anything I can improve here?

\$\endgroup\$
  • \$\begingroup\$ I have a question: aren't you satisfied with existing query syntax? Unless you want to forward 1:1 all supported methods I don't see any gain here (but nostalgia of good old time ™ pattern) \$\endgroup\$ – Adriano Repetti Dec 16 '15 at 7:41
  • \$\begingroup\$ @AdrianoRepetti: the current query syntax is not informative at all, I need to look into the query to understand what it's about. For example: Users.Where(u => u.IsActive && u.Gender == Gender.Male && u.age >= 25).Include(u => u.UserProfile);. And if I want to add additional condition, I will need to modify the where condition (might be very long). \$\endgroup\$ – Kien Chu Dec 16 '15 at 7:55
  • 1
    \$\begingroup\$ No, you can concatenate them as you're already doing: Users.Where(u => u.IsActive).Where(u => user.Gender == Gender.Male) and so on. You may also write from user in Users where IsActive && Gender = Gender.Male && Age >= 25... (second syntax is even shorter). \$\endgroup\$ – Adriano Repetti Dec 16 '15 at 8:02
  • \$\begingroup\$ basically, the querybuilder also use Where query behind the scene, I'm not trying to reinvent the wheel, just to make the code more readable, easier to maintain, and improve the reuseability. Maybe because my sample is too simple, but I think the QueryBuilder will become more handy in more complex usecase. \$\endgroup\$ – Kien Chu Dec 16 '15 at 8:09
  • 2
    \$\begingroup\$ I understand there may be useful shortcuts to include, what I don't understand (at least from code sample) is the needing of a UserQueryBuilder class. Everything may be done using extension methods to easily integrate with existing queries (and reuse existing LINQ extensions when needed). \$\endgroup\$ – Adriano Repetti Dec 16 '15 at 8:20
4
\$\begingroup\$

Casting should never have side effects. Right now, calling ToList in your cast will send the query to the database. This is the most obscure way to execute a query and that's dangerous.

You should have an Execute method, which well.. executes the query and returns the result.

Active should be IsActive, to respect the boolean type of the method.

The Dispose of your query shouldn't dispose the DbContext. The QueryBuilder is dependant on DbContext, not upside down. When disposing, you should dispose of dependant objects. So now, the QueryBuilder shouldn't implement IDisposable, it serves no purpose.

Don't make Query static, that's something you'll regret. Imagine you have two queries to run at the same time, both your queries will step on each other and you'll have a mess of a query. Keep Query an instance property. Because while we're at it, you should make it a protected property. Instance variable shouldn't be anything other than private.

I want to point out that Contains using Entity Framework Linq To Entities is pretty slow. There's nothing much you can do about it, but just be aware that you might have problems with this someday.

I think your Context property should be private set;. You offer the possibility to your child classes to set the Context by the protected constructor, don't let them set it back.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good catch about dispose. Only the one who creates an object should dispose it. \$\endgroup\$ – Heslacher Dec 16 '15 at 15:04
  • \$\begingroup\$ @TopinFrassi: thank you very much for the review. Can you help me to write a sample code for execute method? And please explain why calling ToList() is dangerous. I often execute the query by either casting it to IEnumerable/List or serializing it. The reason I'm implementing IDisposable is I want to destroy the dbcontext & the query itself whenever the querybuilder is released (but I think your point is they will be automatically destroyed without writing any code, please correct me if I'm wrong). \$\endgroup\$ – Kien Chu Dec 17 '15 at 2:56
  • \$\begingroup\$ If I don't make Query protected, the child class cannot access it, I won't be able to add new functions for it (please see UserQueryBuilder.cs, I'm using the Query there). About the concurrent query, can you please suggest me a good way to implement? Because in my opinion, we should never let the query to be interfered with other query, it should be one-per-instance. Thanks again for help. \$\endgroup\$ – Kien Chu Dec 17 '15 at 3:01
  • \$\begingroup\$ Making it protected isn't the problem. But it really shouldn't be ` static` , that's the problem. Calling ToList isn't dangerous, but right now it is obscured behind an implicit casting and that is bad. Calling ToList directly is in fact much better than using the casting. You shouldn't destroy the ` context` when you release the querybuilder , that's a bad pattern. Heslacher gave a good explanation in his comment. \$\endgroup\$ – IEatBagels Dec 17 '15 at 3:09
3
\$\begingroup\$

As Adriano Repetti mentioned in comments you can already achieve this with extension methods on IQueryable.

E.g. for your users example:

public static class UserPredicates
{
    public static IQueryable<User> WhichAreMale(this IQueryable<User> users)
    {
        return users.Where(u => u.Gender == Gender.Male);
    }

    public static IQueryable<User> WhichAreActive(this IQueryable<User> users)
    {
        return users.Where(u => u.IsActive);
    }

    public static IQueryable<User> WhichHaveAgeGreaterThan(this IQueryable<User> users)
    {
        return users.Where(u => u.Gender == Gender.Male);
    }
}

Then you happily use it with your EF DbSet<User>:

var maleOver25s = Users
        .WhichAreActive()
        .WhichAreMale()
        .WhichHaveAgeGreaterThan(25)
        .Include(u => u.UserProfile);

Update:

You can also add in other collections as normal parameters:

public static class Predicates
{
    public static IQueryable<User> SomePredicate(this IQueryable<User> users, IQueryable<Course> courses)
    {
        // join courses and users and return users.
    }
}
\$\endgroup\$
  • \$\begingroup\$ yeah, this looks much more simple to me, maybe I'm overthinking here. One of the main reason I want to implement the query builder is I want to take advantage of db context (for example: do JOIN query). I don't know how to do JOIN with extensions. Another reason is I want to be able to switch the dbcontext easily (by passing the dbcontext to constructor of querybuilder) \$\endgroup\$ – Kien Chu Dec 16 '15 at 9:41
  • \$\begingroup\$ I'll update my answer with an example with a join \$\endgroup\$ – RobH Dec 16 '15 at 9:48
  • \$\begingroup\$ They're extension methods for IQueryable<User>, they're not tied to DbContext. \$\endgroup\$ – Adriano Repetti Dec 16 '15 at 9:48
  • \$\begingroup\$ @RobH: thanks for the edit. however, I feel like it's lacking generic here. That means I need to repeat the logic for every tables in every extensions (let's say I need to join 10 tables, I will need 10 extension methods to achieve that). One more thing is I need to pass the IQueryable object around to make query, that's what I want to avoid at all cost. What do you think? \$\endgroup\$ – Kien Chu Dec 17 '15 at 2:47
  • \$\begingroup\$ This is the approach I use in my own projects - well defined, reusable predicates are defined as extension methods then I compose them together in methods in my repositories/services. The predicates are generally internal to my DAL so you never return an IQueryable higher up the architecture. \$\endgroup\$ – RobH Dec 17 '15 at 7:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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