First, lets understand what was the issue:
The Staple job knows about many more methods which it doesn't use today, (e.g., the print
method) but which are available for it to use, should it want to.
However, the Job class "thinks" that the Staple class will be "a good citizen" and never use the print method at all.
There are many potential big issues here -
For some reason, the Staple job may start using the print method - by accident or intentionally.
Then down the road, either any changes to the print method may go untested,
OR, any changes to the print method will trigger a regression test in the Staple job also,
AND, any impact analysis for changes to the print job will necessarily involve impact analysis of the staple job too.
This is just the issue of Staple knowing about the print functions. Then there's the case of the Print job knowing all about stapling functions. Same problems.
Very soon, this system would reach a point where any change will require a full blown impact analysis of each module and a full blown regression test.
Another problem is that today, all jobs which can be printed can be stapled, and vice versa on this particular printer.
However, tomorrow, there could be a need to install the same firmware on a device that only prints or only staples. What then? The code already assumes that all Jobs are printable and stapleable. So any further granular breakdown / simplification of responsibilities is impossible.
In more recent terms, imagine a class called "AppleDevice" which has functions for MakePhoneCall as well as PlayMusic. Now your problem is while you can easily use this on an iPhone, you cannot use it for an iPod since the iPod cannot make phone calls.
So, the issue is not that the Job class is all-powerful. In fact, that's how it should be, so that it can act as a common link in the entire "workflow" where someone may scan a job, then print it, then staple it etc.
The problem is that the usage of all its methods is not restricted. Anyone and everyone could use and abuse any method whenever they want to, thus making the maintenance difficult.
Hence, the Dependency Injection approach of only telling users "exactly what they need to know, and nothing more" ensures that calling modules only use the code that they are meant to.
A sample implementation would look like:
interface IStapleableJob { void stapleYourself(); }
interface IPrintableJob { void printYourself(); }
class Job implements IStapleableJob, IPrintableJob {
....
}
class Staple {
public static void stapleAllJobs(ArrayList<IStapleableJob> jobs) {
for(IStapleableJob job : jobs) job.stapleYourself();
}
}
class Print {
public static void stapleAllJobs(ArrayList<IPrintableJob> jobs) {
for(IPrintableJob job : jobs) job.printYourself();
}
}
Here, even if you pass a Job object to the Staple and Print methods, they dont know that its a Job, so they cannot use any methods that they are not supposed to. Thus, when you make any changes to a module, your scope of impact analysis and regression testing is restricted. That's the problem that ISP solves.
I see you use php, try to stick to the psr coding styles: psr-1 and psr-2.
Look into the decorator pattern. this is a pattern commonly used in this kind of situations.
You probably have some kind of LineItem
class that is used to represent items in your cart. They probably look something like this:
interface LineItem
{
public function getPrice();
}
With then an implementation for your product:
class ProductLineItem implements LineItem
{
public function __construct(Product $product, $quantity) {
$this->product = $product;
$this->quantity = $quantity;
}
public function getPrice()
{
return $this->quantity * $this->product->getPrice();
}
}
Now it is time to create our abstract decorator:
abstract class LineItemDecorator implements LineItem
{
public function __construct(LineItem $item) {
$this->item = $item;
}
public function getPrice()
{
return $this->item->getPrice(); //delegation
}
}
this abstract class is not needed. I just tend to use it to delegate everything to the base object only in one place. Especially useful if multiple methods exist.
Note also how my Decorator implements the same interface!
And then now it's time for our Discounts:
class DiscountLineITemDecorator extends LineItemDecorator
{
public function getPrice()
{
return $this->item->getPrice() * 0.75;
}
}
We would then procede to use it:
$lineItem = new ProductLineItem($theProduct, 2);
$lineItemWithDiscount= new DiscountLineITemDecorator($lineItem);
//lets add it to our cart
$cart->addLineItem($lineItemWithDiscount);
Our cart then probably also has a getPrice()
method. Probably it will look something like this:
foreach ($this->lineITems as $lineItem) {
$price += $lineItem->getPrice();
}
return $price;
So what about discount on an entire cart?
$cartWithDiscount = new DiscountCartDecorator($cart);
and boom
$cartWithDiscount->getPrice();
All code used here serves as an example. The naming can be improved a lot, so feel free to edit!
Best Answer
As an outsider looking at this code, it is not clear how the "hidden dependencies" get created to begin with. What if there is no "db" dependency registered? Does it throw an exception, return null? If my class doesn't get a "db" dependency, how bad is that?
Many times people lean towards this pattern when there are too many dependencies to pass in the constructor without making the constructor look like complete garbage. If your class requires a large number of dependencies, consider using "setter injection" instead (for a property named "foo", call "setFoo(...)" to inject that dependency). Reserve constructor arguments for those dependencies that are absolutely required just to have the object exist -- not to use the object, but just to make it exist.
It also raises the question, is this class doing too much? Do you really need some of these dependencies injected at all? There is a balance between dependency injection and coupling. Too far either way and you create headaches for yourself.
If you are using true Inversion of Control, then you shouldn't need to. If you need to add another dependency to the class, you should look back at what this class is doing. Perhaps you need a sub type instead. Adding dependencies could be an indication that you are breaking the Open-Closed Principal of programming (Open for extension, but closed for Modification).
It's not to say you can't modify a class once it is written, but you should not modify the intended use of that class -- it's purpose.
Adding a new dependency tells me the purpose of this class has changed, and it's likely you need to break the class up into other classes, or create a child class extending your original one. That way existing code that does not need the functionality of the new class (and the new dependency) doesn't have to change.