kamranahmedse / design-patterns-for-humans

An ultra-simplified explanation to design patterns
45.53k stars 5.31k forks source link

Wrong Composite Pattern Definition #28

Open ozen opened 7 years ago

ozen commented 7 years ago

First of all, thanks to the contributors for their work, there are really good simple explanations. But, the definition of Composite Pattern is completely wrong. What is explained is simply a usage of inheritance. The motivation for using composite pattern is not to treat group of objects in the same way; but treat (1) group of objects with (2) a single object in the same way.

Keeping up with the Employee example; assume there is an Employee class; and a Team class that consists of a group of Employees and other Teams. You define a super type for Employee and Team, let's call it Workforce, and make Team consist of Workforce. This is well explained in Wikipedia article [1]. Matching the terminology with it, Employee is Leaf, Team is Composite, and Workforce is Component. Here Team is the client that make use of the Component.

kevinquinnyo commented 7 years ago

When I read the Composite section the other day I thought it looked more like a simple Registry pattern, but I might be wrong.

PieterScheffers commented 7 years ago

Yeah I thought the composite pattern also was not what I learned in school;)

Would this be a better example?

interface Salary {
    public function getSalary() : float;
}

class Employee implements Salary {

    protected $salary;
    protected $name;

    public function __construct(string $name, float $salary) {
        $this->name = $name;
        $this->salary = $salary;
    }

    public function setSalary(float $salary) {
        $this->salary = $salary;
    }

    public function getSalary() : float {
        return $this->salary;
    }
}

class Division implements Salary {

    protected $employees = [];
    protected $divisions = [];

    public function __construct(array $employees = [], array $divisions = []) {
        $this->employees = $employees;
        $this->divisions = $divisions;
    }

    public function getSalary() : float {
        $total = 0;

        foreach ($this->employees as $employee) {
            $total += $employee->getSalary();
        }

        foreach ($this->divisions as $key => $division) {
            $total += $division->getSalary();
        }

        return $total;
    }
}

You can then have a tree like structure with divisions which have subdivisions and divisions which hold employees or both.

$dave = new Employee('Dave', 25.95);
$sarah = new Employee('Sarah', 22.34);
$jake = new Employee('Jake', 34.95);
$lucius = new Employee('Lucius', 11.22);

$development = new Division([ $dave, $jake ]);
$support = new Division([ $lucius ]);

$it = new Division([ $sarah ], [ $development, $support ]);

// To get the salary of all of it
$it->getSalary();

// to get only the salary of development
$development->getSalary();

// to get salary of lucius
$lucius->getSalary();
Furgas commented 7 years ago

I too think that the example is not exactly as in definition. My shot at this:

interface HumanResource {
    public function getName() : string;
    public function setSalary(float $salary);
    public function getSalary() : float;
    public function getRoles()  : array;
}

class Developer implements HumanResource {

    protected $salary;
    protected $name;
    protected $roles;

    public function __construct(string $name, float $salary) {
        $this->name = $name;
        $this->salary = $salary;
        $this->roles = ["developer"];
    }

    public function getName() : string {
        return $this->name;
    }

    public function setSalary(float $salary) {
        $this->salary = $salary;
    }

    public function getSalary() : float {
        return $this->salary;
    }

    public function getRoles() : array {
        return $this->roles;
    }
}

class Designer implements HumanResource {

    protected $salary;
    protected $name;
    protected $roles;

    public function __construct(string $name, float $salary) {
        $this->name = $name;
        $this->salary = $salary;
        $this->roles = ["designer"];
    }

    public function getName() : string {
        return $this->name;
    }

    public function setSalary(float $salary) {
        $this->salary = $salary;
    }

    public function getSalary() : float {
        return $this->salary;
    }

    public function getRoles() : array {
        return $this->roles;
    }
}

class Manager implements HumanResource {

    protected $salary;
    protected $name;
    protected $roles;

    public function __construct(string $name, float $salary) {
        $this->name = $name;
        $this->salary = $salary;
        $this->roles = ["manager"];
    }

    public function getName() : string {
        return $this->name;
    }

    public function setSalary(float $salary) {
        $this->salary = $salary;
    }

    public function getSalary() : float {
        return $this->salary;
    }

    public function getRoles() : array {
        return $this->roles;
    }
}

class Department implements HumanResource {

    protected $name;
    protected $human_resources;

    public function __construct(string $name, array $human_resources = []) {
        $this->name;
        $this->human_resources = $human_resources;
    }

    public function addHumanResource(HumanResource $human_resource) {
        $this->human_resources[] = $human_resource;
    }

    public function getName(): string {
        return $this->name;
    }

    public function setSalary(float $salary) {
        foreach ($this->human_resources as $human_resource) {
            $human_resource->setSalary($salary);
        }
    }

    public function getSalary(): float {
        $salarySum = 0;

        foreach ($this->human_resources as $human_resource) {
            $salarySum += $human_resource->getSalary();
        }

        return $salarySum;
    }

    public function getRoles(): array {
        $roles = [];

        foreach ($this->human_resources as $human_resource) {
            $roles = array_merge($roles, $human_resource->getRoles());
        }

        $roles = array_unique($roles);

        return $roles;
    }
}

// Prepare the employees
$john = new Developer('John Doe', 12000);
$jane = new Designer('Jane', 10000);
$joe = new Manager('Jane', 50000);

// Create a department
$important_department = new Department("Gets The Work Done Department");
$important_department->addHumanResource($john);
$important_department->addHumanResource($jane);

// Create the head department
$head_department = new Department("Head Department");
$head_department->addHumanResource($important_department);
$head_department->addHumanResource($joe);

echo "Organization salaries: " . $head_department->getSalary() ."\n"; // Organization Salaries: 72000
echo "Organization roles: " . implode(', ', $head_department->getRoles()) ."\n"; // Organization roles: developer, designer, manager
Hyunk3l commented 7 years ago

@Furgas wouldn't be better that Developer, and the rest of ppl, would implement Employee (as in the original example)? And instead of Department, HumanResource (as employee type) would be the composite. Anyway, my 2 cents:

floer32 commented 7 years ago

Yeah, the current example in the README is not an example of Composite Pattern using Composition, but rather just Duck Typing. While Duck Typing is awesome and it's a great thing to demonstrate, and there are plenty of examples of Duck Typing and Composition complementing each other, they are not the same thing.

I also wanted to share some links about Composition demonstrated in Ruby. There are a lot of good examples because Composition is often preferred over Inheritance in Ruby.

seyfer commented 7 years ago

+1 I just entered issues list to see if there already issue about Composite. I was confused to see implementation in README. It's not a Composite.

@PieterScheffers and @Furgas have good examples. The main idea is that container class in composite should implement the same interface as implemented by leaf (just class, not container). Then we could build a complex structure, we could put leafs in the containers and containers in the containers and call method from an interface from the most top container and it will return some summarized value from ALL members of a structure (leafs and containers and their leafs).

Possible fix for example in README is to add Employee interface to an Organisation container and call $this methods inside in order to traverse all container members and return a result in getNetSalaries() method.

Even more. In this case, we don't need getNetSalaries method. We need just call getSalaries on a container and it should return sum of all members salaries.

pablomusumeci commented 7 years ago

Came here to report the same thing, composite definition is completely wrong. Both classes must implement the same interface, otherwise they couldn't be treated as the same thing.

kevinchar93 commented 7 years ago

Again I came here to report the same issue with the Composite pattern.

The current plain words definition: "Composite pattern lets clients treat the individual objects in a uniform manner."

should be changed to: "Composite pattern lets clients treat individual objects and groups (composition) of objects as if they were exactly the same. Clients do not know if they are operating on a group or an individual object."

The example should have an interface that individual classes and collection classes implement, the client can then operate on a single object or collection of objects without knowing this. The GoF example is actually the best I have seen that demonstrates it

shmulim commented 7 years ago

This example is incorrect. The Composite Pattern uses recursive composition to create tree-like structures. The composite class should to be subclassed to the component class and conform to its interface, as described by the GoF.

XaaXaaX commented 6 years ago

Sorry man , but i think after one year of commenting , you should try to modify your example , surely as the definition talks , the composite is to make the composite act as component with no care about the implementation , the base is to have a composite deriving or implementing the component and the leafs as well as . it's not far from decorator but with some more vast usage , ex. a project manager can manage all who works for the project , technical reference is just managing the whole team and a team leader just could manage the developers team, as a senior developer can manage the juniors and the junior have nothing to manage., every one can schedule the task and organise his own tasks.

is every one able to abandon the task? surly no, just the project manager

All are components , but the teamleader is a composite with a list of components, Project manager same , and technical reference also , you can implement the senior in the same way , but it depends your needs , and the developer is always a component.

many times when i did this example , they said me it's chain of responsibility , but non , you can mix the behavioural with structural to have a more sophisticated chain of responsibility pattern .

elgehelge commented 5 years ago

+1 @kamranahmedse