Reduce logic in Web API controller 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 =&gt; sValue.Trim()).ToList();

        // We only need to check if there are Car Models saved in the DB
        if (car.CarModels.Count &gt; 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=&gt; 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&lt;string&gt; CarModelsToRemoveFromDb (Car car, IEnumerable&lt;string&gt; splitSelectedCars)
    {
        var savedCarsInDb = car.CarModels.Select(c=&gt; 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.
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&lt;string&gt; CarModelsToRemoveFromDb (Car car, IEnumerable&lt;string&gt; splitSelectedCars)
{
    var savedCarsInDb = car.CarModels.Select(c=&gt; 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 =&gt; sValue.Trim()).ToList();

    if (car.CarModels.Count &gt; 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=&gt; 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 &gt; 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.