3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Ctrl_Alt_Defeat Jul 23 '14 at 7:41

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.