0

This is a simple scenario in an office with employees and their manager.

In this scenario, both managers and employees have a name attribute.

  • Only the manager can change his own name
  • Only the employee can change his own name

Only the employee has a designation and a workStat attribute. The work status indicates whether the employee is still working (true) or not working (false).

A manager can fire any selected employee using the Manager.fireEmployee() method.

For the above scenario, I created the following class diagram:

Manager/Employee Model

I implemented the above scenario using Java as shown below. (Orchestration code is not included)

class Manager{

    private String name;    

    //create Manager object by providing name
    Manager(String managerName ){           
        this.name        = managerName;
    }

    //only manager can change his name
    private void changeName(String managerName){
        this.name = managerName;
    }


    //manager can fire any selected Employee
    public void fireEmployee(Employee emp){
        emp.changeWorkStat(false);            
    }  


}

/* --------------------------- */


class Employee{
    private String name;
    private String designation;
    private boolean workStat;

    Employee(String name ,String designation ){
        this.designation     = designation;
        this.name            = name;
        this.workStat        = true;
    }

    //only Employee can change his name 
    private void changeName(String empName){
        this.name = empName;
    }

    //to fire the employee manager can execute this method
    public void changeWorkStat(boolean stat){
        this.workStat = stat;
    }

}

But i have doubts. Is it good OOAD practice to include the fireEmployee() method inside the manager class? Because this method is basically just executing the method of another class. Also, this method is not responsible for managing the state of the Manager class (doing nothing to attributes in the Manager class, not using the attributes in the Manager class). Therefore, it is a sign of violation of encapsulation.

If this is not good OOAD practice, then what is the correct way of implement this scenario in code?

3

Your feeling is correct. This is not a good design.

It appears to me that this is an attempt to 'model the real world' with objects: a manager fires an employee, therefore the fireEmployee method must be part of Manager.

What your model should really reflect, however, are the invariants of your business domain. For example, an employee can only be fired by a manager - but what's to keep me from calling Employee.changeWorkStat directly?

I don't know your requirements, but the whole employment management should probably be a class of its own.

class Manager { /* ... personal data ... */ }
class Employee { /* ... personal data ... */ }

class StaffRegistry
{
    void hire(Manager supervisor, Employee employee, Designation designation) { /* ... */ }
    void fire(Manager supervisor, Employee employee) { /* ... */ }
    boolean isEmployed(Employee employee) { /* ... */ }
    // ...
}

The hireand fire methods allow you to enforce whatever invariants you have related to managing your employees. For example: Is this really a manager in the company? Is the employee already hired/fired? Etc.

There are many ways in which you might want to improve on this design. The point is that by extracting this logic into a separate module, you not only gain the advantages of a decoupled design, you can also clearly express your invariants.

  • In the real world, a manager doesn't fire an employee. – gnasher729 Apr 28 at 20:33
  • @gnasher729 What? From OP: "A manager can fire any selected employee [...]". – doubleYou Apr 29 at 8:49
2

Having a method to just execute one method on another object is not wrong per se. It is perfectly fine if the semantics of both methods is different or the methods are on a different abstraction level. For example calling fire() just calls record.delete() to remove from database or something.

In your example it is questionable, since both methods should have the same semantics (i.e. mean the same thing). First, I would fix the method names a bit, changeWorkStat() is difficult to understand. Remember, your public identifiers should be derived directly from the business domain. You should have Employee.fire(). Yes, the employee fires itself. Why? Because you modeled this responsibility into the Employee, which is fine if it fits your requirements.

That only a manager can fire employees is more of an authorization problem and I wouldn't try to express that in the model I think, but as always, depends on your exact requirements.

0

Good question.

  1. It would make sense to keep the fireEmployee(Employee emp) method inside the Manager class only if Manager maintains a list of employees reporting to him and is allowed to fire only those employees... in which case the emp passed as the parameter to this method would have to be one of the employees reporting to him. This way, when an employee is fired we would also need to update the list of employees reporting to that manager which would amount to a change in the state of the Manager object, and hence it would make perfect sense for the fireEmployee method to be a part of the Manager class, as per OOP principles.
  2. You could instead keep this method directly inside the Employee class with a ManagerId as method-parameter, i.e. fireEmployee(string managerId)... Let the Employee class also have a property named empManagerId, which would store the ManagerId of his manager. When the method fireEmployee is called, the client would supply the manager-id of the manager responsible for firing him. This method of the Employee class would then validate that the managerId passed as a parameter should be the same as the id of his manager i.e. empManagerId

Hope this helps.

0

Classes must have single responsability but that doesn't mean that can,t interact with other classes, so method that doesn't change internal state is good. In this case the responsability of manager is to manage employees so is ok to change workstatus of employee. But depending on all the business logic you may need a third class to manage this interactions, like a controllers or services.

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.