HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Keeping data processing out of controllers

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
keepingcontrollersprocessingdataout

Problem

I would like to start by mentioning that I am LAMP stack guy who happens to be making my first ever .NET C# web app and I'm seeking general advises for best practices and to see how more experienced people would have done it.

I am using code first development pattern with latest Entity Framework . I have a AppUser model. The user could be on of few types, one of them is employee.

public class AppUser{
  public int Id { get; set; }
  public UserType Type { get; set; }
  public Title Title { get; set; }
  public string FirstName { get; set; }
  public string MiddleName { get; set; }
  public string LastName { get; set; }
  public virtual AddressDetails AddressDetails { get; set; }
  public virtual Company Company { get; set; }
  ..................................................
}

public class AddressDetails
{
  public int Id { get; set; }
  public string AddressLine { get; set; }
  public string City { get; set; }
  public string County { get; set; }
  public string PostCode { get; set; }
  public float Latitude { get; set; }
  public float Longtitude { get; set; }
  [JsonIgnore]
  public virtual AppUser User { get; set; }
}


I believe that smaller methods are the better. So, I am trying to keep controllers as small as possible. I made classes which handles all interactions with database and keeps business logic out of Models and Controllers. First class is abstract MasterRepository ,which has logic for Database Update/Add/SaveChanges. All other "repositories" inherit it.
( "AppDB" is inheriting from DbContext and holds all DbSets)

```
public abstract class MasterRep
{
protected AppDB _db { get; set; }
protected AppUser _currentUser { get; set; }

protected int _companyId { get; set; }
protected string _applicationUserId { get; set; }
public MasterRep(string applicationUserId, AppDB db = null)
{
_db = db ?? new AppDB ();
_currentUser = GetUserByApplicationId(applicationUserId);
_companyId = _currentUser.Company.Id;
_appl

Solution

Your brace style is inconsistent. Allman style is usually applied in C#.

public class AppUser{
    // snipped
}

public class AddressDetails
{
    // snipped
}


Longtitude is a spelling error, it should be Longitude.

Underscores are usually only used for fields; properties should be PascalCase:

protected AppDB _db { get; set; }
  protected AppUser _currentUser { get; set; }


Don't abbreviate unnecessarily: I don't know what Rep is, I do know what Repository is.

public class EmployeeRep : UserRep


Is this a field or a property:

private EmployeeRep _employeeRep { get; set; }


The { get; set; } says "property", but the private and the underscore at the start of the name says "field".

Avoid "negative checks" like you do here:

_employeeRep = (User.Identity.GetUserId() != null) 
    ? new EmployeeRep(User.Identity.GetUserId()) 
    : null;


Invert that logic and things become a lot clearer:

_employeeRep = (User.Identity.GetUserId() == null) 
    ? null 
    : new EmployeeRep(User.Identity.GetUserId());


Also, since you call User.Identity.GetUserId() twice, consider calling it once, storing the result in a variable and then using that variable.

Code Snippets

public class AppUser{
    // snipped
}

public class AddressDetails
{
    // snipped
}
protected AppDB _db { get; set; }
  protected AppUser _currentUser { get; set; }
public class EmployeeRep : UserRep
private EmployeeRep _employeeRep { get; set; }
_employeeRep = (User.Identity.GetUserId() != null) 
    ? new EmployeeRep(User.Identity.GetUserId()) 
    : null;

Context

StackExchange Code Review Q#113388, answer score: 5

Revisions (0)

No revisions yet.