Code separation in MVC

Question

Firstly I am not a professional programmer and I am just learning C#, MVC and Web Development.

Much of what I describe here is self taught and comes from lots of googling and posts on Stack Overflow. I have adopted the approach outlined in Mike Brind's recent blog post

Can some of you with more experience than me critically look at my approach and let me know if there are better ways or improvements I can make? I can't help but think that there is a better way to implement the Service Layer as there does seem to be a lot of replicated code.

VehicleWorksRequest Model

I start with a Model that reflects the entity contained in the Database:

using System;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;
using System.Web;

namespace ATAS.Models
{
    public class VehicleWorkRequest : IControllerHooks
    {
        [Key]
        public int RequestId { get; set; }
        public bool Closed { get; set; }
        public bool Critical { get; set; }
        [Display(Name = "Fleet number")]
        public int? VehicleId { get; set; }
        [Display(Name = "Odometer Reading")]
        public int? OdometerReadingId { get; set; }
        [Display(Name = "Report Date")]
        public DateTime? DateReported { get; set; }
        [Display(Name = "Person reporting")]
        public int ReportingEmployeeId { get; set; }           
        [StringLength(255)]
        public string Request { get; set; }
        [StringLength(255)]
        public string Comment { get; set; }
        // don't update the database context with posted-back data 
        [PersistPropertyOnEdit(false)]
        // don't show the CreatedDate on any of the views 
        [ScaffoldColumn(false)]
        public DateTime? CreatedDate { get; set; }
        [Editable(false)]
        [DisplayOnEditView(true)]
        [DataType(DataType.Date)]
        public DateTime? ModifiedDate { get; set; }
        [Editable(false)]
        [DisplayOnEditView(true)]
        public string LastModifiedBy { get; set; }
        [ForeignKey("ReportingEmployeeId")]
        public virtual Employee ReportingEmployee { get; set; }
        public void OnCreate()
        {
            CreatedDate = DateTime.UtcNow;
            ModifiedDate = CreatedDate;
            LastModifiedBy = HttpContext.Current.User.Identity.Name;
            Closed = false;
            Critical = false;
        }
        public void OnEdit()
        {
            ModifiedDate = DateTime.UtcNow;
            LastModifiedBy = HttpContext.Current.User.Identity.Name;
        }
    }
}

View Model for Home View

The view model I understand, and as Mike points out, serves as a container for data for a view. My home page view (really a dashboard) features several pieces of information that is held in the VehicleWorkRequest database table

• A list of all Critical Work Requests
• A list of all Routine Work Requests
• A list of all Completed Work Requests
• Some other measures showing a count of work requests etc.

Whilst I know it is possible to just pass the collection of work requests to the view and then to filter the various class of work request (Critical, Routine and Completed) there, but from what I have read and understood from Mikes post the recommended approach is to keep that kind of logic out of the view, which is why it has been done in the view model instead.

I think this may be one place I can improve things as all the Work Request Lists carry the same information just a different subset of it depending on its criticality and if its open.

using System.Collections.Generic;

namespace ATAS.Models.ViewModels
    {
    public class HomeViewModel
        {
        public List<Vehicle> Vehicles { get; set; }
        public int VehicleCount { get; set; }
        public List<VehicleWorkRequest> VehicleWorkRequests { get; set; }
        public List<VehicleWorkRequest> VehicleCriticalWorkRequests { get; set; }
        public List<VehicleWorkRequest> VehicleRoutineWorkRequests { get; set; }
        public List<VehicleWorkRequest> VehicleCompletedWorkRequests { get; set; }
        public int VehicleWorkRequestCount { get; set; }

        }
    }

Service Layer

This is responsible for talking to my data access layer (EF) and delivering data to the controller so that it can pass it on to the view. It will also ultimately accept data from my controller and do whatever it needs to with it. I cant help but think that I can refactor this better as there seems I have a lot of repeated code and database requests again for essentially the same information.

using ATAS.Data;
using ATAS.Models;
using System.Collections.Generic;
using System.Linq;

namespace ATAS.Services
{

    public class VehicleService : IVehicleService
    {
        public List<Vehicle> GetVehicles()
        {
            using (DataManager db = new DataManager())
            {

                var ActiveVehicles = from v in db.Vehicles
                                     where v.Active == true
                                     orderby v.FleetNumber ascending
                                     select v;

               return ActiveVehicles.ToList();
            }
        }

    public Vehicle GetVehicle(int id)
        {
            using (DataManager db = new DataManager())
            {
                return db.Vehicles.Find(id);
            }
        }

    public List<VehicleWorkRequest> GetAllVehiclesWorkRequests()
        {
            using (DataManager db = new DataManager())
            {

                var Requests = from vwr in db.VehicleWorkRequest
                               where vwr.Closed == false
                               orderby vwr.DateReported ascending
                               select vwr;

                return Requests.ToList();
            }
        }

    public List<VehicleWorkRequest> GetAllVehiclesCriticalWorkRequests()
        {
            using (DataManager db = new DataManager())
            {

                var Requests = from vwr in db.VehicleWorkRequest
                               where vwr.Closed == false && vwr.Critical == true 
                               orderby vwr.DateReported ascending
                               select vwr;

                return Requests.ToList();
            }
        }

    public List<VehicleWorkRequest> GetAllVehiclesRoutineWorkRequests()
        {
            using (DataManager db = new DataManager())
            {

                var Requests = from vwr in db.VehicleWorkRequest
                               where vwr.Closed == false && vwr.Critical == false

                               orderby vwr.DateReported ascending
                               select vwr;

                return Requests.ToList();
            }
        }

     public List<VehicleWorkRequest> GetAllVehiclesCompletedWorkRequests()
        {
            using (DataManager db = new DataManager())
            {

                var Requests = from vwr in db.VehicleWorkRequest
                               where vwr.Closed == true
                               orderby vwr.DateReported ascending
                               select vwr;

                return Requests.ToList();
            }
        }

    public VehicleWorkRequest GetIndividualVehiclesWorkRequests(int id)
        {
            using (DataManager db = new DataManager())
            {
                return db.VehicleWorkRequest.Find(id);
            }
        }
    }
}

Service Layer Interface

To promote code separation I have created an Interface to the Service Layer.

using ATAS.Models;
using System.Collections.Generic;
namespace ATAS.Services
{
public interface IVehicleService
    {
    List<Vehicle> GetVehicles();
    Vehicle GetVehicle(int id);
    VehicleWorkRequest GetIndividualVehiclesWorkRequests(int id);
    List<VehicleWorkRequest> GetAllVehiclesWorkRequests();
    List<VehicleWorkRequest> GetAllVehiclesCriticalWorkRequests();
    List<VehicleWorkRequest> GetAllVehiclesRoutineWorkRequests();
    List<VehicleWorkRequest> GetAllVehiclesCompletedWorkRequests();
    }
}

The controller

The first task of the index action is to instantiate the HomeViewModel. The data for the view model is provided by the Service Interface which in turn uses the classes in the Service layer to interact with the Data access Layer.

using System.Web.Mvc;
using ATAS.Data; 
using ATAS.Models.ViewModels;
using ATAS.Services;
namespace ATAS.Controllers
{
[Authorize]
public class HomeController : Controller
    {
    private DataManager db = new DataManager();

    // GET: home/index
    public ActionResult Index()
    {
        IVehicleService service = new VehicleService();
        HomeViewModel Model = new HomeViewModel
            {
                Vehicles = service.GetVehicles(),
                VehicleCount = service.GetVehicles().Count,
                VehicleWorkRequestCount = service.GetAllVehiclesWorkRequests().Count,
                VehicleWorkRequests = service.GetAllVehiclesWorkRequests(),
                VehicleCriticalWorkRequests = service.GetAllVehiclesCriticalWorkRequests(),
                VehicleRoutineWorkRequests = service.GetAllVehiclesRoutineWorkRequests(),
                VehicleCompletedWorkRequests = service.GetAllVehiclesCompletedWorkRequests()
            };
        return View(Model);
        }
    }
}

Answer 1

You can write a generic method in service for getting data to reducing duplication by sending Expression<Func> as search parameter to the GetVehicle method.

public IEnumerable<Vehicle> GetVehicles(Expression<Func<Vehicle, bool>> predicate = null)
{
    using (DataManager db = new DataManager())
    {
        if (predicate == null)
            return db.Vehicles.ToList();

        var vehicles = db.Vehicles.Where(predicate).ToList();
        return vehicles;
    }
}

public ActionResult Index()
{
    IVehicleService service = new VehicleService();
    var allVehicles = service.GetVehicles();
    var vehicleWorkRequest = service.GetVehicles(v => v.Closed == false);
    var vehicleCriticalWorkRequests = service.GetVehicles(v => v.Closed == false && v.Critical == true).OrderByDescending(v => v.DateReported);
    var vehicleRoutineWorkRequests= service.GetVehicles(v => v.Closed == false && v.Critical == false).OrderByDescending(v => v.DateReported);
    var vehicleCompletedWorkRequests = service.GetVehicles(v => v.Closed == true).OrderByDescending(v => v.DateReported);

    var Model = new HomeViewModel
        {
            Vehicles = allVehicles,
            VehicleCount = allVehicles.Count(),
            VehicleWorkRequestCount = vehicleWorkRequest.Count(),
            VehicleWorkRequests = vehicleWorkRequest,
            VehicleCriticalWorkRequests = ehicleCriticalWorkRequests,
            VehicleRoutineWorkRequests = vehicleRoutineWorkRequests,
            VehicleCompletedWorkRequests = vehicleCompletedWorkRequests
        };
    return View(Model);
    }
}

You can also make it async for better performance:

public async Task<IEnumerable<Vehicle>> GetVehiclesAsync(Expression<Func<Vehicle, bool>> predicate = null)
{
    using (DataManager db = new DataManager())
    {
        if (predicate == null)
            return await db.Vehicles.ToListAsync();

        return await db.Vehicles.Where(predicate).ToListAsync();
    }
}


public async Task<ActionResult> Index()
{
    IVehicleService service = new VehicleService();
    var allVehicles = await service.GetVehiclesAsync();
    var vehicleWorkRequest = await service.GetVehiclesAsync(v => v.Closed == false);
    var vehicleCriticalWorkRequests = await service.GetVehiclesAsync(v => v.Closed == false && v.Critical == true).OrderByDescending(v => v.DateReported);
    var vehicleRoutineWorkRequests= await service.GetVehiclesAsync(v => v.Closed == false && v.Critical == false).OrderByDescending(v => v.DateReported);
    var vehicleCompletedWorkRequests = await service.GetVehiclesAsync(v => v.Closed == true).OrderByDescending(v => v.DateReported);

    var Model = new HomeViewModel
        {
            Vehicles = allVehicles,
            VehicleCount = allVehicles.Count(),
            VehicleWorkRequestCount = vehicleWorkRequest.Count(),
            VehicleWorkRequests = vehicleWorkRequest,
            VehicleCriticalWorkRequests = ehicleCriticalWorkRequests,
            VehicleRoutineWorkRequests = vehicleRoutineWorkRequests,
            VehicleCompletedWorkRequests = vehicleCompletedWorkRequests
        };
    return View(Model);
    }
}

Answer 2

Some suggestions:

• Introduce repository layer under service for database access. Service layer should not have database access. Service mainly contains business logic. Then you can use Generic Repository Pattern to shorten your repository code
• Service layer will be a thin layer just pass data from repository to controller.
• As your app is simply retrieving dataset and its counts, it is going to be some repeated codes for different entities. I don't recommend using generic method on service layer for code visibility and maintainability reason. The method name should reflect what it does as much as possible. (Code tells what it does, Comment tells why).