How would you modify PromotionalRules so that you could add more rules without having to modify the apply_promotions method?

[Yorkshireman]

Not sure how I feel about this one, actually; as it stands, there being few promotions, this refactoring may not provide much/any benefit, in my opinion. However, if there were many promotions, as would be the case with a large website, this new code would be easier to use, so this is more scalable. So, I guess it is better!

The only catch with this new code is that the order of the promotions is important - as the code stands here, the ten_percent_discount method has be to placed at the end of the PROMOTIONS array in order for the discount to be applied on the total after the lavender_heart promotion.

I'm slightly concerned that the next modification I have to make will force further changes on this solution, but we'll see.

In case you missed it - the code for this commit.

[dwfait]

You're quite right that these changes don't really improve anything right now. However, what the question was trying to drive at, is that writing software is all about writing software that is easy to change.

Two principles which help us do that that this code was violating are the Open / Closed principle, and the Single Responsibility principle.

Let's say that the initial promotional rules are wildly successful, and in a couple of months, marketing wants to start A/B testing promotional rules. Now, each rule just got more complicated, because they only refer to a subset of customer; and in addition, this file is likely to see a lot of churn of promotional rules being added, removed, and altered. If we're not careful, it could end up a mess.

You also make a good point of order precedence, however that detail was buried even more deeply previously - it wasn't clear that because the 10% discount was at the end of an if/else, that it needed to be there.

After this change, the class still has a lot of responsibility. If I were to add a new rule, I would edit the array at the top of this file, and add a method. As you documented, I would also have to remember the precedence of rules and try and honour it.

What I would probably do is extract out each promotional rule as a class, and have the array hold objects of the class instead of the symbols for methods.

It also sounds like we need a way for either an order for the rules to be run in, or a dependency, or split the list of rules into two types of rules to be run - one which works on an item by item level, and one which works on the basket as a whole.

Would every discount promotional rule be applied after or before item-specific discounts? Would you have mutually exclusive rules such that the highest discount one applies and the rest are ignored?

It's a very complicated issue indeed, which is why many systems only allow one promotional rule, but asking these questions about the system now, when the cost to change is relatively very low, will save a lot of time and money in the long run, and we'll have a better system for it.

[Yorkshireman]

A lot of food for thought here - what I like about this is that yet again you are satisfying my itch to split things out a lot. I thought it was just me, but clearly it's not! Like I said in the other thread, I often get the urge to split things out a lot more than I have here, but have held back.

Having said that, when there were only two promotions, it didn't cross my mind to split things out too much, but when this extra promotion was added, it did cross my mind at one point to create two different 'types' of promotions, like you said - one for 'item promotions' and one for discounts on the whole basket.

I'll try to find time to take a look at this more thoroughly but these are just some initial thoughts. I quite like the idea of having a class for each promotional rule because if there were a lot in future, the PromotionalRules class would become quite difficult to work with, whereas a whole class can be easily deleted (or added), for example. It also follows good design principles - come to think of it, PromotionalRules may have been violating the SRP from the start because simply by being pluralised and having more than one rule in it! Debatable but interesting.

A technique I often use in Rails is Service Objects (example here. I wonder if it might be better to have a ApplyPromotions service object class instead of a PromotionalRules class? And then have a class for each promotional rule? Sounds like a good idea to me - what do you think?

Could have an array of promotions objects inside ApplyPromotions and that service object could be called from the Checkout class.

[dwfait]

Splitting things out and applying SRP and Open/Closed are quite popular here, so no need to hold back :) It's proved quite successful many times for us.

You're quite right in that it makes no sense for just two promotional rules. And if you knew with 100% absolute certainty that there will only ever be two, you'd be mad to split it out. It'd cost you time that could be doing other useful things, for what benefit? The code would just sit there, doing its job, working as intended.

In the real world, however, the only constant in software is change. Feature and change requests come in from the client, and things are added, removed, altered. If we're not careful to start with structure and keep our code in order, entropy (https://en.wikipedia.org/wiki/Software_entropy) kicks in and ruins our day, making everything we do slower and bug-prone.

So, when designing things like this, it's important to not only see the software as it is right now, but ask, "what is this likely to become?" "What are the invariants here and how can I make it easy to change them?"

Your service objects look great and they definitely look like examples of what we use here in our Rails apps. I like the idea of having an ApplyPromotions class which knows how to do that, "PromotionalRules" doesn't really tell us about what the class does, but "ApplyPromotions" is very clear. :+1:

[Yorkshireman]

I've done a big refactor - ditched the PromotionalRules class and basically did what I outlined above: created an ApplyPromotions service object and made everything work with that. Also restructured the test suite accordingly. It's all on the master branch here.

You'll probably notice that I decided to split the types of discounts into item_promotions and basket_discounts, where basket discounts are discounts applied to a total price and item promotions are discounts applied to individual items. I have a nagging doubt about the name choice for these variables - maybe they're not the most self-explanatory names?

I also noticed that, in the Checkout class, the ProductRange module wasn't 'injected' into the class, but was referenced in two different places, so I've refactored that to be injected and used via an attr_reader instead.

I know that's always the best thing to do if ProductRange was a class. Making the modification I've made seems to make more sense because now the Checkout is always initialized with a product range and if ProductRange was to change to something else, it's easier to change. What do you think?Relevant commit (same as above) here

I'm also wondering what you think about my lib/ and spec/ folder structure now?

[Yorkshireman]

I've just noticed an inconsistency between my Checkout class and ApplyPromotions class: Checkout uses attr_reader + initialize method to set basket and product_range and ApplyPromotions just uses straight instance variables. Maybe I should change one of them, but not sure which is best?