2
\$\begingroup\$

I have the following controller with a couple of database queries and I'd like you to review it.

I've tried to make it clean for other developers but the communication with the database can be improved (more effective). The exception returned when nothing is found, so I'm not sure if it's the best way to do it.

public class LookupController : ApiController
{
    // GET api/Lookup
    [CacheOutput(ServerTimeSpan = 3600, ClientTimeSpan = 3600)]
    public List<GlassModel> Get(string filter)
    {
        if (string.IsNullOrEmpty(filter)) throw new ArgumentNullException(nameof(filter));

        using (var db = new LookupDbContext())
        {
            var salesCodes = GetSalesCodes(db, filter.ToUpper().Split(','));

            if (salesCodes.Any())
            {
                var glassQuantities = GetQuantitesByCountry(salesCodes, db);

                if (glassQuantities.Any())
                    return glassQuantities;
            }
            throw new SystemException("Value not found.");
        }
    }

    private List<GlassModel> GetQuantitesByCountry(List<string> salesCodes, LookupDbContext db)
    {
        var queryGlass = db.Countries
            .Where(c => salesCodes.Contains(c.Eurocode))
           .GroupBy(c => new { c.Countrycode })
           .Select(g => new
           {
               g.Key.Countrycode,
               Quantity = g.Sum(p => p.Quantity)
           });

        // build model
        return queryGlass.ToList().Select(x => new GlassModel
        {
            Coordinates = LocationHelper.GetCoordinatesByCountry(x.Countrycode),
            Country = x.Countrycode,
            Quantity = x.Quantity
        }).ToList();
    }

    private List<string> GetSalesCodes(LookupDbContext db, string[] filterValues)
    {
        var make = filterValues[0];
        IQueryable<Car> filter;

        if (filterValues.Length == 1) // make
        {
            filter = db.Cars.Where(c => c.VehicleMakeName.Contains(make));
        }
        else if (filterValues.Length == 2) // make, model
        {
            var carModel = filterValues[1].Trim();

            filter = db.Cars.Where(c => c.VehicleMakeName.Contains(make) &&
                                         c.VehicleModelName.Contains(carModel));
        }
        else // make, model, year
        {
            var model = filterValues[1].Trim();
            var year = Convert.ToDouble(filterValues[2]);

            filter = db.Cars.Where(c => c.VehicleMakeName.Contains(make) &&
                                          c.VehicleModelName.Contains(model) &&
                                          (c.FromYear <= year || c.FromYear == null) &&
                                          (c.ToYear >= year || c.ToYear == null));
        }

        return filter
            .GroupBy(c => new { c.SalesCode })
            .Select(g => g.Key.SalesCode).ToList();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ For one: you typically return a 404 when something isn't found. \$\endgroup\$ – Gert Arnold Apr 5 '16 at 10:17
  • \$\begingroup\$ Do you consider any improvement for the db queries? \$\endgroup\$ – tribet Apr 5 '16 at 15:55
3
\$\begingroup\$

I see two improvements, one of which Gert Arnold pointed on in a comment.

  • If an API method returns a list, collection or IEnumerable of anything, worse case scenario you should return an empty collection. I wouldn't even recommend returning a 404 Not Found because you are searching for multiple things. If returning a single resource, a 404 Not Found is appropriate, but not for returning a collection.

  • Abstract away the searching and generation of your view models into their own service. This makes it much easier to test your controller.

Moving query logic into a service

This is a three part process:

  1. Create an interface for your service that exposes the minimal methods and parameters required for your controller to work

  2. Create the service class that implements the interface

  3. Use this service object in your controller through its interface

First, the interface:

public interface IVehicleService
{
    List<GlassModel> GetGlassModelQuantitesByCountry(string[] filterValues);
}

This has a single method that accepts the filter values, and returns a list of view models. I called it IVehicleService because the actual entity your are querying against is a Car (which really is a "vehicle" in the generic sense).

Before diving into the implementation of the IVehicleService interface, let's look at how the controller will use it.

public class LookupController : ApiController
{
    // GET api/Lookup
    [CacheOutput(ServerTimeSpan = 3600, ClientTimeSpan = 3600)]
    public List<GlassModel> Get(string filter)
    {
        if (string.IsNullOrEmpty(filter)) throw new ArgumentNullException(nameof(filter));

        using (var db = new LookupDbContext())
        {
            IVehicleService service = new VehicleService(db);
            IEnumerable<string> filterValues = filter.ToUpper().Split(',');

            return service.GetGlassModelQuantitesByCountry(filterValues );
        }
    }
}

This makes the controller very slim and easy to read. The ugliness of the query is hidden away in the service class.

public class VehicleService : IVehicleService
{
    public VehicleService(LookupDbContext context)
    {
        this.context = context;
    }

    private LookupDbContext context;

    public List<GlassModel> GetGlassModelQuantitesByCountry(IEnumerable<string> filterValues)
    {
        var salesCodes = GetSalesCodes(filterValues);
        var queryGlass = context.Countries
            .Where(c => salesCodes.Contains(c.Eurocode))
            .GroupBy(c => new { c.Countrycode })
            .Select(g => new
            {
                g.Key.Countrycode,
                Quantity = g.Sum(p => p.Quantity)
            });

        // build model
        return queryGlass.ToList().Select(x => new GlassModel
        {
            Coordinates = LocationHelper.GetCoordinatesByCountry(x.Countrycode),
            Country = x.Countrycode,
            Quantity = x.Quantity
        }).ToList();
    }

    private GetSalesCodes(IEnumerable<string> filterValues)
    {
        var make = filterValues[0];
        IQueryable<Car> filter;

        if (filterValues.Length == 1) // make
        {
            filter = context.Cars.Where(c => c.VehicleMakeName.Contains(make));
        }
        else if (filterValues.Length == 2) // make, model
        {
            var carModel = filterValues[1].Trim();

            filter = context.Cars.Where(c => c.VehicleMakeName.Contains(make)
                                          && c.VehicleModelName.Contains(carModel));
        }
        else // make, model, year
        {
            var model = filterValues[1].Trim();
            var year = Convert.ToDouble(filterValues[2]);

            filter = context.Cars.Where(c => c.VehicleMakeName.Contains(make)
                                          && c.VehicleModelName.Contains(model)
                                          && (c.FromYear <= year || c.FromYear == null)
                                          && (c.ToYear >= year || c.ToYear == null));
        }

        return filter
            .GroupBy(c => new { c.SalesCode })
            .Select(g => g.Key.SalesCode).ToList();
    }
}

Move "filter" logic into its own class

There is a slight problem here. The "filter" is a comma separated value string. We can improve this too by making a class called VehicleSearchFilter which will give you a strongly typed way to define the search parameters:

public abstract class VehicleSearchFilter
{
    public static VehicleSearchFilter Parse(string filter)
    {
        if (string.IsNullOrEmpty(filter))
            throw new ArgumentNullException(nameof(filter));

        string[] filterValues = filter.ToUpper().Split(',');
        string make, model;
        int? year;

        if (filterValues.Length > 0)
        {
            make = filterValues[0];
        }

        if (filterValues.Length > 1)
        {
            model = filterValues[1];
        }

        if (filterValues.Length > 2)
        {
            year = int.Parse(filterValues[2]);
        }

        return year == null
            ? new VehicleSearchFilter(make, model)
            : new VehicleSearchFilter(make, model, year);
    }

    public VehicleSearchFilter(string, make string model)
    {
        Make = make;
        Model = model;
    }

    public VehicleSearchFilter(string make, string model, int year)
    {
        Make = make;
        Model = model;
        Year = year;
    }

    public string Make { get; set; }
    public string Model { get; set; }
    public int? Year { get; set; }
}

Now your vehicle search service can use a VehicleSearchFilter object instead of a list of strings:

public class LookupController : ApiController
{
    // GET api/Lookup
    [CacheOutput(ServerTimeSpan = 3600, ClientTimeSpan = 3600)]
    public List<GlassModel> Get(string filter)
    {
        if (string.IsNullOrEmpty(filter)) throw new ArgumentNullException(nameof(filter));

        using (var db = new LookupDbContext())
        {
            IVehicleService service = new VehicleService(db);
            VehicleSearchFilter searchFilter = VehicleSearchFilter.Parse(filter);

            return service.GetGlassModelQuantitesByCountry(searchFilter);
        }
    }
}

And then a few changes to the VehicleService class to use this:

public class VehicleService : IVehicleService
{
    public VehicleService(LookupDbContext context)
    {
        this.context = context;
    }

    private LookupDbContext context;

    public List<GlassModel> GetGlassModelQuantitesByCountry(VehicleSearchFilter filter)
    {
        var salesCodes = GetSalesCodes(filter);
        var queryGlass = context.Countries
            .Where(c => salesCodes.Contains(c.Eurocode))
            .GroupBy(c => new { c.Countrycode })
            .Select(g => new
            {
                g.Key.Countrycode,
                Quantity = g.Sum(p => p.Quantity)
            });

        // build model
        return queryGlass.ToList().Select(x => new GlassModel
        {
            Coordinates = LocationHelper.GetCoordinatesByCountry(x.Countrycode),
            Country = x.Countrycode,
            Quantity = x.Quantity
        }).ToList();
    }

    private GetSalesCodes(VehicleSearchFilter filter)
    {
        var make = filterValues[0];
        IQueryable<Car> filter;

        if (filter.Year == null && filter.Model == null)
        {
            filter = context.Cars.Where(c => c.VehicleMakeName.Contains(filter.Make));
        }
        else if (filter.Model != null)
        {
            filter = context.Cars.Where(c => c.VehicleMakeName.Contains(filter.Make)
                                          && c.VehicleModelName.Contains(filter.Model));
        }
        else
        {
            filter = context.Cars.Where(c => c.VehicleMakeName.Contains(filter.Make)
                                          && c.VehicleModelName.Contains(filter.Model)
                                          && (c.FromYear <= filter.Year || c.FromYear == null)
                                          && (c.ToYear >= filter.Year || c.ToYear == null));
        }

        return filter
            .GroupBy(c => new { c.SalesCode })
            .Select(g => g.Key.SalesCode).ToList();
    }
}

This gives you some compile time checks for your queries, plus it centralizes the parsing of the make, model and year values (which is generic enough I imagine you use it elsewhere too).

\$\endgroup\$
  • \$\begingroup\$ Before adding the VehicleSearchFilter class in your example you are using IEnumerable<string> type for the filter which is not working if you use it as an array. Is that an error? \$\endgroup\$ – tribet Apr 5 '16 at 18:23
  • \$\begingroup\$ @tribet: An array of strings (string[]) is an IEnumerable<string>. Not sure why it's not working. This should compile: IEnumerable<string> filter = new string[1] { "Foo" }; \$\endgroup\$ – Greg Burghardt Apr 5 '16 at 20:21

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.