2

I know eval() and exec() should be avoided, but in this situation it seems like the best choice: I'm getting values from checkboxes and textboxes in wxPython and putting them in my config class. Here's how I'm using eval():

config = wx.Config()
checkBoxes = ['option_1', 'option_2']
for key in checkBoxes:
    config.Write(key, str(eval('self.m_checkBox_'+key+'.GetValue()'))

There aren't any security problems because there isn't any user-input to eval, and it seems pretty clear to me. Is there a better way to do this?

br1ckd
  • 556
  • 3
  • 15
  • 1
    Put checkbox instances into an array and enumerate through it. – Raman Zhylich Aug 01 '12 at 21:53
  • 1
    you can store m_checkBox_key's in a dict... – zenpoy Aug 01 '12 at 21:53
  • A dictionary would be my first choice in this situation, but I'm using wxFormBuilder to generate my GUI so I'm a little restricted. I could set all of the object names to something like `m_checkBox['option_1']`, but then I'd have to add code to initialize the dictionary. I'm going to port this to C++ after I finish the python version, so I want to keep the GUI as portable as possible. – br1ckd Aug 01 '12 at 22:12
  • If you have complete control over what strings get `eval()`'d, then it's not "wrong" to use it. There are very little use cases, (this one included), where using `eval()` is the appropriate solution, however. – Joel Cornett Aug 01 '12 at 22:36
  • @user1091954 If you're porting to C++ later, wouldn't using a dictionary make this ultimately *easier*, seeing as C++ doesn't really let you look up fields by name? – millimoose Aug 02 '12 at 23:27

5 Answers5

6

How about:

config = wx.Config()
checkBoxes = {'option_1': m_checkBox_option_1, 
              'option_2': m_checkBox_option_2}
for checkbox in checkBoxes:
    config.Write(checkbox, str(checkBoxes[checkbox].Value()))
Sam Mussmann
  • 5,883
  • 2
  • 29
  • 43
  • I'm trying to avoid writing things more than once. You had to write `option_1` twice, and `m_checkBox_` twice. With lots of options it gets tedious, but still better than writing the config.Write() function each time. – br1ckd Aug 01 '12 at 22:05
  • In my experience, typing somewhat more when you write code to make it simpler to read usually pays off. :-) That said, if you don't want to type as much, you could consider (1) making a class of your own that holds onto both the name of the checkbox and the checkbox object and (2) making the dictionary the canonical location of the checkbox objects. (e.g. `checkBoxes = {'option_1': create_new_checkbox}`) Let me know if you'd like further explanation of these in the answer. – Sam Mussmann Aug 01 '12 at 22:10
  • It seems perfectly clear to me, but I'm the one who wrote it. I posted it here to make sure it's easy for other people to understand, and to see if there is a better way. I'm using wxFormBuilder to generate my GUI, and I want to keep it (the GUI) entirely portable so I can easily port it to C++ later, so I'm a little restricted. – br1ckd Aug 01 '12 at 22:20
  • Instead of `self` having separate `m_checkBox_option_1` and `m_checkBox_option_2` in the first place, it should have `m_checkBox` which is a dictionary with `'option_1'` and `'option_2'` keys, and the original checkbox objects as values. Then we can do code like `self.m_checkBox[name].GetValue()`. The only reason this solution has redundant code is because it works around a design that was wrong in the first place. – Karl Knechtel May 30 '22 at 00:46
6

The other answers have done a good job of proposing alternate solutions to the problem at hand, so I'm going to look at the bigger question you asked, and direct you to the words of a smarter developer who wrote about eval's problems at length.

When I see eval, a dark cloud descends upon the surrounding code, and I eye the whole mess with suspicion and mistrust until I’m satisfied that its use is justified.

[...]

eval is a bad idea because nearly every time I have seen it used, it has caused unforeseen and unnecessary problems.

The important bits are “unforeseen” and “unnecessary”. Unforeseen because eval has a huge pile of caveats associated with it, a list that I can’t even recall in its entirety without some thought. Unnecessary because the alternatives to eval tend not to require much more work to implement, whereas the problems caused by eval are subtle and nefarious.

[...]

eval is bad because it introduces a lot of subtle security and translation issues, it defeats bytecode caching, it hides syntax and other errors until runtime, it causes action at a distance that’s hard to follow, it defeats syntax highlighting. It just makes your code worse.

You say you made sure not to expose eval() to user input. Great! That's a good first step - but as the quote mentions, that's not at all the end of the list of things you have to think about with eval(). What brings this answer and the others you've gotten together is that eval() is a false economy. It is, at the very least, the incurring of technical debt. Like optimization, the two answers to "should I use eval()?" are "you shouldn't" and "you shouldn't yet."

Brighid McDonnell
  • 4,293
  • 4
  • 36
  • 61
  • I wish I could pick two answers. Thanks, I appreciate getting the background. – br1ckd Aug 01 '12 at 22:24
  • Helping is more important than having my answer accepted. I'm glad it helped you. :) – Brighid McDonnell Aug 01 '12 at 22:49
  • For future reference: what the whole tedious post linked to above misses is that Python already has an ast.literal_eval() method *specifically* for safely evaluating literal expressions! http://docs.python.org/2/library/ast.html#ast.literal_eval – lost May 01 '13 at 18:11
5
config = wx.Config()
checkBoxes = ['option_1', 'option_2']
for key in checkBoxes:
    config.Write(key, str(getattr(self, 'm_checkBox_'+key).Value()))
Jakob Bowyer
  • 33,878
  • 8
  • 76
  • 91
-1

There usually is a better way to write it. In this case, it's locals(), although you may want to have a list of checkbox objects instead of their names and multiple variables in the first place:

config = wx.Config()
checkBoxes = ['option_1', 'option_2']
for key in checkBoxes:
    config.Write(key, str(locals()['m_checkBox_' + key].Value()))
phihag
  • 278,196
  • 72
  • 453
  • 469
-1
config.Write(key, str(locals()['m_checkBox_%s' % key].Value()))

Use locals() for locals, globals() for globals, getattr(obj, attr_name) for getting attr of obj by attr_name.

Jakob Bowyer
  • 33,878
  • 8
  • 76
  • 91
Victor Gavro
  • 1,347
  • 9
  • 13
  • Is there any way to access class variables? I'm declaring the m_checkBox's in a superclass and locals() is only returning variables declared in that function. – br1ckd Aug 02 '12 at 00:17
  • I found the answer, if anyone else is wondering: http://stackoverflow.com/questions/5607307/get-the-list-of-a-classs-variables-methods-in-python – br1ckd Aug 02 '12 at 00:19
  • access class variables using getattr, since class is object too, like everything in python. `getattr(self, 'function')` gives you `self.function` object. You can achieve same result using `obj.__dict__` anyway, but it's better practice to use `getattr`. – Victor Gavro Aug 02 '12 at 01:44
  • I need to be able to get all of the objects whos names match a certain pattern. I think the best way to do that is to get a list of local objects, and the best way to do *that* is with __dict__. As always, please correct me if I'm wrong. – br1ckd Aug 02 '12 at 23:01