4
\$\begingroup\$

Business rules: This application is for internal employees to request supplies. All requests must be approved my the persons manager. In the database, a manager is an employee, with a self referencing key joining an employees managerid to the managers employeeid.

The database does not pull employee records from anywhere, it is all self contained, so it will be managing the employee/manager relationship. This controller needs to handle various scenarios, such as:

  • Existing employee, existing manager
  • Existing Employee, New manager (manager does not exist as an employee)
  • New Employee, existing Manager (the employee is not in the DB but the manager is)
  • New employee, new manager
  • Existing Employee, Existing Manager
  • Existing Employee, Existing Manager - but employee has changed managers

This things works fine, but it is wicked messy. I am using a repository to hold crud functions. I have not included the repo here; let me know if you need it.

 [HttpPost]
    public ActionResult Create(EmployeeDTO model)
    {
        if(ModelState.IsValid)
        {
            int managerId = 0;  // this is the employee id of the manager passed in from the view
            int existingManagerId = 0;  // this is the current managerid from this employee, found int he db.

            model = repo.FormatManagerName(model);

            // Search for employee
            var employee = repo.GetEmployeeId(model.FirstName, model.LastName);
            // Search for manager
            var manager = repo.GetEmployeeId(model.ManagerFirst, model.ManagerLast);

            if(manager != null)
            {
                managerId = manager.EmployeeId;
            }

            if (employee != null)
            {
                model.EmployeeId = employee.EmployeeId;
            }

            // Get currently stored managerid for thsi employee
            existingManagerId = employee == null ? 0 : repo.GetManagerId(employee.EmployeeId);  // Set existingManagerid to either 0 or managers employeeid, depending on if employee exists..

            // If Manager does not exist, add and return employeeId
            if (manager == null)
            {
                try
                {
                    // If the manager does not exist, add manager as a new employee
                    EmployeeDTO newManager = new EmployeeDTO()
                    {
                        FirstName = model.ManagerFirst,
                        LastName = model.ManagerLast,
                        EmailAddress = model.ManagerEmail,
                        IsManager = true
                    };

                    managerId = repo.AddEmployee(newManager);
                }
                catch (Exception ex)
                {
                    Danger("Hmmm that didn't quite work.  Please make sure all the fields are filled in and resubmit.  If no luck, send us a help ticket.");
                    ErrorSignal.FromCurrentContext().Raise(ex);
                    return View("Error");
                }
            }
            // If Employee does not exist, Add and add managerId as FK.
            if (employee == null)
            {
                if(managerId > 0)  // If employee is new, but manager record exists, set new employees managerid.
                {
                    model.ManagerId = existingManagerId = managerId;
                }

                try
                {
                    model.EmployeeId = repo.AddEmployee(model);
                }
                catch (Exception ex)
                {
                    Danger("Hmmm that didn't quite work.  Please make sure all the fields are filled in and resubmit.  If no luck, send us a help ticket.");
                    ErrorSignal.FromCurrentContext().Raise(ex);
                    return View("Error");
                }
            }

            if (managerId != existingManagerId)
            {
                model.EmployeeId = employee.EmployeeId;
                model.ManagerId = managerId;
                model.CreatedDate = employee.CreatedDate;
                model.CreatedBy = employee.CreatedBy;
                repo.UpdateEmployee(model);
            }

            try
            {
                // Create a new request object, then proceed through the request controller
                RequestDTO request = new RequestDTO() { EmployeeId = model.EmployeeId };
                return RedirectToAction("Create", "Request", request);
            }
            catch (Exception ex)
            {
                Danger("Well that's just crazy cakes!.  I was able to save your employee information, but I can't create a new request.  Please submit a help ticket");
                ErrorSignal.FromCurrentContext().Raise(ex);
                return View();
            }
        }
        Danger("That didn't quite work.  Please make sure all fields are filled in. If you are a Manager or Director, you can leave the Manger Name and Email fields empty, but make sure you click the Manager/Director check box.");
        return View(model);

    }
\$\endgroup\$
4
\$\begingroup\$

The Overall Theme

Pushing details down into classes where they belong and making methods responsible for doing their jobs very significantly reduces client code clutter and makes object use simpler, cleaner, and reusable.

large if block is hurting code comprehension

Handle the outliers, the exceptional cases up front. I find this really helps eliminate unnecessary control structure.

if(!ModelState.IsValid) {
     Danger("That didn't quite work.  Please make sure all fields are filled in. If you are a Manager or Director, you can leave the Manger Name and Email fields empty, but make sure you click the Manager/Director check box.");
    return View(model);
}

// the rest of the code

Duplicate object data

In the database, a manager is an employee, with a self referencing key joining an employees managerid to the managers employeeid.

You have managerId and employeeId as local variables. Are these not already part of the employee class? You will find in the long run that it is a mistake to gut object properties and "lay them out on the table." You are corrupting model integrity. You now have to keep redundant properties in sync and for each layer (M.V.C.), that's 6x the work per property. The whole thing is very un-Object Oriented.

Make your methods do their jobs

GetManagerId() should know how to handle a null employee. Its a cruel world and methods must not rely on the kindness of strangers to pass valid arguments. As is the method is not at all reusable. Every client will have to know 1) don't pass in a null 2)I'm supposed to get a zero in this case. You'll re-write the exceptional handling over and over and ....

That's like the customer - every customer every time - having to tell the restaurant staff where they keep the french fries.

So, not this:

existingManagerId = employee == null ? 0 : repo.GetManagerId(employee.EmployeeId);  // Set existingManagerid to either 0 or managers employeeid, depending on if employee exists..

Instead, this:

existingManageId = GetManagerId(employee);

///<summary>
/// Returns zero if employee is null or no manager assigned
/// </summary>
public int GetManagerId(Employee theEmployee) { 
    return theEmployee == null ? 0 : repo.GetManagerId(employee);
}
// if we had fetched an employee object in the first place we'd already have the manager id.

Make objects keep their own information

public class Employee {
    ///<summary>
    /// Zero means no manager is assigned
    /// Defaults to zero.
    ///</summary>
    public int ManagerId { get; protected set; }
}

The definition of "I have no manager" (e.g. a zero) is in the Employee class where it belongs. Every new employee automatically has no manager by definition because it's build into the DNA of the class.

Who's your Boss?

Warning! Confusing and ambiguous code ahead!

int managerId = 0;
int existingManagerId = 0;

Hope! Object Oriented disambiguation

bossyDude.ManagerId;
bob.ManagerId;

Null Object

Will make the following annoying code go away

if(manager != null)
{
    managerId = manager.EmployeeId;
}

if (employee != null)
{
    model.EmployeeId = employee.EmployeeId;
}

If there is no existing employe create an Employee object with zero in the ID fields. So when you call

justFetchedEmployee.ManagerId
justFetchedEmployee.EmployeeId

The client gets zero and does not have to guard for null. Other methods and properties return default values, null, etc. whatever is appropriate for benign, "do nothing" behavior.

You might also have a Employee.IsNull(). The client should only have to ask if the object is "null" and should not have to know how it's "null", just that is is. Client code should not be forced to know intimate details to determine state. The object itself knows. If a class is not controlling, encapsulating, determining it's own state that class is not reusable.

Hiding Details

Along the same lines above, a client should ask if there is a manager, not determine it for itself.

Employee.HasManager() { return this.ManagerId == 0; }

Just because it's simple does not mean its OK to ignore proper encapsulation.

Parameterized Constructors == Happy objects

The constructor is the place where there is a chance for creating a valid object. Validate the parameters. Test for nulls, valid ranges, valid data combinations, etc. I control my state, not the client.

EmployeeDTO newManager = new EmployeeDTO(string firstName
                            , string lastName
                            , string emailAddress
                            , bool isAManager = false)

// Default parameters - gotta love em!

this.FirstName = firstName?? string.Empty; 
// now we won't throw exceptions calling methods on null strings

this.IsManager = isAManager;

// now we KNOW what our initial state is. For example I won't have to 
//  "if (this.FirstName == null)" all over the class!
\$\endgroup\$
2
\$\begingroup\$

1. Skinny controllers - many advocate that controllers should be skinny and have most of the logic elsewhere (I prefer the services for doing that). Next valid question - since controllers are skinny and need view models, how do I obtain them? Great answers are here.

One suggested approach is to obtain view models from services. It is not orthodox, since view models are built having UI in mind, but it is somehow practical. Not returning view models means an extra mapping step outside of the services.

One way to model it:

// can be injected if using a DI and property injection. e.g. for Ninject (one of the most popular libraries)
// [Ninject]
public IEmployeeService EmployeeService { get; set; }

[HttpPost]
public ActionResult Create(EmployeeDTO model)
{
    var empVm = EmployeeService.PersistEmployee(model);
    return View(empVm);
}

// interfaces are used for injection and contain only exposed properties and methods of the service    
public class EmployeeService : IEmployeeService 
{   
    // gets an employee based on its identifier
    public EmployeeDTO GetEmployee(int employeeId)
    {

    }

    public EmployeeDTO PersistEmployee(EmployeeDTO inputModel)
    {
        // persistence logic comes here
    }
}

Another advantage of this approach is automatic testing friendliness (e.g. used in Test Driven Development) since all the business logic (and some of the view logic in this case) will be isolated and easier to test. Also, services can be reused in other context (e.g. asynchronous jobs implemented as services that have nothing to do with MVC framework).

2. No manager value

If this can be changed, having zero integer as no manager might lead to trouble. Zero is a valid integer value and I saw cases when, by mistake, a zero identifier was generated in records. SQL has a special token for "undefined" values: NULL. If an employee does not have a manager, it is NULL.

Of course, this required a nullable type (i.e. int? managerId).

3. Functionality split

Split different things to avoid clutter and long methods. E.g. Persisting new manager (this code belongs to the service)

// returns newly created manager id or null on failure
private int? createNewManager(EmployeeDTO inputModel)
{
    try
    {
        // If the manager does not exist, add manager as a new employee
        EmployeeDTO newManager = new EmployeeDTO()
        {
            FirstName = model.ManagerFirst,
            LastName = model.ManagerLast,
            EmailAddress = model.ManagerEmail,
            IsManager = true
        };

        managerId = repo.AddEmployee(newManager);
        return managerId;
    }
    catch (Exception ex)
    {
        Danger("Hmmm that didn't quite work.  Please make sure all the fields are filled in and resubmit.  If no luck, send us a help ticket.");
        ErrorSignal.FromCurrentContext().Raise(ex);
        return null;
    }
}

Then, your persistence logic code can reuse it:

    // If Manager does not exist, add and return employeeId
    if (manager == null)
    {    
        managerId = createNewManager(model);
        if (managerId == null) 
            return View("Error");
    }

3. Possible error prone functionality

The following code identifies employee and manager by their first and last names. If the user has misspelled a name, added a blank or it just happens to have a full name collision in the organization, you are in trouble, as incorrect new employees might get generated or even worse - create links to incorrect person.

// Search for employee
var employee = repo.GetEmployeeId(model.FirstName, model.LastName);
// Search for manager
var manager = repo.GetEmployeeId(model.ManagerFirst, model.ManagerLast);

4. Null condition

If nullable ids would be used (int? instead of int), null conditional could be used to simplify some constructs (this is available in C# 6 in Visual Studio 2015).

int managerId = 0; 
if(manager != null)
{
    managerId = manager.EmployeeId;
}

becomes only

int? managerId = manager?.EmployeeId;

also, existing manager id assignment is simplified.

existingManagerId = repo.GetManagerId(employee?.EmployeeId);

Although null value requires testing for null, with coalesce operator (??) and null conditional this is quite simple now.

5. DTOs for "binding"

I have just realized that you seem to use your DTOs to get and pass information to the view. While this seems convenient, it may lead to trouble as DTOs have problems serializing (they may hold navigation properties and other references that you do not want to get to the view).

Solution: define view models and map data DTO <-> ViewModel

6. Automatic mapping

This is used in conjuction to point 5 to avoid manually writing the assignments. One of the most popular libraries is [AutoMapper][4]. 

model.EmployeeId = employee.EmployeeId;
model.ManagerId = managerId;
model.CreatedDate = employee.CreatedDate;
model.CreatedBy = employee.CreatedBy;

becomes something like:

// I have assumed using a view model. Otherwise it would be 
// Mapper.Map<EmployeeDTO, EmployeeDTO>();
// this is statically called one time (this construct is obsolete in the latest version)
Mapper.CreateMap<EmployeeViewModel, EmployeeDTO>();

// automatic map based on property name
Mapper.Map(model, employee);

6. Transactionality

Your code does not seem to handle persistence in a transactional manner:

// if manager does not exist
managerId = repo.AddEmployee(newManager);

// employee does not exist
model.EmployeeId = repo.AddEmployee(model);

// different manager
repo.UpdateEmployee(model);

If new manager is created successfully, but employee creation fails, is it ok?

One way to handle this is to have some sort of scoped data access or unit of work that ensures that all your operations are performed in a transaction:

public class UnitOfWork : IUnitOfWork
{
    private IDbContext _context;

    public IRepository<Employee> EmployeeRepository { get; private set; }

    // if DI is used (and it should), caller does not have to provide these arguments as they are injected
    public UnitOfWork(IDbContext context, 
        IRepository<Employee> employeeRepository)
    {
        _context = context;
        EmployeeRepository = employeeRepository;
    }

    public IRepository<T> GetRepository<T>()
        where T: class
    {
        Type thisType = this.GetType();
        foreach (var prop in thisType.GetProperties())
        {
            var propType = prop.PropertyType;

            if (!typeof(IRepository).IsAssignableFrom(propType))
                continue;

            var repoType = propType.GenericTypeArguments[0];
            if (repoType == typeof(T))
                return (IRepository<T>) prop.GetValue(this);
        }

        throw new ArgumentException(String.Format("No repository of type {0} found", typeof(T).FullName));
    }

    public void SaveChanges()
    {
        _context.SaveChanges();
    }

}

So, you instantiate a UnitOfContext, do your work with the repositories and call SaveChanges.

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

You got very good reviews of your code, but your basic business logic is flawed. I'm sure there's some user driving this requirement, but some times it's our job to push back and not let some cockamamie feature make the system unstable.

If the employee or manager is not found, then the user should be redirected to some place where they can be added, given they have sufficient permissions, otherwise they should be shown a meaningful error message. The way you've implemented this is going to lead to many, many, many erroneously entered employee/manager records that will later need to be scrubbed from the database.

Don't let your product owner destabilize the system because they're lazy.

\$\endgroup\$

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.