Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong Composite Pattern Definition #28

Open
ozen opened this issue Feb 17, 2017 · 11 comments
Open

Wrong Composite Pattern Definition #28

ozen opened this issue Feb 17, 2017 · 11 comments

Comments

@ozen
Copy link

ozen commented Feb 17, 2017

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
Copy link
Contributor

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
Copy link

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
Copy link

Furgas commented Feb 23, 2017

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
Copy link

Hyunk3l commented Feb 26, 2017

@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
Copy link

floer32 commented Feb 28, 2017

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
Copy link

seyfer commented Feb 28, 2017

+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
Copy link

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
Copy link

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
Copy link

shmulim commented Dec 1, 2017

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
Copy link

XaaXaaX commented Sep 2, 2018

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
Copy link

+1
@kamranahmedse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests