c# - Can this approach to code separation in MVC be improved? -

firstly not professional programmer , learning c#, mvc , web development.

much of describe here self taught , comes lots of googling , posts on stack overflow. have adopted approach outlined in mike brind's recent blog post

question: can of more experience me critically @ approach , let me know if there better ways or improvements can make? cant think there better way implement service layer there seem lot of replicated code.

my code snippets

vehicleworksrequest model

i start model reflects entity contained in 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 database context posted-back data          [persistpropertyonedit(false)]         // don't show createddate on of 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 home view view model understand, , mike points out, serves container data view. home page view (really dashboard) features several pieces of information held in vehicleworkrequest database table

• a list of critical work requests
• a list of routine work requests
• a list of completed work requests
• some other measures showing count of work requests etc.

whilst know possible pass collection of work requests view , filter various class of work request (critical, routine , completed) there, have read , understood mikes post recommended approach keep kind of logic out of view, why has been done in view model instead.

i think may 1 place can improve things work request lists carry same information different subset of depending on criticality , if 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 responsible talking data access layer (ef) , delivering data controller can pass on view. accept data controller , whatever needs it. i cant think can refactor better there seems have lot of repeated code , database requests again 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 = v in db.vehicles                                      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 = vwr in db.vehicleworkrequest                                vwr.closed == false                                orderby vwr.datereported ascending                                select vwr;                  return requests.tolist();             }         }      public list<vehicleworkrequest> getallvehiclescriticalworkrequests()         {             using (datamanager db = new datamanager())             {                  var requests = vwr in db.vehicleworkrequest                                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 = vwr in db.vehicleworkrequest                                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 = vwr in db.vehicleworkrequest                                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 promote code separation have created interface 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();     } } 

finally controller first task of index action instantiate homeviewmodel. data view model provided service interface in turn uses classes in service layer interact 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);         }     } } 

you can write generic method in service getting data reducing duplication sending expression<func> search parameter 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 make async 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);     } }