0

So I've got a class that calculates a discount (Let's call it class "A"). It has a price fetcher class as a dependency (constructor injection) (Class "B"). The price that is returned (by class "B") is used to calculate the discount. However it is not obvious that class "B" is used to fetch a price unless you go into the implementation of the function calculateDiscount in class "A".

There are multiple ways one can fetch a product price in the framework; one of them is through the use of class "B". But class "B" can do more stuff than just returning the price, it has some other functionalities (not built by me; so I can say it has multiple responsibilities).

My goal is to amend the price that class "B" returns. To achieve that I can create another class (Class "C") that inherits from "B" (it has no interface) and inject it.

The problem is that I know that I rely on the implementation detail in my code. If the code changes, and another way of fetching the price is used, the whole thing will stop working. I am not happy about it. But also that very class ("B") is explicitly injected in class "A" so it's not necessarily an implementation detail ONLY, it is also some sort of the abstraction as well (because it is injected in the constructor). However I am never sure that it is used the way I want it to be used; as I said above it has multiple functions, what if it is no longer used in class "A" to fetch the price, but still used (in same class "A") to do something else? It means that replacing it with class "C" will not achieve fetching the amended price (since another way of fetching is used). For it to work I kind of have to have a promise that that class "B" will be used to fetch the price.

I am not sure if I've explained it well. I am looking for a neat way to amend the functionality; but at the same time I want to make sure I am not dependent on the implementation detail.

My fallback solution would be just to copy/paste the whole method into another class and implement some changes. It would work; and it would not depend on any implementation detail; but the drawback is the code duplication (that discount calculation function is quite large on its own).

I am perplexed because it feels like that's the whole purpose of using DI - to allow extensibility. But what happens if that DId item is capable of doing multiple things and you never know how it is used inside the class? Looking at the class to verify that the dependency is used the way you want of course can work, but someone else might work on some other task and change that code and it will work for the new task, but my part can fail because of it; I want to avoid that.

Any ideas?

Update: I took a shower and realised that DI as we have is not as good as having a DI of methods/functions. It would be cool if the method requested another method instead of an object. Object might have all kinds of stuff, but method is unambiguous. But I can't do it in this case though.

Let me show an example (product price without the tax is 500; with the tax it is 600). Depending on the database tax setting the price fetcher will return either 500 or 600:

function calculateDiscount($item, $discountPercentage) {
    return $priceFetcher->getPrice() * $discountPercentage
}

I want to change it so that it is not dependent on that database tax setting:

function calculateDiscount($item, $discountPercentage) {
    return ($includeTax ? $item->getPriceExclTax() : $item->getPriceInclTax()) * $discountPercentage
}

I can simply go to the constructor and replace my price fetcher with another so that the first implementation would work! The problem is that if someday somebody changes that implementation and uses another service? I am working in the scope of the function calculateDiscount. I can't rely that inside the price fetcher method will be used, can I? My idea is about making sure that my alteration works for the method calculateDiscount regardless of what's happening inside that method; like if it was completely hidden from the reader!

To achieve that it feels like I need to just have another discount calculator that would have a copied/pasted implementation with some amendments. But that's not good either. I am hoping there is a third solution somewhere. I came up with the idea about injecting methods instead of objects; and wondering that there is some sort of literatue about this topic or some good examples. Maybe this is functional programming stuff here; not sure.

6
  • 2
    im confused. why does the method of fetching the price affect the discount? should it?
    – Ewan
    Jun 30, 2022 at 14:12
  • 2
    "If the code changes, and another way of fetching the price is used, the whole thing will stop working." It really, really should not. And honestly, I can't see why it does, in your case. This part could use some more info; why is this the case? (or, at least, why do you think this is the case?)
    – Duroth
    Jun 30, 2022 at 14:20
  • It doesn't affect the discount, Ewan; it is used to calculated the discount ("20% Off" means we take the price and multiply it by 0.2). In my case it does change because I want to conditionally choose whether the discount includes the tax. By default it does not, but I need that change, Duroth.
    – pro100tom
    Jun 30, 2022 at 14:36
  • Whether you charge tax or not should not affect the rest of the price calculations. Jun 30, 2022 at 14:44
  • 1
    "My goal is to amend the price that class "B" returns." — this is the crucial sentence in your question. Why do you need to amend the price? How do you need to amend it? Without answering those questions, I can't fathom an answer. Jul 4, 2022 at 2:30

2 Answers 2

1

I am perplexed because it feels like that's the whole purpose of using DI - to allow extensibility. But what happens if that DId item is capable of doing multiple things and you never know how it is used inside the class? Looking at the class to verify that the dependency is used the way you want of course can work, but someone else might work on some other task and change that code and it will work for the new task, but my part can fail because of it; I want to avoid that.

Well stop that. You're fighting the very concept of abstraction. When you design something to use something (a dependency) you should clearly communicate your need. You should not seek to micromanage what you're using to ensure such and such is called. Just stick to your need. How what your using does it's job is none of your business.

function calculateDiscount($item, $discountPercentage) {
    return $priceFetcher->getPrice() * $discountPercentage
}

Done in a proper DI style that function looks more like this:

function calculateDiscount($price) {
    return $price * $discountPercentage
}

Now the tax issue is a composition problem not an implementation problem.

calculateDiscount(calculateTax($price))

You don't have to pass controlling Booleans inside to control what it does. You can see what's happening at a glance.

4
  • Thank you for your answer. Does passing the PriceFetcher in the construction as a DI clearly communicates the need? If it only had one public method, then I would be pretty sure it does! But since it has multiple methods and I can't be 100% sure the method I need is used (unless I double check) so I am not sure whether it still clearly communicates the need in this case.
    – pro100tom
    Jun 30, 2022 at 16:00
  • @pro100tom why does it need to be a PriceFetcher, and not an interface of one method that PriceFetcher implements?
    – Caleth
    Jun 30, 2022 at 16:26
  • That price fetcher is core a framework class, that happened to be has no interface. Framework sucks, I should say, it's Magento. I've been working with it since 2014; and even though it brings me good money, I would never recommend anyone to use it!
    – pro100tom
    Jun 30, 2022 at 16:29
  • 2
    @pro100tom The idea here is to express the calculations in the same place and clearly show what's happing in one place. If you need this to happen in different places yet still enforce that tax is always calculated first consider making taxed price a different type than price. That way the type system enforces your rule. Jun 30, 2022 at 21:45
0

It seems to me that this is solved by simply differentiating between the price including tax and the price before tax.

Usually you would have tax as a separate line on an invoice eg

Type  , Price
Item 1, £1.50
item 2, £3.00
tax   , £0.50 

total   £5.00

With this level of detail I can calculated multiple discount types

x% off total
-£1.00 on all "item 1"s
free "item 3" if you also buy two "item 2"s etc

I can also calculate different taxes for different items and ensure that the tax calc includes any discounts.

Its not a DI problem you have it's that your price calculation, including tax and discounts, has a dependency on the order in which you run your various processes.

If you remove that order dependency, or at least keep track of what processes have been applied, then your problem goes away and your code will be more reliable going forward.

..To expand, if you don't want to go the full "invoice is required to calc discounts" route, or you have a big object with multiple responsibilities. Split the interface.

IPriceCalcWithTax //indicates that the internals will calc the price with tax
{
    decimal GetPriceWithTax(order o) 
}

IPriceCalcWithoutTax //indicates that the internals will calc without tax
{
    decimal GetPriceWithoutTax(order o)
}

MyPriceCalculator : IPriceCalcWithoutTax, IPriceCalcWithTax, IWhateverElseTheObjectsResponsiblitiesAre //one or both
{
...

}

MyDiscountCalc(IPriceCalcWithoutTax pricecalc) //indicates that it will need the price calculated without tax
2
  • Thank you for your answer. I understand your approach and agree with it. However the logic in that (existing) class is a bit different and I am unable to add amendmends after the main calculation is happening. Also, my question is not about me being stuck to solve this particular task; my question is generic and is about dependency injection scenario when we don't know whether the specific method is going to be called or not. I am looking for an architectural/structural solution here.
    – pro100tom
    Jun 30, 2022 at 15:02
  • 1
    just have IPriceCalcWithoutTax and IPriceCalcWithTax and inject as required
    – Ewan
    Jun 30, 2022 at 16:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.