Scanning other module: bad code design or clever trick?

Game Engine Game Engine Support and Discussion
#1

In game, I have an interactive console. You can type commands into it and it will run them.
Instead of manually putting each command inside the console.py file, I have a clever piece of code:


CONSOLE_COMMAND_DICT = {}
for module in sys.modules:
        if 'CONSOLE_COMMAND_DICT' in dir(sys.modules[module]):
            CONSOLE_COMMAND_DICT.update(sys.modules[module].CONSOLE_COMMAND_DICT)

It scans through all modules currently loaded in the game engine and looks for a dictionary ‘CONSOLE COMMAND_DICT.’ It then combines them all into a single dictionary which is used to map words into function calls.

So in another module, say ‘objectives’ I can have:


CONSOLE_COMMAND_DICT = {
    'addObjective':(addObjective, 'Adds an objective for the user to navigate to.'),
    'listObjectives':(listObjectives, 'Lists all objectives'),
    'removeObjective':(removeObjective, 'Removes the specified objective'),
    }

And so on, scattered throughout the various modules I’ve written.

Now. this could be good design because:

  • It keeps code where it belongs. Code to edit objectives should be with the objectives
    But it could be bad design because:
  • Things magically appear in the command dictionary.
  • Code to do with the console is scattered everywhere.

Now I’m planning to do something similar with settings. A single script manages reading and writing to a settings file, but scans other loaded modules for the settings it needs to save and what values they should have.

Is this good design, bad design or just plain strange?

#2

Well in my opinion is bad design (not horrible tought). As you say “Code to do with the console is scattered everywhere.”, this for me is the main drawback. If some day you want to disable the console, you will not free any memory for that, scanning all modules is slow, and things get messier this way.

You say your main reason for this is “Instead of manually putting each command inside the console.py file”, but your solution is just “manually put each command inside an apropiate .py file”, so the amount of work is the same if not more. In some situation there may be confusion of wich is the apropiate .py file, isn’t the command.py file the apropiate file for commands anyway?

Well in any case as long as you have some design it should be fine.

Ahh and by the way, even as a bad design, I still think it’s a clever trick.

#3

Why not.

I assume CONSOLE_COMMAND_DICT is some sort of configuration.

Your first code snippets registers all of your dictionaries within the loaded modules. But, it will miss any module that is not loaded yet. IT can be hard to find out when another potential module gets loaded that you rerun the registration.

Alternative
Another way would be that they explicitly register themselves when loading. You already set them up as “registerable” by adding the dict with the name CONSOLE_COMMAND_DICT. So I guess it is no big deal to do the same thing in a different way.

I think implementing a central registration, makes it easier for you. You can use any name you like or even multiple registrations per module. The registration can happen as soon as a “registerable” module is loaded (dependent on your implementation).


import consoleRegistry
CONSOLE_COMMAND_DICT = {
    'addObjective':(addObjective, 'Adds an objective for the user to navigate to.'),
    'listObjectives':(listObjectives, 'Lists all objectives'),
    'removeObjective':(removeObjective, 'Removes the specified objective'),
    }

consoleRegistry.register(CONSOLE_COMMAND_DICT)

or


import consoleRegistry

consoleRegistry.register(addObjective, 'Adds an objective for the user to navigate to.')
consoleRegistry.register(listObjectives, 'Lists all objectives')
consoleRegistry.register(removeObjective, 'Removes the specified objective')

Regardless of that the approaches you chose, you need to deal with the situation of a key registered twice.

I hope you get the idea.

#4

If I were to put everything inside console.py then the top of console.py would look like:


import bge

import objectives
import settings
import audio
import player
...
..

And it would have a dozen functions like:


def listObjectives(*args):
    out_str = ''
    for obj in objectives.getlListOfObjectives():
        out_str += obj + '
'
    return out_str

def addObjective(*args):
    for arg in args:
        objectives.addObjective(arg)

and at the bottom it would have a dictionary containing all the commands anyway.

What this does is mean that if I make a change to the behaviour of getListOfObjectives, or if I replace it with getDictOfObjectives, I then need to go and edit console.py. The method of displaying the list is not within the objectives module anymore.

By scanning through, it means that the commands relating to objectives are in the objectives file.

console.py should contain code relating to the console: displaying, parsing commands etc. It should not contain code that operates on those commands. As a result it needs some other way to populate the commands. On a Unix system, this is done by looking in $(PATH). In effect, scanning the modules is like searching the whole directory tree for any executable.

I now think that better design would be to have a function call ‘console.registerCommand()’ as there is far less magic involved and the code stays in it’s related modules.
— edit —
And that’s exactly what Monster suggested. He bet me to it while I was writing this post!

#5

Hey, and why not forget about everything and give the user complete control using exec and globals()? Besides importing the modules you wouldn’t need to do anything and you would have all the advantages of python in your command system.

What this does is mean that if I make a change to the behaviour of getListOfObjectives, or if I replace it with getDictOfObjectives, I then need to go and edit console.py. The method of displaying the list is not within the objectives module anymore.

Or you can have getListOfObjectives in the objectives.py file and it’s entry on the commad list on the command.py file. You will have to change the name in the dictionary if you change it on the definition, but that won’t be a problem considering that you will have to change it too in any other call of that function.

#6

Hmm. I hadn’t thought of giving the user a complete python shell in-game. In this particular use: SECURITY HOLE!!!
Yeah, uh.

#7

Sorry if it is a sarcasm, it is particullary hard to understand it on the internet. Proably we should start indictaing it with an underline. :confused:

There is no security hole becouse the code is been writted by the user. I mean, of course he can break everything, but you’re giving him permision to do that, it is his responsavility. The problem is when you use exec but you don’t want the user to use it, so in this case there is no problem.

#8

I generally feel that you should avoid code duplication, and that helper tools can often be very useful. However, module scraping is something that I feel is somewhat tricky. For all the time that you save in declaring things in the same module, and the cleanliness of doing so, in the most naive implementation you lose quite a lot of control.

For example, here you have each module with provides some features X, Y and Z, but most of the time you can’t logically separate a game into distinct logic modules. For example, although you can have your audio module, etc, typically these modules require some level of interaction with one another. In short, code modules tend to be organised and divided in a manner that does not directly map to logical category. The console command scraping assumes that your modules are logically divided. Imagine trying to add some kind of restriction on the kinds of commands that could be executed, or adding some overarching commands that aren’t exclusively audio or physics. These kinds of changes are common enough, and generally speaking, I advise not to restrict yourself to a particular design constraint until as late as possible (often, you can avoid this altogether).

Monster’s suggestion is that you sidestep the scraping code and have the modules register their API to a separate module. This is probably slightly better as you could more easily add more parameters to the registration system, but would still require that you import all the modules at least once before the console commands are used, in order for the commands to be available. With this constraint in mind, why not simply do this explicitly? You’re writing modules that offer an interface to other modules, why not simply define the console commands as clients of that interface? The commands don’t care how the API is implemented, just that they can call it, and as such, it is probably more sensible to move them out of the specifics of the implementation.

Here’s what I would do:

  1. Create a console factory. This produces a console object.
  2. Within the factory, register the available commands.
  3. Return the console object.

If you wanted to add support for permissions, just modify the interface. Rather than placing permissions code inside the command (Which makes it hard to predict if the command could be called successfully), you could use a decorator (which directly associates it with the function), function annotations (which aren’t a great usage of the API here) or, if you want to separate the what from the if, even as part of the registration API.


console.register_command(command_string, command, access_policy)

Here we separate the how (command string) from the what (command) and the if (access policy) which already adds support for access policies, command aliasing, mapping of available commands according to the policy etc.

You probably don’t need access policies.

But, it’s a fun thought experiment. To elaborate on my aversion to “fancy” Python - I wrote my original network library in a manner which heavily relied on metamagic. Great fun, but a real pain to manage. Import order mattered, certain classes returned instances of others when instantiated and when I came to add scene support, I added a context manager paradigm that was very fun, but in the end left my code looking like a desperate attempt to avoid explicit, predictability, which was far easier and nicer to use.

In short, if you come back in two months and want to immediately list where all of your console commands are, and check what you’ve defined, you have to touch several modules, and that’s a pain!

Just my thoughts :slight_smile: