HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

API controller with database queries

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
asp.net-web-apicodereviewcsharpstackoverflowentity-frameworkc#
withcontrollerdatabasequeriesapi

Problem

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 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 GetQuantitesByCountry(List 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 GetSalesCodes(LookupDbContext db, string[] filterValues)
{
var make = filterValues[0];
IQueryable filter;

if (filterValues.Length == 1) // make
{
filter = db.Cars.Where(c => c.VehicleMakeName.Contains(make));
}
else if (filterValues.Length == 2) // make

Solution

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:

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

-
Create the service class that implements the interface

-
Use this service object in your controller through its interface

First, the interface:

public interface IVehicleService
{
List 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 Get(string filter)
    {
        if (string.IsNullOrEmpty(filter)) throw new ArgumentNullException(nameof(filter));

        using (var db = new LookupDbContext())
        {
            IVehicleService service = new VehicleService(db);
            IEnumerable 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 GetGlassModelQuantitesByCountry(IEnumerable 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 filterValues)
    {
        var make = filterValues[0];
        IQueryable 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.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)

Code Snippets

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 );
        }
    }
}
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();
    }
}
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; }
}
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);
        }
    }
}
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();
    }
}

Context

StackExchange Code Review Q#124769, answer score: 4

Revisions (0)

No revisions yet.