8
\$\begingroup\$

I have a GET method that handles paging, filtering, and sorting. Development has been fairly rapid and I'm not completely sold on some of the implementation. A few assumptions have to be made for this method: I'm using System.Dynamic.Linq. I want to put as much work as possible on SQL Server. The page will make use of both the resulting record set after filtering and paging have been applied, as well as the count of the filtered record set before paging is applied. I have full control of the entity definitions, and each has a [Key] attribute on only one property. Currently, I'm only interested in seeing the IsActive records.

Any thoughts on improvements are welcome. Thanks in advance.

    public IList<TEntity> Get<TParamater>(IList<Expression<Func<TEntity, TParamater>>> includeProperties, out int recordCount, Dictionary<string,string> filter, bool filterExact,
    string orderBy = null, bool orderByAsc = true, int pageNumber = 0, int perPage = 10)

    {
        //IQueryable that will eventually contain the record set
        IQueryable<TEntity> query = _context.Set<TEntity>();
        //IQueryable that will get the count of post-filter, but pre-page records
        IQueryable<TEntity> countQuery = _context.Set<TEntity>();
        List<TEntity> page;


        //countQuery mirrors query through the filter assignments
        query = query.Where("IsActive = true");
        countQuery = countQuery.Where("IsActive = true");

        if (filter.Count > 0)
        {                
            string _template = filterExact ? "{0}.ToString().Equals(@0)" : "{0}.ToString().Contains(@0)";
            string _clause = "";
            foreach (KeyValuePair<string, string> entry in filter)
            {
                _clause = string.Format(_template, entry.Key);
                query = query.Where(_clause, entry.Value);
                countQuery = countQuery.Where(_clause, entry.Value);
            }               
        }
        //countQuery's job is finished at this point
        recordCount = countQuery.Count();

        if (includeProperties != null)
        {
            foreach (var include in includeProperties)
            {

                query = query.Include(include);
            }
        }

        if (orderBy != null)
        {
            query = query.OrderBy(orderBy + ((orderByAsc) ? " ascending" : " descending"));
        }
        else
        {
            //Using reflection to find the [Key] attribute to use as a default ordering because Skip/Take requires an ordered set
            Type entityType = typeof(TEntity);
            var keyField = entityType.GetProperties()
                .Single(p => Attribute.IsDefined(p, typeof(KeyAttribute)));
            query = query.OrderBy(keyField.Name);
        }

        if (pageNumber > 0)
        {
            int _start = (pageNumber - 1) * perPage;
            query = query.Skip(_start);
            query = query.Take(perPage);
        }

        page = query.ToList();

        return page;
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ One small thing: you don't need countQuery; you can use query for both counting and querying data, because EF will ignore Includes and ordering if you apply Count(). \$\endgroup\$ – Gert Arnold Jun 27 '16 at 19:40
  • \$\begingroup\$ Thanks for the feedback. That works splendidly. I was thinking that once an executable statement had been run the state was set and that was that. That duplicate IQueryable was bothering me the most. Sorry I can't upvote your comment, I'm too new on this particular community. I'll come back once I can though! \$\endgroup\$ – G. J. Jun 27 '16 at 21:05
4
\$\begingroup\$
//countQuery mirrors query through the filter assignments

I don't think that's necessary. You could have only one query for the first part of the method, then do recordCount = query.Count(); and then keep using query as you do now.

This way, you have less code and less chance of the count getting out of sync.


query.Where("IsActive = true")

I realize Dynamic Linq is an option, but I think you shouldn't use it when you don't have to. If possible, I would constraint TEntity to some Entity base class or IEntity interface (maybe you already do that) and then write:

query.Where(entity => entity.IsActive);

query = query.Skip(_start);
query = query.Take(perPage);

I don't see any reason why spread this over two lines:

query = query.Skip(_start).Take(perPage);

List<TEntity> page;

…

page = query.ToList();

return page;

The variable page does not serve any purpose, get rid of it:

return query.ToList();
\$\endgroup\$
  • \$\begingroup\$ I wouldn't get rid of the page variable. It's priceless when debugging but I'd define it just before the return. \$\endgroup\$ – t3chb0t Jul 3 '16 at 17:24
  • \$\begingroup\$ @t3chb0t VS2015 can show you what a function you called returned, so you don't need such variables anymore. \$\endgroup\$ – svick Jul 3 '16 at 17:35
  • \$\begingroup\$ This is true, but it's also true that it's easier to hover over it then to find it somewhere among the locals ;-] \$\endgroup\$ – t3chb0t Jul 3 '16 at 17:40

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.