title: Can this approach to code separation in MVC be improved?
body: <p>Firstly I am not a professional programmer and I am just learning C#, MVC and Web Development.</p>
  
  <p>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 <a href="http://www.mikesdotnetting.com/article/263/migrating-from-razor-web-pages-to-asp-net-mvc-5-data-access-and-view-models" rel="nofollow">Mike Brind's recent blog post</a></p>
  
  <p><strong><em>Question:</em></strong>
  <em>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 cant 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.</em></p>
  
  <h2>My Code Snippets</h2>
  
  <p><strong>VehicleWorksRequest Model</strong></p>
  
  <p>I start with a Model that reflects the entity contained in the Database. </p>
  
  <pre><code>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;
          }
      }
  }
  </code></pre>
  
  <p><strong>View Model for Home View</strong>
  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</p>
  
  <ul>
  <li>A list of all Critical Work Requests</li>
  <li>A list of all Routine Work Requests </li>
  <li>A list of all Completed Work Requests</li>
  <li>some other measures showing a count of work requests etc.</li>
  </ul>
  
  <p>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.</p>
  
  <p><em>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.</em></p>
  
  <pre><code>using System.Collections.Generic;
  
  namespace ATAS.Models.ViewModels
      {
      public class HomeViewModel
          {
          public List&lt;Vehicle&gt; Vehicles { get; set; }
          public int VehicleCount { get; set; }
          public List&lt;VehicleWorkRequest&gt; VehicleWorkRequests { get; set; }
          public List&lt;VehicleWorkRequest&gt; VehicleCriticalWorkRequests { get; set; }
          public List&lt;VehicleWorkRequest&gt; VehicleRoutineWorkRequests { get; set; }
          public List&lt;VehicleWorkRequest&gt; VehicleCompletedWorkRequests { get; set; }
          public int VehicleWorkRequestCount { get; set; }
  
          }
      }
  </code></pre>
  
  <p><strong>Service Layer</strong>
  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.
  <em>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.</em></p>
  
  <pre><code>using ATAS.Data;
  using ATAS.Models;
  using System.Collections.Generic;
  using System.Linq;
  
  namespace ATAS.Services
  {
  
      public class VehicleService : IVehicleService
      {
          public List&lt;Vehicle&gt; 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&lt;VehicleWorkRequest&gt; 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&lt;VehicleWorkRequest&gt; GetAllVehiclesCriticalWorkRequests()
          {
              using (DataManager db = new DataManager())
              {
  
                  var Requests = from vwr in db.VehicleWorkRequest
                                 where vwr.Closed == false &amp;&amp; vwr.Critical == true 
                                 orderby vwr.DateReported ascending
                                 select vwr;
  
                  return Requests.ToList();
              }
          }
  
      public List&lt;VehicleWorkRequest&gt; GetAllVehiclesRoutineWorkRequests()
          {
              using (DataManager db = new DataManager())
              {
  
                  var Requests = from vwr in db.VehicleWorkRequest
                                 where vwr.Closed == false &amp;&amp; vwr.Critical == false
  
                                 orderby vwr.DateReported ascending
                                 select vwr;
  
                  return Requests.ToList();
              }
          }
  
       public List&lt;VehicleWorkRequest&gt; 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);
              }
          }
      }
  }
  </code></pre>
  
  <p><strong>Service Layer Interface</strong>
  To promote code separation I have created an Interface to the Service Layer. </p>
  
  <pre><code>using ATAS.Models;
  using System.Collections.Generic;
  namespace ATAS.Services
  {
  public interface IVehicleService
      {
      List&lt;Vehicle&gt; GetVehicles();
      Vehicle GetVehicle(int id);
      VehicleWorkRequest GetIndividualVehiclesWorkRequests(int id);
      List&lt;VehicleWorkRequest&gt; GetAllVehiclesWorkRequests();
      List&lt;VehicleWorkRequest&gt; GetAllVehiclesCriticalWorkRequests();
      List&lt;VehicleWorkRequest&gt; GetAllVehiclesRoutineWorkRequests();
      List&lt;VehicleWorkRequest&gt; GetAllVehiclesCompletedWorkRequests();
      }
  }
  </code></pre>
  
  <p><strong>Finally the controller</strong>
  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.</p>
  
  <pre><code>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);
          }
      }
  }
  </code></pre>