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?

Improve this question

4 Answers 4

Reset to default
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.

Improve this answer
2
  • In the real world, a manager doesn't fire an employee. Commented Apr 28, 2019 at 20:33
  • @gnasher729 What? From OP: "A manager can fire any selected employee [...]". Commented Apr 29, 2019 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.

Improve this answer
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.

Improve this answer
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.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.