Function design:
check_if_condition_was_met(...)
is an awkward name, partly because of the unusual use of past tense. But why make it so wordy? Couldn't you just call itverify(...)
?Furthermore, it would be customary to put the
condition
parameter first, and therow
second. It would certainly read more naturally in English, especially after the function rename. Also, based on the observation thatverify(condition)
could be considered as a test, it's a general functional-programming principle that thecondition
parameter should be considered more tightly associated with the verification process and should therefore be put first.Use
all(...)
: To You want to express the idea that a function should returnTrue
if all of the conditions must be met, andFalse
if any condition fails, use. You can do that usingall()
with a generator expression. It's a lot less cumbersome than yourcondition_met
,for
,continue
, andbreak
. The entire function can be simplified down to a single expression!Naming: I think that "key" and "val" are not quite descriptive enough. I suggest the following renamings:
key
→comparator
val
→criteria
condition_key
→key
condition_val
→desired_val
Function design:
check_if_condition_was_met(...)
is an awkward name, partly because of the unusual use of past tense. But why make it so wordy? Couldn't you just call itverify(...)
?Furthermore, it would be customary to put the
condition
parameter first, and therow
second. It would certainly read more naturally in English, especially after the function rename. Also, based on the observation thatverify(condition)
could be considered as a test, it's a general functional-programming principle that thecondition
parameter should be considered more tightly associated with the verification process and should therefore be put first.Use
all(...)
: To express the idea that a function should returnTrue
if all of the conditions must be met, andFalse
if any condition fails, useall()
. It's a lot less cumbersome than yourcondition_met
,for
,continue
, andbreak
. The entire function can be simplified down to a single expression!Naming: I think that "key" and "val" are not quite descriptive enough. I suggest the following renamings:
key
→comparator
val
→criteria
condition_key
→key
condition_val
→desired_val
Function design:
check_if_condition_was_met(...)
is an awkward name, partly because of the unusual use of past tense. But why make it so wordy? Couldn't you just call itverify(...)
?Furthermore, it would be customary to put the
condition
parameter first, and therow
second. It would certainly read more naturally in English, especially after the function rename. Also, based on the observation thatverify(condition)
could be considered as a test, it's a general functional-programming principle that thecondition
parameter should be considered more tightly associated with the verification process and should therefore be put first.Use
all(...)
: You want to express the idea that a function should returnTrue
if all of the conditions must be met, andFalse
if any condition fails. You can do that usingall()
with a generator expression. It's a lot less cumbersome than yourcondition_met
,for
,continue
, andbreak
. The entire function can be simplified down to a single expression!Naming: I think that "key" and "val" are not quite descriptive enough. I suggest the following renamings:
key
→comparator
val
→criteria
condition_key
→key
condition_val
→desired_val
Similar to @Peilonrayz, I would map each of the comparators to a function, but I'd go further.
Function design:
check_if_condition_was_met(...)
is an awkward name, partly because of the unusual use of past tense. But why make it so wordy? Couldn't you just call itverify(...)
?Furthermore, it would be customary to put the
condition
parameter first, and therow
second. It would certainly read more naturally in English, especially after the function rename. Also, based on the observation thatverify(condition)
could be considered as a test, it's a general functional-programming principle that thecondition
parameter should be considered more tightly associated with the verification process and should therefore be put first.Use
all(...)
: To express the idea that a function should returnTrue
if all of the conditions must be met, andFalse
if any condition fails, useall()
. It's a lot less cumbersome than yourcondition_met
,for
,continue
, andbreak
. The entire function can be simplified down to a single expression!Naming: I think that "key" and "val" are not quite descriptive enough. I suggest the following renamings:
key
→comparator
val
→criteria
condition_key
→key
condition_val
→desired_val
I'm also skeptical about some of the behaviour when given anomalous input:
- Why are there
int(...)
casts with'min'
and'max'
? Is it for parsing strings as numbers? None of your example cases needs such parsing, though. Is it for truncating floats towards zero? Probably not, but it might have that unintended effect, if the thresholds or data are already numeric. - What happens if the row is missing a key that is specified in the condition? Maybe it's not a concern to you, but it might be more appropriate to have the function return
False
rather than raise aKeyError
, as your code does?
Consider writing doctests to explain what the function does. This is a situation where examples are more expressive than words.
Suggested solution
COMPARATORS = {
'in': lambda v, lst: v in lst,
'not in': lambda v, lst: v not in lst,
'min': lambda v, n: (v is not None) and (v >= n),
'max': lambda v, n: (v is not None) and (v <= n),
}
def verify(condition, row):
"""
Verify that the specified criteria are all true for the given row.
>>> rows = [
... {'Flag1':'Y', 'Flag2':'Canada', 'Number':35},
... {'Flag1':'Y', 'Flag2':'United States', 'Number':35},
... {'Flag1':'N', 'Flag2':'United States', 'Number':35},
... {'Flag1':'N', 'Flag2':'England', 'Number':35},
... {'Flag1':'N', 'Flag2':'Canada', 'Number':35},
... {'Flag1':'N', 'Flag2':'Canada', 'Number':5},
... ]
>>> [verify({'in': {'Flag1': ['N'], 'Flag2': ['United States']}}, r)
... for r in rows]
[False, False, True, False, False, False]
>>> [verify({'not in': {'Flag1': ['Y']}, 'min': {'Number': 7}}, r)
... for r in rows]
[False, False, True, True, True, False]
>>> [verify({'not in': {'Blah': ['whatever']}}, r) for r in rows]
[True, True, True, True, True, True]
"""
return all(
all(
COMPARATORS[comparator](row.get(key), desired_val)
for key, desired_val in criteria.iteritems()
)
for comparator, criteria in condition.iteritems()
)