Reduce logic in Web API controller

Question

I have the following scenario - i have a grid on web page that is populated with data. User can check a check-boxes in grid and data is saved to DB on navigating to next page. If they then navigate back to the page and remove a selection the data is deleted from the DB and also if the original selection is still checked the data is not saved to the DB twice. It is a MVC 5 application with SQL server db using Entity Framework - this particular logic is contained in my Web API controller.

The following is my code for the API method:

 public HttpResponseMessage SaveCarModelSelect(CarModelSelectViewModel model)
 {
 var car = _carService.GetById(model.carId);
 var splitSelectedCars = model.SelectedCars.Split(',').Select(sValue => sValue.Trim()).ToList();
 // We only need to check if there are Car Models saved in the DB
 if (car.CarModels.Count > 0)
 {
 var carsToRemove = CarModelsToRemoveFromDb(car, splitSelectedCars);
 foreach (var carToRemove in carsToRemove )
 {
 var carIdToDelete = car.RemoveCar(carToRemove);
 _carService.Delete(carIdToDelete);
 }
 }
 foreach (var singleSelectedCar in splitSelectedCars)
 {
 var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
 .ToList();
 if (!savedCarsInDb.Contains(singleSelectedCar))
 {
 riskAppraisal.AddCar(singleSelectedCar);
 }
 }
 _carService.Update(car);
 return Request.CreateResponse(HttpStatusCode.OK);
 }

The private method for CarModelsToRemoveFromDb is below

 private static IEnumerable<string> CarModelsToRemoveFromDb (Car car, IEnumerable<string> splitSelectedCars)
 {
 var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
 .ToList();
 return savedCarsInDb.Except(splitSelectedCars, StringComparer.OrdinalIgnoreCase);
 }

I'm guessing has anyone got suggestions to improve this - all the functionality is working as expected. Where I call the AddCar method I suppose the checking database logic could be wrapped in another if the .Count is> 0 but then if I had the AddCar(singleSelectedCar) outside that it would get added twice. The Goal I am trying to get to is make my API method thin and separate out other logic into their own methods.

Any suggested improvements appreciated.

Answer 1

To clean up your controller I came up with this:

• entire logic is moved to your CarService
• method CarModelsToRemoveFromDb is also moved to CarService
• CarServicehas two overloads for method Update

Methods for CarService:

private IEnumerable<string> CarModelsToRemoveFromDb (Car car, IEnumerable<string> splitSelectedCars)
{
 var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
 .ToList();
 return savedCarsInDb.Except(splitSelectedCars, StringComparer.OrdinalIgnoreCase);
}
public void Update(CarModelSelectViewModel model)
{
 var car = GetById(model.carId);
 var splitSelectedCars = model.SelectedCars.Split(',').Select(sValue => sValue.Trim()).ToList();
 if (car.CarModels.Count > 0)
 {
 var carsToRemove = CarModelsToRemoveFromDb(car, splitSelectedCars);
 foreach (var carToRemove in carsToRemove )
 {
 var carIdToDelete = car.RemoveCar(carToRemove);
 Delete(carIdToDelete);
 }
 }
 foreach (var singleSelectedCar in splitSelectedCars)
 {
 var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
 .ToList();
 if (!savedCarsInDb.Contains(singleSelectedCar))
 {
 riskAppraisal.AddCar(singleSelectedCar);
 }
 }
 Update(car);
}
public void Update(Car car)
{
 //your current logic
}

Controller:

public HttpResponseMessage SaveCarModelSelect(CarModelSelectViewModel model)
{
 _carService.Update(model);
 return Request.CreateResponse(HttpStatusCode.OK);
}

Logic that checks count of Car (car.CarModels.Count > 0) can also be separated out to private method, same for loop foreach (var singleSelectedCar in splitSelectedCars).

I hope it can give you some sense of direction.

• Ctrl_Alt_Defeat: This looks not bad...the only reason I wouldn't move all this logic to service layer is because I don't want to have a web layer view model in my service layer. I suppose I could define another class at web layer which does your refactoring