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.