Allowing internal employees to request supplies

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 t

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);

///
/// Returns zero if employee is null or no manager assigned
/// 
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 {
    ///
    /// Zero means no manager is assigned
    /// Defaults to zero.
    ///
    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!

StackExchange Code Review Q#120569, answer score: 4