2
\$\begingroup\$

in my ASP.NET MVC application I have a service that had a method for paging, sorting and filtering of Vehicle Makes:

public class VehicleService : IVehicleService
{
    private readonly DbContext _context;

    public VehicleService(DbContext context)
    {
        _context = context;
    }

    public async Task<IPagedList<VehicleMake>> GetVehicleMakesWithPaginationAsync(string search, int? page, string sort)
    {
        var makes = _context.VehicleMakes.AsQueryable();

        switch (sort)
        {
            case "Name desc":
                makes = makes.OrderByDescending(x => x.Name);
                break;
            default:
                makes = makes.OrderBy(x => x.Name);
                break;
        }

        return await makes.Where(x => x.Name.StartsWith(search) || search == null).ToPagedListAsync(page ?? 1, 5);
    }
}

VehicleService interface:

public interface IVehicleService{
Task<X.PagedList.IPagedList<VehicleMake>> GetVehicleMakesWithPaginationAsync(string search, int? page, string sort);
}

After the review of my code, I was told that sorting, filtering and paging should be in separate classes that have interfaces. I implemented that in following way:

Sorting:

internal class Sorting : ISorting
{
    private readonly DbContext _context;

    public Sorting(DbContext context)
    {
        _context = context;
    }

    public IEnumerable<VehicleMake> SortMakes(string sort)
    {
        var makes = _context.VehicleMakes.AsQueryable();

        makes = sort == "Name desc" ? makes.OrderByDescending(x => x.Name) : makes.OrderBy(x => x.Name);
        return makes;
    }
}

Sorting interface:

interface ISorting
{
 IEnumerable<VehicleMake> SortMakes(string sort);
}

Paging:

class Paging : IPaging
{
    private readonly ISorting _sorting;

    public Paging(DbContext context)
    {
        _sorting = new Sorting(context);
    }

    public async Task<IPagedList<VehicleMake>> GetPagedListOfSortedMakes(string search, int? page, string sort)
    {
        var sortedMakes = _sorting.SortMakes(sort).AsQueryable();
        return await sortedMakes.Where(x => x.Name.StartsWith(search) || search == null).ToPagedListAsync(page ?? 1, 5);
    }
}

Paging interface:

interface IPaging
   {
    Task<IPagedList<VehicleMake>> GetPagedListOfSortedMakes(string search, int? page, string sort);
   }

And then in my service:

public class VehicleMakeService : IVehicleMakeService
{
    private readonly DbContext _context;
    private readonly IPaging _paging;

    public VehicleMakeService(DbContext context)
    {
        _context = context;
        _paging = new Paging(context);
    }

    public async Task<IPagedList<VehicleMake>> GetVehicleMakesWithPaginationAsync(string search, int? page, string sort)
    {
        return await _paging.GetPagedListOfSortedMakes(search, page, sort);
    }
}

This works well, but I'm not sure if I implemented this correctly. Is there a better (cleaner) way to do this?

\$\endgroup\$
  • \$\begingroup\$ Could you please add the interfaces too? \$\endgroup\$ – t3chb0t Dec 8 '17 at 20:06
  • \$\begingroup\$ I added the interfaces. \$\endgroup\$ – iantukic Dec 9 '17 at 7:44
2
\$\begingroup\$

For my money it would only be worthwhile having separate classes for Sorting/Filtering/Pagination if they were generic and able to work on multiple entity types. Even then they'd more likely be helper classes than a part of the top-level application interface.

If the current requirement of your application is purely to do what GetVehicleMakesWithPaginationAsync does then the proposed split into 3 classes doesn't strike me as justified. I'm more concerned about the nature/names of the parameters being passed.

Why not make "sort" a boolean flag called "sortDescending"? And I'd rename search to namePrefix or something that makes it clearer.

Lastly - is the AsQueryable() really needed?

\$\endgroup\$
0
\$\begingroup\$

I agree with Dylan. Even if you are going to swap multiple sorting implementations in and out (as in strategy pattern) or reuse Sorting class in multiple places, ISorting interface still looks like an unnecessary abstraction layer, at least in its current form, where it simply wraps LINQ query.

As for your code:

1) You should try to flatten dependency tree. Right now your service depends on paging, which depends on sorting, which would probably depend on filtering, etc. It results in tightly coupled, hard-to-follow code with deep call stack.

2) Your ISorting interface should do one thing, which is sorting. It probably should not have a direct access to database.

Also it might be a good idea to invert dependencies, but it depends on how the rest of your code base is organized:

public VehicleMakeService(DbContext context, ISoirting sorting, IFiltering filter, IPaging paging)
{
    ...
}

public async Task<IPagedList<VehicleMake>> GetVehicleMakesWithPaginationAsync(...)
{
    var query = _context.VehicleMakes.AsQueryable();
    query = _filter.Filter(query, ...);
    query = _sorting.Sort(query, ...);
    return await _paging.Paginate(query, ...);
}
\$\endgroup\$

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.