there must be a better way to do this

Question

This is an ugly, high maintenance factory. I really just need a way to use the string to instantiate an object with a name that matches the string. I think metaclass is the answer but I can't figure out how to apply it:

from commands.shVersionCmd import shVersionCmd
from commands.shVRFCmd import shVRFCmd
def CommandFactory(commandnode):
    if commandnode.attrib['name'] == 'shVersionCmd': return shVersionCmd(commandnode)        
    if commandnode.attrib['name'] == 'shVRFCmd': return shVRFCmd(commandnode)

Maybe you would like to change the title to "Call function by name"? And maybe you can try to google this then? ;) — yo', Feb 23 '13 at 13:14
I may have had a research-fail, but I can say with confidence it wasn't for lack of trying. Thanks for the keyword hints though:) — mnate, Feb 25 '13 at 13:34

score 2 · Accepted Answer · answered Feb 23 '13 at 13:46

2

You can look up global names with the globals() function, which returns a dict:

from commands.shVersionCmd import shVersionCmd
from commands.shVRFCmd import shVRFCmd

# An explicit list of allowed commands to prevent malicious activity.
commands = ['shVersionCmd', 'shVRFCmd']

def CommandFactory(commandnode):
    cmd = commandnode.attrib['name']
    if cmd in commands:
        fn = globals()[cmd]
        fn(commandnode)

answered Feb 23 '13 at 13:46

Ned Batchelder

364,293
75
561
662

One problem with this solution: If `cmd` it *not* in `commands`, it just returns without raising an exception. Easy to fix, but as easy to miss. And you need to add a `return` before `fn(commandnode)`. – Daniel Frey Feb 23 '13 at 14:12
True, but the same is true of the originally shown code. I didn't want to invent a behavior for that case. – Ned Batchelder Feb 23 '13 at 14:16
Just confirming: It doesn't matter that shVersionCmd and shVRFCdm are classes (not functions,) , right? thanks for this. I haven't seen globals used before. – mnate Feb 25 '13 at 13:29

Daniel Frey · Answer 2 · 2013-02-23T13:57:33.817

0

eval is your friend:

from commands import *
def CommandFactory(commandnode):
    name=commandnode.attrib['name']
    assert name in ( "shVersionCmd", "shVRFCmd" ), "illegal command"
    return eval(name+"."+name)(commandnode)

Note that if you are sure that name will never contain any illegal commands, you could remove the assert and turn the function into a no-maintenance-delight. In case of doubt, leave it in and maintain the list in a single place.

edited Feb 23 '13 at 13:57

answered Feb 23 '13 at 13:14

Daniel Frey

55,810
13
122
180

1

eval is rarely a good idea, and introduces vulnerabilites into your program. – Ned Batchelder Feb 23 '13 at 13:38
So, show us *your* solution. `eval` is a feature and yes, it might be dangerous. That doesn't mean it's useless. And you make it sound as if it *always* introduces vulnerabilities, but that is not true, it depends on the source of `commandnode.attrib['name']`. – Daniel Frey Feb 23 '13 at 13:44
No, it's not useless, but it's a big blunt hammer that is easy to overuse. `globals()` would be enough here. – Ned Batchelder Feb 23 '13 at 13:47

score 0 · Answer 3 · edited May 23 '17 at 11:56

0

This answer How to make an anonymous function in Python without Christening it? discusses how to cleanly call blocks of code based on a key

edited May 23 '17 at 11:56

Community

1
1

answered Feb 23 '13 at 13:27

Vorsprung

32,923
5
39
63

score 0 · Answer 4 · answered Feb 23 '13 at 14:33

My personal preference would be to turn the dependencies between the factory and the command implementations around, so that each command registers itself with the factory.

Example implementation:

File commands/__init__.py:

import pkgutil
import commands

_commands = {}

def command(commandCls):
    _commands[commandCls.__name__] = commandCls
    return commandCls

def CommandFactory(commandnode):
    name = commandnode.attrib['name']
    if name in _commands.keys():
        return _commands[name](commandnode)

# Load all commands
for loader, module_name, is_pkg in  pkgutil.walk_packages(commands.__path__):
    if module_name!=__name__:
        module = loader.find_module(module_name).load_module(module_name)

File commands/mycommand.py:

from commands import command

@command
class MyCommand(object):    
    def __init__(self, commandnode):
        pass

Small test:

from commands import CommandFactory

# Stub node implementation
class Node(object):
    def __init__(self, name):
        self.attrib = { "name": name }

if __name__=='__main__':
    cmd = CommandFactory(Node("MyCommand"))
    assert cmd.__class__.__name__=="MyCommand", "New command is instance of MyCommand"
    cmd = CommandFactory(Node("UnknownCommand"))
    assert cmd is None, "Returns None for unknown command type"

there must be a better way to do this

4 Answers4