tags:

views:

869

answers:

4

I am using the following class to easily store data of my songs.

class Song:
    """The class to store the details of each song"""
    attsToStore=('Name', 'Artist', 'Album', 'Genre', 'Location')
    def __init__(self):
        for att in self.attsToStore:
            exec 'self.%s=None'%(att.lower()) in locals()
    def setDetail(self, key, val):
        if key in self.attsToStore:
            exec 'self.%s=val'%(key.lower()) in locals()

I feel that this is just much more extensible than writing out an if/else block, however eval seems to be considered a bad practice and unsafe to use. If so, can anyone explain to me why and show me a better way of defining the above class?

+30  A: 

Yes, using eval is a bad practice. Just to name a few reasons:

  1. There is almost always a better way to do it
  2. Very dangerous and insecure
  3. Makes debugging difficult
  4. Slow

In your case you can use setattr instead:

class Song:
    """The class to store the details of each song"""
    attsToStore=('Name', 'Artist', 'Album', 'Genre', 'Location')
    def __init__(self):
        for att in self.attsToStore:
            setattr(self, att.lower(), None)
    def setDetail(self, key, val):
        if key in self.attsToStore:
            setattr(self, key.lower(), val)

EDIT:

There are some cases where you have to use eval or exec. But they are rare. Using eval in your case is a bad practice for sure. I'm emphasizing on bad practice because eval and exec are frequently used in the wrong place.

EDIT 2:

It looks like some disagree that eval is 'very dangerous and insecure' in the OP case. That might be true for this specific case but not in general. The question was general and the reasons I listed are true for the general case as well.

EDIT 3: Reordered point 1 and 4

Nadia Alramli
I like this better than `__dic__` since it's more explicit on what you intend to do.
extraneon
In this case, eval is a bad practice. But eval is not a universal bad practice. There are cases where using eval/exec/execfile is great.
codeape
Thank you, I just changed my code to use setattr.
Nikwin
-1: "Very dangerous and insecure" is false. The other three are outstandingly clear. Please reorder them so that 2 and 4 are the first two. It's only insecure if you are surrounded by evil sociopaths who are looking for ways to subvert your application.
S.Lott
@S.Lott, Insecurity is a very important reason to avoid eval/exec in general. Many applications like websites should take extra care. Take the OP example in a website that expects users to enter the song name. It is bound to be exploited sooner or later. Even an innocent input like: Let's have fun. will cause a syntax error and expose the vulnerability.
Nadia Alramli
If eval is evil, why do it exists, in the first place?
Selinap
I am in line with S.Lott.
Selinap
@Selinap, eval has practical use cases but they are rare and special. In the OP case it is very very clear eval is a bad and insecure choice. If something exists that doesn't mean we should blindly use it.
Nadia Alramli
I agree with S.Lott: there is no user data in executed statement, so there is no reason to consider it insecure.
Denis Otkidach
@Denis, you are right the OP case doesn't have user data. But still the question was general: "Is Using eval In Python A Bad Practice?"
Nadia Alramli
@Nadia Alramli: User input and `eval` have nothing to do with each other. An application that's fundamentally mis-designed is fundamentally mis-designed. `eval` is no more the root cause of bad design than division by zero or attempting to import a module which is known not to exist. `eval` isn't insecure. Applications are insecure.
S.Lott
S.Lott's point is really valid. `eval` fundamentally isnt bad/evil. Its the design which is almost always to blame. If your design doesnt guarantee you against evil user-input, `eval` might not be what you should be worried about.
jeffjose
@jeffjose: Actually, **it is** fundamentally bad/evil because it's treating unparamaterized data as code (this is why XSS, SQL injection, and stack smashes exist). @S.Lott: "It's only insecure if you are surrounded by evil sociopaths who are looking for ways to subvert your application." Cool, so say you make a program `calc`, and to add numbers it executes `print(eval("{} + {}".format(n1, n2)))` and exits. Now you distribute this program with some OS. Then someone makes a bash script that takes some numbers from a stock site and adds them using `calc`. boom?
Longpoke
@S.Lott: It's probably better to say that `eval` is sane **if and only if** the method using `eval` doesn't execute insane code as a side effect... Say I have a JSON decoder in JavaScript... I want to pass **anything** to it, I don't want to waste my time writing sanitization code every time I call it (which would be the case if the function just returned `eval(input)`.
Longpoke
Look, everyone using Python isn't writing a web script or application where security is a major concern. So, while I agree that there's often a better way, say using `setattr()`, and that one should make the effort to find them for a better design, I definitely don't agree that eval/exec are **inherently** 'very dangerous and insecure'. There's good reasons they're available and having the abilities they provide is one of the coolest and most powerful features of interpreted languages like Python.
martineau
+5  A: 

In this case, yes. Instead of

exec 'self.Foo=val'

you should use the builtin function setattr:

setattr(self, 'Foo', val)
jleedev
+5  A: 

Using eval is weak, not a clearly bad practice.

  1. It violates the "Fundamental Principle of Software". Your source is not the sum total of what's executable. In addition to your source, there are the arguments to eval, which must be clearly understood. For this reason, it's the tool of last resort.

  2. It's usually a sign of thoughtless design. There's rarely a good reason for dynamic source code, built on-the-fly. Almost anything can be done with delegation and other OO design techniques.

  3. It leads to relatively slow on-the-fly compilation of small pieces of code. An overhead which can be avoided by using better design patterns.

As a footnote, in the hands of deranged sociopaths, it may not work out well. However, when confronted with deranged sociopathic users or administrators, it's best to not give them interpreted Python in the first place. In the hands of the truly evil, Python can a liability; eval doesn't increase the risk at all.

S.Lott
+1  A: 

It's worth noting that for the specific problem in question, there are several alternatives to using eval:

The simplest, as noted, is using setattr:

def __init__(self):
    for name in attsToStore:
        setattr(self, name, None)

A less obvious approach is updating the object's __dict__ object directly. If all you want to do is initialize the attributes to None, then this is less straightforward than the above. But consider this:

def __init__(self, **kwargs):
    for name in self.attsToStore:
       self.__dict__[name] = kwargs.get(name, None)

This allows you to pass keyword arguments to the constructor, e.g.:

s = Song(name='History', artist='The Verve')

It also allows you to make your use of locals() more explicit, e.g.:

s = Song(**locals())

...and, if you really want to assign None to the attributes whose names are found in locals():

s = Song(**dict([(k, None) for k in locals().keys()]))

Another approach to providing an object with default values for a list of attributes is to define the class's __getattr__ method:

def __getattr__(self, name):
    if name in self.attsToStore:
        return None
    raise NameError, name

This method gets called when the named attribute isn't found in the normal way. This approach somewhat less straightforward than simply setting the attributes in the constructor or updating the __dict__, but it has the merit of not actually creating the attribute unless it exists, which can pretty substantially reduce the class's memory usage.

The point of all this: There are lots of reasons, in general, to avoid eval - the security problem of executing code that you don't control, the practical problem of code you can't debug, etc. But an even more important reason is that generally, you don't need to use it. Python exposes so much of its internal mechanisms to the programmer that you rarely really need to write code that writes code.

Robert Rossney
Another way that's arguably more (or less) Pythonic: Instead of using the object's `__dict__` directly, give the object an actual dictionary object, either through inheritance or as an attribute.
jleedev