Kata

I just noticed that this kata was tagged "puzzle". Maybe the puzzle is to figure out why there are discrepancies in a small number of random tests. LOL

As usual, "puzzle" tag is used as a poor excuse of "I can't figure out how to make my kata logically sound and not insane, and would like to frustrate other users by subjecting them to torments of trying to read my mind"...

Hello mortonfox, The discrepancy is due to assert_approx_equals accidentally being left in after converting all the floats to use ints. Let me know if this solves your problem.

I chose the puzzles tag because I don't know what other category this falls under. I figured a vague over-used tag would be better than no tag. If there's one that's more applicable, or if no tag is actually better, I'd be happy to change it.

order type coupons should each be processed exactly once if they exist

Reference solution is inconsistent over itself on this matter too:

• n% off order/n% off x$ or larger order can only be applied once for each coupon with the same string. So 10% off order, 20% off order and 20% off 0$ or larger order can be stacked on each other, but not multiple 10% off orders
• Reward points order can only be applied once. Period. Not even if multiple reward points coupon exist with different values

Related issue: reward points coupons are always the same inside each test. This is defintely unexpected.

Some coupons are incorrectly implemented:

• n% off order: lambda x: x - (x * 0.1) this returns the after discount value (90%), not the discount (10%)
• 2 for 5$: lambda x: 5 * (x // 2) if x >= 2 else 0 this means save 5$ every 2 item, not every 2 item is priced 5$

This description is just plain wrong. In fact these are 3 different kinds of coupons, and the accompanied functions all have different signatures.

I agree with your observation about how the coupons function. I've double checked that the logic works correctly with this usage, and it can be correctly calculated as described, as long as you consider how and why these functions return the number that they do. However, I've changed the descriptions of coupons drastically to account for some of the other issues raised. This specific wording is no longer in the description.

Reference solution is inconsistent over itself on this matter too.

I believe this is incorrect. n% off order and n% off x$ or larger order do not exist in the actual testing. Much like actual coupons, these two discounts are predetermined and will not change. The random coupons cover a pretty wide variety of items, which is the main contributor to the total. I didn't really see the need to go too overboard with sub-total manipulation type coupons since this value is fixed after items are scanned.

Related issue: reward points coupons are always the same inside each test. This is defintely unexpected.

I really hate saying this, but I cannot replicate the issue. I added a print statment each time a rewards coupon is generated, and then ran the kata about 10 times. I copied the logs into a pretty simple parser that isolated the numbers, and I couldn't find any that were duplicated. I'm using import random for the random tests, and I'm not changing the seed anywhere, so it should be automatically updated, right? If you can replicate, or have something I could try, just let me know and I'll jump on it asap.

Some coupons are incorrectly implemented.

10% off order was fixed. 2 for $5 can be correctly implimented using the modified lambda. If I need to update the description to indicate that the lambda functions are not all encompasing, but simply one option to calculate the correct discount, let me know. From the current description:

All coupons should be based on the sub-total and NOT the final total. All coupons come with a lambda function that you are free to use, but not required to. Either way, it is up to you to apply the coupon correctly. (For those unaware, BOGO is short for buy one, get one free.) See example:

{
# ITEM TYPE COUPONS
f'BOGO {item}' : lambda x: x // 2,
f'2 for $5 {item}' : lambda x: 5 * (x // 2),
f'25% off {item}' : lambda x: x * 0.25,

# ORDER TYPE COUPONS
f'10% off order' : lambda x: x * 0.1,
f'10$ off 20$ or larger order' : lambda x: 10 if x >= 20 else 0,
f'Reward points: {0 <= n <= 100000}' : lambda x: x // 1000
}

I've double checked that the logic works correctly with this usage, and it can be correctly calculated as described, as long as you consider how and why these functions return the number that they do.

The problem is, you've basically implemented a completely wrong version of Strategy Pattern. The point of providing a lambda function for each coupon is so that you can calculate the discount for all coupons without having to handle specific implementations one by one on the consuming side. What you've done is the opposite: you've provided a function for every coupon, but every coupon's function are used differently, so the coupon-consuming code still has to pattern every coupon type. This is an anti-pattern.

.

This has been fixed. Those lambda functions now require two variables, but they have been adjusted to fix the anti-pattern. I'll do some reading on that link you provided to make sure everything else is working right.

Those lambda functions now require two variables

The implementation is wrong again: BOGO expects to pass in f(quantity, price per item), x for y$ item expects to pass in f(quantity, quantity * price per item), x% off item expects to pass in x(quantity * price per item), x% off order and x$ off y$ or larger order coupons expects to pass in f(subtotal).

2 for 5$ coupons is again implemented incorrectly: lambda x, y: y - (500 * (x // 2)) This would claim that 3 items costs only 500, but the lone 1 item should cost the price of an item itself.

Also, BOGO and 2 for 5$ coupons are straight out ignored if there are only 1 item. This is not mentioned at all.

Also, BOGO forgot to count the item as already scanned.

I have to say, I didn't expect this to get worse than before. Now coupons don't even expect the same amount of arguments (and yet still expects all kinds of different arguments), and the implementation is still wrong. It looks you haven't tested your changes at all before making the edits, or actually understand what was the anti-pattern.

I really hate saying this, but I cannot replicate the issue. I added a print statment each time a rewards coupon is generated, and then ran the kata about 10 times. I copied the logs into a pretty simple parser that isolated the numbers, and I couldn't find any that were duplicated. I'm using import random for the random tests, and I'm not changing the seed anywhere, so it should be automatically updated, right? If you can replicate, or have something I could try, just let me know and I'll jump on it asap.

Have you actually tested your kata and logged your random inputs? I'm very dissatisfied with this response, because it certainly looks like you haven't actually tested anything: Logging coupons array generated by your random tests already tells easily the reward coupons in any coupons array contain the same value.

I understand your frustration, but yes. To the best of my ability, I have tested that this works the way it was intended. I see the issue you're describing now, and I'm not sure if it matters. If given a coupon array of one reward points coupon, you'll get a random number that should be given to the coupon for the discount amount. If you recieve two or more rewards coupons, while, yes they all have the same number currently, it doesn't matter. That coupon should only be processed one time anyway. If it's that important to add the appearance of randomness, I will happily do this. I just don't see the reason given that the rules were pretty explicit about how to process duplicate coupons.

If you recieve two or more rewards coupons, while, yes they all have the same number currently, it doesn't matter

It matters from the standpoint of de-duplicating coupons: de-duplicating by the entire coupon name rather than reward coupons will fail tests where multiple reward coupons of different values exist.

This isn't ruled out by the description, so there are no reasons to arbitrarily remove some of the possible inputs. Lack of test coverage (testing the possible kind of scenarios fully) is a serious testing issue: you can't just say "I don't need to test it because the rules explained this pretty clearly". Code doesn't care about your description; it only cares about your tests.

Speaking of which, where are the explanations of reward points coupons? It used to be in the description but now it's gone, so the meaning of it is unclear.

Updated:

# ORDER TYPE COUPONS
f'10% off order' : lambda x: x // 10,
f'10$ off 20$ or larger order' : lambda x: 1000 if x >= 2000 else 0,

# Reward points is a special order type coupon that gives $1 discount per 1000 points.
f'Reward points: {0 <= n <= 100000}' : lambda x: 100 * x // 1000

order type coupons should each be processed exactly once if they exist.

Let me know if that's a clear enough description for the rewards coupons.

It matters from the standpoint of de-duplicating coupons: de-duplicating by the entire coupon name rather than reward coupons will fail tests where multiple reward coupons of different values exist. This isn't ruled out by the description, so there are no reasons to arbitrarily remove some of the possible inputs.

I see your point now. My appologies for not understanding.

preloaded.COUPONS is completely useless: it's a string of newline-delimited key-values which is a hassle to parse, and this is not needed anyway since the coupons pass in are already the parsed form of the key-value pairs.

2 for $5 coupons for the majority of the items are actually pointless because most items are priced below 2.5, in which case buying 2 for $5 would be paying more than the normal price.

Currently coupons are applied with no regards of this possibility, so in certain cases it can result in a negative discount, or a increased total from sub-total.

(Not like this can be tested anyway, since 2 for $5 coupons are incorrectly implemented right now: see comments in another issue above)

I will add a check to the coupon randomizer to account for the price of an item.

This is happening all the time in the random tests at the moment: coupons like 2 for $5 soda generates negative discounts.

@pxsonger: you can refer to this page to implement tests with floats: https://docs.codewars.com/languages/python/authoring (search for assert_approx_equals).

Note how the article another article points out that floats and money don't mix.

@mortonfox Thanks for the heads up. I've upded the tests to use 0.001 accuracy with approx_equals. I'm pretty sure your solution will work now. Sorry for the trouble.

@hobovsky and @akar-0 Thanks for the links. I've thought about having the tests use rounding to 2 decimal places (since we're representing money after all), but since there are issues with floats generally, I didn't want to make the requirements to difficult for rounding errors. I've never used the decimal module, but I'd be happy to do some digging and implement that into the tests if you think that would make things better and more consistent?

alternatively, I could change the way we're calculating sales tax, since (at least in my code) that's the only place a float is used. I could change it to use max(1, subtotal // 10)) which would be able to return a clean integer for tax, but then we get into the potential for rounding errors.

Any direction is appreciated. I'll wait to resolve the issue until I either come up with something, or we decide how it should be changed.