Code style - 'hiding' functions inside other functions

Question

Recently I've been doing things like this:

import Tkinter
class C(object):
    def __init__(self):
        self.root = Tkinter.Tk()
        def f():
            print 'hello'
        self.button = Tkinter.Button(master=self.root, command=f, text='say hello')

as opposed to something like this:

import Tkinter
class C(object):
    def __init__(self):
        self.root = Tkinter.Tk()
        self.button = Tkinter.Button(master=self.root, command=self.f, text='say hello')
    def f(self):
        print 'hello'

The question isn't specific to Tkinter, but it's a good example. The function f is only used as the callback for the button, so I've chosen to define it inside __init__. That way, only code inside __init__ even knows about f's existence - outer scopes don't start getting cluttered up with names, and the user doesn't need to care about a load of methods designed to be internal.

My question is: is this regarded as good style? I'm worrying because I've got a GUI class with quite a lot of buttons - __init__ is starting to look very long, with a lot of local function definitions. Is there a more appropriate alternative that I should be using?

Lierne

Answer 1

The typical way to do something like this in the context of Tkinter is to use a lambda function.

self.button = Tkinter.Button(master=self.root, 
                             command=lambda:sys.stdout.write("Hello!\n"),
                             text='say hello')

At it's core, this is really the same as your first example though, so if you prefer the first way -- Go with it. I think that it is generally not idiomatic to create a new method in this case (unless you actually need the instance to be passed to the callback -- in which case, you should do it the second way).

There are two things to worry about here. The first is readability. If __init__ gets too cluttered to read, then you have a problem. You can move those functions to the module level (prefixed with an _ to keep them from importing in the from module import * context) and use lambda to bind local variables as arguments to those functions if necessary.

If __init__ isn't getting cluttered, then feel free to put the functions in there. Having the functions in there does mean that a new function is created every time a new instance is created (same with lambda) which I suppose could be wasteful as far as memory is concerned, but it shouldn't be that big of a deal (especially in a gui).

The second thing to worry about is namespace cluttering. However, if your module is so big that moving those local functions to module level functions creates a namespace issue, then your module is too big to begin with. As long as you prefix the functions with underscores (see suggestion above), you shouldn't have namespace issues importing this module in other ones either.

Vandalize

You're right, of course, but in the real-world version of my example, f is doing more than just print 'hello'. It won't fit inside a lambda function. – Lierne

@poorsod -- if it isn't using the instance, then it shouldn't be a method. You can make it a module level function, or you can keep doing it the way you are now, but I wouldn't use a method (or even a staticmethod) here if you don't need the instance in the function. – Vandalize

@mgilson: I think you miss the point of the question, which whether callbacks should be nonlocal functions. The reason you see lambda so often in Tkinter callbacks is to bind local arguments to the callback. The callback is often defined elsewhere. – Hazen

@mgilson: What would you say the correct course of action is when the callback does use the instance? At the moment it's getting it from the closure (self is, of course, defined within __init__); I'm sure the more idiomatic way would be to define it as a method, though that causes the proliferation of methods that made me want to put it inside __init__ in the first place. – Lierne

I should add that not all of the callbacks I'm defining are making use of the instance. – Lierne

@poorsod : If the callback uses the instance, make it a method. If it doesn't use the instance, make it a module level function (unless it's small, then use a local function or lambda). There's no harm in making it a module-level function or method as appropriate -- and what's to say that you won't want access to those methods/functions outside your callback later? Small functions are easy enough to move, but big ones involving closures are a bit more tricky. – Vandalize

Answer 2

In this case it's harmless. On the whole, though, I would avoid it for two reasons:

1) The definition of the callbacks as a member of the class is somewhat more readable (especially when you're using pydoc to investigate)

2) Creating the function inside another introduces more closures (variables inherited from the calling context).

Horseshoe

It wouldn't hurt to take this further and define the callbacks in another module, separating the gui from the application logic. – Hazen