Python: if more than one of three things is true, return false

Question

I'm writing a django model that allows my site to have coupons.

Coupons can have three types: lifetime account voucher, certain period of months voucher, certain number of dollars voucher.

To keep things simple, I'm only allowing coupons to have one of the three possible values (i.e. a voucher can't be for $10 and 5 months). But I want to check when a coupon is being saved to ensure this rule is true.

Currently I have:

true_count = 0
if self.months:
    true_count += 1
if self.dollars:
    true_count += 1
if self.lifetime:
    true_count += 1    

if true_count > 1:
    raise ValueError("Coupon can be valid for only one of: months, lifetime, or dollars")

I know there's a better way to do this, but I'm not seeing it (call it coder's block).

Help is much appreciated.

In case it maters, the three types are int, int, and bool

months = models.IntegerField(default=0)
cents = models.IntegerField(default=0)
#dollars = models.FloatField(default=0.00)
#dollars replaced with integer cents per advice of group
lifetime = models.BooleanField(default=False)

1 hour after asking 9 thoughtful answers, this is why I love Stackoverflow — Ted, Jul 14 '11 at 02:47

randlet · Accepted Answer · 2019-02-27T15:54:10.030

12

One thing I've done in similar situations is this:

coupon_types = (self.months, self.dollars, self.lifetime,)

true_count =  sum(1 for ct in coupon_types if ct)
if true_count > 1:
    raise ValueError("Coupon can be valid for only one of: months, lifetime, or dollars")

It's now much easier to add new coupon types to check for in the future!

edited Feb 27 '19 at 15:54

answered Jul 14 '11 at 02:33

randlet

3,628
1
17
21

score 5 · Answer 2 · edited May 23 '17 at 12:13

5

You could also use a list comp to filter false values:

if len([x for x in [self.months, self.dollars, self.lifetime] if x]) > 1:
    raise ValueError()

Or building off MRAB's answer:

if sum(map(bool, [self.months, self.dollars, self.lifetime])) > 1:
    raise ValueErrro()

edited May 23 '17 at 12:13

Community

1
1

answered Jul 14 '11 at 01:55

Zach Kelling

52,505
13
109
108

Thanks. This solution made me chuckle a bit - it wouldn't be a python question without a solution that included a list comprehension. – Ted Jul 14 '11 at 01:57
2

`sum(bool(x) for x in (self.months, self.dollars, self.lifetime))` – John La Rooy Jul 14 '11 at 02:22

Swift · Answer 3 · 2011-07-14T01:59:55.193

2

if (self.months && (self.dollars || self.lifetime))  || (self.dollars && (self.months || self.lifetime)) || (self.lifetime && (self.dollars || self.months))
    raise ValueError("Coupon can be valid for only one of: months, lifetime, or dollars")

Edit:

I did a quick circuit mimization using a Karnaugh map (http://en.wikipedia.org/wiki/Karnaugh_map). It ends up this is the smallest possible function with boolean logic:

if((self.months && self.dollars) || (self.dollars && self.lifetime) || (self.lifetime && self.months))
    raise ValueError("Coupon can be valid for only one of: months, lifetime, or dollars")

Logically both my statements are equivelant but the second one is technically faster / more efficient.

Edit #2: If anyone is interested here is the K-Map

A | B | C | f(A, B, C)
----------------------
0 | 0 | 0 |     0
----------------------
0 | 0 | 1 |     0
----------------------
0 | 1 | 0 |     0
----------------------
0 | 1 | 1 |     1
----------------------
1 | 0 | 0 |     0
----------------------
1 | 0 | 1 |     1
----------------------
1 | 1 | 0 |     1
----------------------
1 | 1 | 1 |     1

Which Reduces to:

   C\AB
     -----------------
     | 0 | 0 | 1 | 0 |     
     -----------------      OR      AB + BC + AC
     | 0 | 1 | 1 | 1 |
     -----------------

edited Jul 14 '11 at 01:59

answered Jul 14 '11 at 01:41

Swift

13,118
5
56
80

2

While this is less lines of code, it makes the code more difficult to understand. I'll take readability over less lines of code. – Mark Hildreth Jul 14 '11 at 01:48
Check out my edited answer. It's easier to read and the most efficient possible algorithm I can think of. – Swift Jul 14 '11 at 01:54
1

How does this scale when the number of potential values goes to four? I.e. if the business requirements change and the voucher can also be good for an integer number of ponies? – Ted Jul 14 '11 at 02:02
It becomes 6 `AND`s separated by logical `OR`s. It scales like any logical circuit with multiple inputs – Swift Jul 14 '11 at 02:24
I like the K-Map idea; I think my implementation of it (below) is a bit more readable since it is raunchily functional, and it scales better. – machine yearning Jul 14 '11 at 03:00

Mark Hildreth · Answer 4 · 2011-07-14T01:53:06.660

2

Your code looks fine. Here's why:

1.) You wrote it, and you're the one describing the logic. You can play all sort of syntactical tricks to cut down the lines of code (true_count += 1 if self.months else 0, huge if statement, etc.), but I think the way you have it is perfect because it's what you first thought of when trying to describe the logic.

Leave the cute code for the programming challenges, this is the real world.

2.) If you ever decide that you need to add another type of coupon value type, you know exactly what you need to do: add another if statement. In one complex if statement, you'd end up with a harder task to do this.

edited Jul 14 '11 at 01:53

answered Jul 14 '11 at 01:47

Mark Hildreth

42,023
11
120
109

4

I disagree with this sentiment and I think it's good that OP is looking to improve upon his code. As soon as you have more than a few if statements like this, it starts to look pretty messy. A more elegant solution is possible without losing clarity. There are a few other solutions posted (including mine) that are quite clear and more easily extensible in the future. – randlet Jul 14 '11 at 02:43
I also have to disagree. While it is possible to get overly clever, [the checked answer](http://stackoverflow.com/questions/6687557/python-if-more-than-one-of-three-things-is-true-return-false/6687869#6687869) is *very* clear and understandable, plus easily extendable. – John C Jul 14 '11 at 14:18

score 2 · Answer 5 · answered Jul 14 '11 at 01:48

2

Keep the quantity in a single field, and have the type be a separate field that uses choices.

answered Jul 14 '11 at 01:48

Ignacio Vazquez-Abrams

776,304
153
1,341
1,358

I was thinking about this possible solution after I posted. I'm wondering if your proposed method or the current method has better fault tolerance. – Ted Jul 14 '11 at 01:56
I think the main issue here is that the "quantity" field needs to store different types. A float could store all of them, but do you really want to store a boolean in a float field? I am, however, in favor of another field that explicitly lists the type, hence the +1 from me. – Mark Hildreth Jul 14 '11 at 02:04
2

One shouldn't store money in a floating point number regardless. Just make it integer cents instead. – Ignacio Vazquez-Abrams Jul 14 '11 at 02:21
What's the rationale behind storing money as integer cents vs. floating point? In certain applications (not this one) there are partial cents that need to be tracked. – Ted Jul 14 '11 at 02:55
Nevermind - quick trip to google found the answer http://stackoverflow.com/questions/3730019/why-not-use-double-or-float-to-represent-currency. – Ted Jul 14 '11 at 02:57

score 2 · Answer 6 · answered Jul 14 '11 at 02:20

I think spreading this over a few lines is fine - this makes it easier to maintain if there were more attributes to test in the future. Using len or sum feels a bit too obfuscated

# Ensure that only one of these values is set
true_count = 0
true_count += bool(self.months)
true_count += bool(self.dollars)
true_count += bool(self.lifetime)

score 1 · Answer 7 · answered Jul 14 '11 at 01:45

I don't know if this is better for you, but doing it this way would work:

if (self.months && self.dollars) || (self.months && self.lifetime) || (self.dollars && self.lifetime):
   raise ValueError("Coupon can be valid for only one of: months, lifetime, or dollars")

score 1 · Answer 8 · answered Jul 14 '11 at 03:48

1

If you have Python2.7 or newer

from collections import Counter
items_to_test = (self.months, self.dollars, self.lifetime)
true_count = Counter(map(bool, items_to_test))[True]

answered Jul 14 '11 at 03:48

John La Rooy

295,403
53
369
502

1

You could make this Python version agnostic by doing true_count = sum(map(bool,items_to_test)) – randlet Jul 14 '11 at 04:42

machine yearning · Answer 9 · 2011-07-14T03:08:17.997

0

Even better solution than before, with combinations, any, and all. Assuming you have all the attributes you want to test in a sequence called attributes:

from itertools import combinations
any(map(all, combinations(attributes, 2)))

In english, it reads

Are any length-2 combinations of the attributes all true?

This solution works for an arbitrary number of attributes, and can be modified to test for an arbitrary number of them being true.

Although admittedly it's very inefficient, I'd say it's pretty cute and readable.

edited Jul 14 '11 at 03:08

answered Jul 14 '11 at 02:39

machine yearning

9,889
5
38
51

1

This would send many programmers to the man pages for half an hour or more, so I can't say its maintainable in most shops. But I have to agree, it is cute. – Ted Jul 14 '11 at 03:05
@Ted: Haha yes, +1 for constructive criticism, but I do think that if you comment your code well, even a novice programmer might see through to the logic behind such a cute snippet. – machine yearning Jul 14 '11 at 03:10
you should replace the literal `2` with `len(attributes)-1` in case attributes get added in the future – John La Rooy Jul 14 '11 at 03:43
@gnibbler: that's not the point... he wanted to know if more than one of the attributes was populated, not if all but one was populated. – machine yearning Jul 14 '11 at 04:15

score 0 · Answer 10 · answered Jul 14 '11 at 04:29

0

How about

if len(filter([self.months, self.dollars, self.lifetime])) > 1:
...

I find it just as readable as a list comprehension with an if clause, and more concise.

answered Jul 14 '11 at 04:29

Greg Ball

3,671
3
22
15

score -2 · Answer 11 · answered Jul 14 '11 at 01:47

-2

bool is a subclass of int because Python originally lacked bool, using int for Boolean, so:

if self.months + self.dollars + self.lifetime > 1:
    ...

This works because False == 0 and True == 1 are both true.

answered Jul 14 '11 at 01:47

MRAB

20,356
6
40
33

6

if self.months is 2, this gives a false positive. You'd need to convert the variables to booleans before summing. – Mark Hildreth Jul 14 '11 at 01:51

Python: if more than one of three things is true, return false

11 Answers11

Linked