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!
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()
- Django admin β group permissions to edit or view models
- Free-form input for ForeignKey Field on a Django ModelForm
- Can I delete the django migration files inside migrations directory
- Django settings Unknown parameters: TEMPLATE_DEBUG
- Django-storages get the full S3 url
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 |
-----------------
- Removing case sensitivity from Email in Django login form
- TypeError at / __init__() takes exactly 1 argument (2 given)
- Adding static() to urlpatterns only work by appending to the list
- Is it possible to stop Django from creating .pyc files, whilst in development?
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.
- How to create a filter form for a (class based) generic object list in Django?
- ImportError: 'tests' module incorrectly
- Shouldn't django's model get_or_create method be wrapped in a transaction?
- Set numbers of admin.TabularInline in django admin
- Django translations and gettext: The deprecation of the % (string interpolation) operator
2π
Keep the quantity in a single field, and have the type be a separate field that uses choices
.
- Django: Custom User Model fields not appearing in Django admin
- Django mails not being saved (File backend)
- Google App Engine logs a mess of New connection for β¦ and Client closed local connection on
- How can I use Django Social Auth to connect with Twitter?
2π
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)
1π
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")
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]
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.
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.
- Python ctypes MemoryError in fcgi process from PIL library
- Filter queryset by reverse exists check in Django
- How to solve Authentication process canceled error?
- Validating upload file type in Django
-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.
- Start django shell with a temporary database
- What is the difference between `assertIn` and `assertContains` in Django?
- Python β pyodbc call stored procedure with parameter name
- Nested Serializer not showing up