Is it a bad programing practice to put a function inside a class method?

Question

I'm looking at a case like this:

def parse_item(self, response):

    item = MetrocItem()

    def ver(string):
        if string:
            return string
        else:
            return 'null'

    item['latitude'] = ver(response.xpath('//input[@id="latitude"]/@value').extract_first())

It works, but is there a better way to do this?

Why don't you define the function outside of the method (indeed even outside the class)? None of its local variables depend on anything in the current scope. — Graipher, Apr 16 '18 at 15:10
This is bad practice since the function is defined again on every call of the method. You should put it anywhere else, like in to the namespace of the module or into a `helpers.py`, or similar. — Klaus D., Apr 16 '18 at 15:11
It is debatable, but it is good practice to keep the scope of variables and functions as narrow as possible. — imreal, Apr 16 '18 at 15:14
@KlausD. Do you have any reference to back up that statement? I would think that the function is defined once and a name is bound to it everytime the method is called. — imreal, Apr 16 '18 at 15:38

score 3 · Accepted Answer · answered Apr 16 '18 at 15:14

3

As @Graipher mentioned in the comments, this is certainly valid in some cases, but in your particular case, it is unnecessary. If your function depends on a local variable, then you're returning a closure that needs to be reconstructed every time you call the method. But in your case, the function is going to behave the same way every time you call the method, so it would make more sense to define it globally as a private method, or even as a lambda in your case.

ver = lambda x: x if x else 'null'

But the preferred approach would be to simply define it globally and start the name with an underscore to make the intention clear.

def _ver(string):
    ...

answered Apr 16 '18 at 15:14

Silvio Mayolo

62,821
6
74
116

1

or just `x = response.xpath('//input[@id="latitude"]/@value').extract_first()` and then `item['latitude'] = x if x else "null"` oh this is @Niklas answer actually... – progmatico Apr 16 '18 at 15:28
It's not going to get reconstructed every time. Code objects will be created for every function, including local functions and reused for every invocation. So in a sense, it's already constructed, just a new reference to that code is created in every call. – Jeff Mercado Apr 16 '18 at 20:10
Ok my code was a bit longer though: but I defined the function outside the class and used it inside the method. – Emiliano Isaza Villamizar Apr 18 '18 at 22:05

score 2 · Answer 2 · answered Apr 16 '18 at 15:16

2

You can get rid of that function completely:

string = response.xpath('//input[@id="latitude"]/@value').extract_first()
item['latitude'] = string if string else 'null'

answered Apr 16 '18 at 15:16

Niklas Mertsch

1,399
12
24

score 1 · Answer 3 · answered Apr 16 '18 at 15:17

There are use cases for having a local function - either in a class method or other method, say if you want a closure capturing something local.

In you case you want something like a colasecing null:

e.g.

>>> s = None
>>> s or "None"
'None'

So, you can use

item['latitude'] = response.xpath('//input[@id="latitude"]@value').extract_first() or "null"

score 1 · Answer 4 · answered Apr 16 '18 at 15:19

It is not bad practice, but here's an alternative.

Since this small function doesn't depend on anything in parse_item and you might want it to be constructed only once, for performance, you could do this:

def parse_item(self, response):
    ...

def _ver(string):
    ...

parse_item.ver = _ver
del _ver

and refer to it inside parse_item as parse_item.ver instead of just ver. This avoids cluttering the global namespace, even with an underscored method that Python partially hides for you.

I would have recommended the lambda expression as another alternative, but Silvio got to it first.

Also, the comments that it could be a global function, visible to all, only makes sense if there's any chance it might be used anywhere else.

Also also, you can eliminate this example completely, as doctorlove and Niklas have suggested, but I assume that you mean this as an example of more complex cases.

score 0 · Answer 5 · answered Apr 16 '18 at 15:15

0

I guess parse_item is a method inside your class. Then you can use Python's "private" methods to make your code more readable and cleaner. Check how to write Python "private" method and then check why it's not actually a private method. ;)

answered Apr 16 '18 at 15:15

Qback

4,310
3
25
38

Is it a bad programing practice to put a function inside a class method?

5 Answers5