if statements or eval?

Question

I'm wondering about the following code:

def ValidateGoto(placeToGo):
 conditions = {}
 conditions["not placeToGo"] = "Enter a positive integer in the text box"
 conditions[
 "int(placeToGo) <= 0"] = "I need a positive integer in the text box please"
 conditions[
 "not DataStore.DBFILENAME"] = "There is no data available in the table"
 for condition, errormessage in conditions.iteritems():
 try:
 if eval(condition):
 print errormessage
 return False
 except ValueError:
 return False
 return True

1. Should the eval statement be avoided?

2. Is this approach justified to reduce the number of if statements?

• Gareth Rees: This code does not work: the indentation is wrong and there is a missing double quote. Can you fix, please?
• Michael Urman: Since I commented on it below, I figured I should point out the indentation of the first return False is still probably wrong. Inside the try/except, this code will always return False, or not catch the exception. It will never hit return True unless there had been no conditions.

Answer 1

While I agree 100% that you should avoid eval for this, if all the cases come from within your code (i.e. not from user provided strings) it's more likely to be a performance problem than a security problem. If you do accept user-provided validations, lambdas aren't inherently safer.

Finally, unless you're accumulating these in some fashion other than described, I think it's more readable to keep your logic together in the same function. Other approaches reduce the number of if statements, but not the number of executed if statements. As a trade-off you have more complex code. I'd suggest keeping it simple, perhaps introducing an exception so that everything can be a single line:

def ValidateGoto(placeGoto):
 if not placeGoto:
 raise ValidationError("Enter a positive integer in the text box")
 if placeGoto <= 0:
 raise ValidationError("I need a positive integer in the text box please")
 if not DataStore.DBFILENAME:
 raise ValidationError("There is no data available in the table")
 # optionally return placeGoto, or True, or whatever.

You'd use this with calling code like this:

try:
 ValidateGoto(goto)
except ValidationError, err
 print err # then possibly return
else: # passed-validation case (can omit else if there's a return in the except)
 ...

If you are looking to make it easier to add named validations, you could take an approach similar to that used by unit tests. Choose a naming convention and write code that looks for the methods that follow that name. Here's a very simple skeleton to illustrate the idea:

class ValidateGoto:
 def __init__(self, placeGoto):
 for validation in dir(self):
 if validation.startswith("validate_"):
 getattr(self)(placeGoto)
 def validate_provided(self, placeGoto):
 if not placeGoto:
 raise ValidationError("Enter a positive integer in the text box")
 def validate_positive(self, placeGoto):
 if placeGoto <= 0:
 raise ValidationError("I need a positive integer in the text box please")
 def validate_table(self, placeGoto):
 if not DataStore.DBFILENAME:
 raise ValidationError("There is no data available in the table")

This variant added a lot of complexity, yet it does allow you to use other tools (such as inheritance) to manage lists of validation functions. For the case as shown I would definitely not use this, but if your actual case has enough complexity to benefit from this, it could be a viable approach.

• itsbruce: That is thumpingly plodding imperative code. A list of checks is being applied; treating them as a list is more accurate, flexible and extensible (you can have a list of named functions, there is no need for them to be lambdas). The checks in a list can be applied in any order, you can stop at the first error or do all the checks or filter the list. All of this is lost in your versions. In addition, you're duplicating the same logic again and again. The basic logic is "make a check, report an error with a message if it fails". You repeat that logic in each function...
• itsbruce: ... when it should only have to be written once. That's a particularly egregious sin in your last example, which adopts an OO style. Your OO example adds complexity to no benefit - why isn't it implementing the shared behaviour once, in a way that could be extended for all checks if desired? If you're going to do OO, make the error-reporting class implement the behaviour - once - and have the checks be a class that contains the salient properties (the check to be applied and the message to report). The OP's original code is actually better style than any of your examples.
• Michael Urman: @itsbruce I think we'll have to disagree here. When the repeated code is if and raise ValidationError, I find the mechanics necessary to avoid repeating it are worse than the repetition. If the repeated raise is troublesome, the message could be stored, and raised if not empty (but that's more mental overhead, and not as flexible in terms of exception thrown). In short, I agree it's imperative. But the first way is also extremely easy to read, so it doesn't distract from the real work. Did you notice the OP's version doesn't even check the second condition (as of rev 3's early return)?
• user1932405: I think that the code provided above is plain and boring. I like the idea with the multiple functions in a list and going through them. Unfortunately, the answer above is simpler, that's why I am going for it. Its creativity against simplicity where I would always personally prefer creativity but I know that simplicity would be much more appreciated.

Answer 2

You have the right basic idea - pass a list of conditions - but the implementation is wrong.

Yes, you should avoid eval; it is a dangerous source of potential error (and performs poorly as a non-bonus). You might put anything in there. You might not even notice a typo which caused nasty side effects, since Python has a very tolerant definition of truth and will be happy as long as the expression evaluates to something. Instead, why not pass a list of lambda functions? Then you just call each function in turn. Much safer.

For example, you could build a list of tuples (condition, error message) like this:

conditions = [(lambda: not placeToGo, "Enter a positive integer in the text box"),
(lambda: int(placeToGo) <= 0, "I need a positive integer in the text box please"),
(lambda: not DataStore.DBFILENAME, "There is no data available in the table")]

And then iterate through the list, calling each function in turn.

If you are not familiar with functional programming (in Python or any other language), take a look at this short tutorial

• user1932405: Thank you very much :) The use of lambads makes it much better.
• itsbruce: Happy to be of help. The impulse behind your question was a good one :)

Answer 3

A safe eval aproach (not sure though if this helps but i would definitily recommend it if you must use it) is by using the ast module's literal eval

import ast
ast.literal_eval(data)

for more information you can take a look here