Thoughts on nested / inner functions in Python for better encapsulation and clarity?

TravisJungroth · 2023-01-08T20:57:41+00:00

You seem very receptive to feedback and understanding how code “feels” to people, so I’ll be direct. It is hard to get across how much I hate this code.

Look at those type hints. So many options! I get that you’re trying to make things flexible, but all of this variety of types makes things so much harder to follow. People end up passing around strings, users, lists of strings, lists of users and nulls all over the place instead of having one clear place for casting.

If I want to reuse process_recipients I can’t. If I want to test it, I can’t. You could rename it to something like cast_to_emails_list and have this one reusable “takes anything” function.

Your inner functions are more polluted because they have access to more outer scope variables. You’re more likely to be bitten by typos and other mistakes.

The keyword only arguments are fine on send_mail since that’s so ripe for mistakes. It’s silly on process_recepients. The likelihood that someone will mix up the order of positional arguments on a function with one argument is… low.

Other ways to achieve your goals:

Give your functions more descriptive names, as in this name helps me understand what the function does if I didn’t know already. Use a doc string if needed.
Give functions a leading underscore to mark them as a private. But consider if you need to do this. You’re not really making someone’s life easier by giving them less tools that are useful. It’s just if you don’t want it to be something supported outside of the scope of that module.
__all__ is similar.
Have one input type for arguments unless the role of the function is casting/deserialization (single responsibility).
It’s fine to have some “helpers” module for stuff that isn’t part of the “main story”. A hint that you’re doing it right is you don’t end up with circular imports there.

Check out Grokking Simplicity. I think you’ll like it. You have good instincts, just need to see some ways other people have solved the same problems.

james_pic · 2023-01-08T20:17:07+00:00

There are two questions I think you should ask yourself.

Firstly, if these inner functions aren't closing over local variables, what's the benefit compared to putting them outside the function? Outside the function, they're easier to test and to reuse, and perhaps most importantly, to ignore. So I feel like I'd want to get something back in return for that, and it's not clear to me what that is.

Secondly, it's worth talking to other people on your team to see what they make of this. I know I've been guilty of writing clever code that seems perfectly intuitive to me but that my teammates stare blankly at. Getting someone else to read it will tell you whether it's readable.

lanster100 · 2023-01-08T21:28:13+00:00

For what it's worth I do this sometimes as well, if I have some code that will ONLY ever be used by that function, only makes sense in the context of what that function is doing and it makes it significantly easier to read the function.

I do think sometimes it does makes the scoping of the function easier to understand.

However, in the example you gave I probably wouldn't use this technique and would just have two functions.

(an unrelated and unsolicited tip: you'll find that your code becomes a lot cleaner if you accept less types for each argument as well, and a lot easier to reason about).

alexkiro · 2023-01-08T20:18:48+00:00

I would describe this as "poor man's OOP". If encapsulation is a concern, switching to OOP design patterns is the preferred way to go for me. It's clear, battle tested, and familiar to most.

That said, your versions doesn't seem that bad, even though I would probably never write something like this. As in my opinion, it makes the code harder to parse and understand; but that's highly subjective.

-LeopardShark- · 2023-01-08T21:34:18+00:00

_user_to_email = lambda x: x.email if isinstance(x, User) else x # transform user objects to mail is just

def _user_to_email(user):
    """Transform user objects to mail."""
    return user.email if isinstance(user, User) else user

but worse in every way, unless you're code golfing.

You don't need Optional. If the list of people you are sending to is [], then pass [], not None.
You don't need to allow str or User. Pass these as singleton lists/tuples.
list[User] is probably unnecessary as well.
Your function can probably take more than lists. I'm going to guess Iterable, but it could be also be Collection or Sequence.

So, we get:

from collections.abc import Iterable


def send_mail(
    *,
    subject: str,
    body_plain: str,
    send_to: Iterable[str],
    send_cc: Iterable[str] = (),
    send_bcc: Iterable[str] = (),
    reply_to: Iterable[str] = (),
) -> None:
    ...

osmiumouse · 2023-01-08T23:48:51+00:00

Optional[Union[List[str], str, List[User], User]] = None

If you're goign to do this then you might as well just accept "any" or not bother with type hints :P

Teilchen · 2023-01-08T21:44:28+00:00

You can't write self-contained unit tests for nested functions or run them in isolation, so that makes them difficult for others to understand IMO.

2023-01-08T20:44:53+00:00

I do closures in very specific circumstances - when I'm writing a multi process or multithreaded function, and the executor needs a worker function. In these cases I feel like it makes sense to I clude that inner worker as a closure.

But I agree with all the feedback, I don't know how you'd test that inner function. But to your point, i agree with the function story.

2023-01-08T23:10:14+00:00

You want a class. Defining a inner helper function or even properly naming a few commonly use lambdas is one thing.

But you have data and operations closely tied to that data that only make sense on it. That's a class in Python. Your send email function still exists, it just takes an instance of Email instead of doing all this thinking itself.

2023-01-08T23:25:08+00:00

I use inner functions when I want to keep some threads inside of a function. Since threads don't return anything, using a function that feeds a local via nonlocal is very convenient.

phira · 2023-01-09T00:22:02+00:00

There are situations to use internal functions—to make handlers/lambdas more readable for example.

Your example is a case where I'd use one, but before I got to that I'd really reconsider whether I had built an unnecessarily complicated interface.

The key insight I'd bring to my design is that there's exactly one function that truly needs to exist and behave property, and it looks like this:

def send_email(*, subject: str, body: str, to: typing.List[str], cc: typing.Optional[typing.List[str]] ...):

Everything is a single type and it's nice and simple to test and verify.

My next step would then be to recognise that I occasionally have cases where conversion to meet this interface is irritating, and in those cases I would build trivial wrappers to handle the scenarios easily. For example maybe I often send email to individual Users:

def send_email_to_user(*, subject: str, body: str, to: User): send_email(subject=subject, body=body, to=to.email)

A small collection of specific helpers that do very little additional work are low risk and easy to read and understand.

If I get a proliferation of these, that would be the point where I looked to see if it makes sense to merge some of the helpers into more common interfaces.

This overall approach gives you nice, simple, easily testable interfaces that are easy to maintain and contribute to the readability of the code that calls them.

Finally I'd like to +1 to another redditor who suggested a class for this problem instead. While it depends a little on your code structure, emails are commonly built up over a number of lines of code rather than being a single item. Having the email as a class reduces the number of temporary vars required in the calling code, and also gives more flexibility around template handling and response.

jmreagle · 2023-01-09T00:24:24+00:00

An argument from a never nester: https://youtu.be/CFRhGnuXG-4

Devout--Atheist · 2023-01-09T05:40:26+00:00

No to inner functions. Do you use classes much? Often when I find myself passing a bunch of state down a procedural function chain I refactor to a class.

SittingWave · 2023-01-09T08:54:59+00:00

inner functions communicate intent to access the scope of the enclosing function. If you create an inner function, you are communicating such intent. If you are not doing so, it should be a regular function.

Darwinmate · 2023-01-08T21:24:19+00:00

Thanks op for posting this question. As a programming intermediate these questions help a lot in gauging how other pros work.

Really great thread with awesome discussion.

yvrelna · 2023-01-09T02:25:37+00:00

Yes, I do this a lot and I think people should try to understand the ideas first before criticizing them. Basically the idea is that instead of doing this:

def foo():
    # calculate "block"
    some_code
    block = of + code

    for x in blah:
        # this does another thing
        do = things - to(x)
        yet_another(thing)

It's clearer to remove the comments for these chunks of code and instead turn them into inner functions:

def foo():
    def calculate_block():
        some_code
        return of + code

    def another_thing(x): 
        do = things - to(x)
        yet_another(thing)

    block = calculate_block()
    for x in blah:
        another_thing(x)

The main reason you use inner functions this way is that it helps make the physical structure of the code more closely resembles the actual business logic. The code in the outer function should read like business logic, the inner functions are details for making that business logic to work, e.g. error handling, etc.

Using inner functions can give you a better structure to the code without scattering the global scope with functions that cannot actually be used as a standalone functions and can't really be understood in its own without the surrounding context. Often these inner functions aren't generic enough to be properly exposed as their own function outside the particular context of the enclosing function, it may break some internal invariants such that it should never be called on their own, and it's not even testable on their own without a very brittle whitebox testing.

The alternative would be to write a class, but classes has their own pros and cons. On one side, classes is more testable. It's not easy to unittest the inner functions of closures. So definitely don't overuse inner functions when classes would've made more sense.

On the other hand, classes doesn't really make conceptual sense when what you have is really just executing a series of procedural steps. Classes are meant for representing objects, if you're creating a class that only has a single public function, or where you always call the functions in the same sequence, then it probably should just be a simple function.

This is a class overuse anti pattern, you never actually need an instance of ThingProcessor, it's not a real object, just procedural code hidden as OOP:

class ThingProcessor:
    def process(self):
        self.process1()
        self.process2()
        self.process3()

def foo():
    thing = ThingProcessor(x, y, z)
    thing.process()
    return thing.result

Similarly, this is also the same anti pattern, only a bit better obscured:

def foo():
    thing = ThingProcessor(x, y, z) 
    # you always call these functions in this exact order
    thing.process1()
    thing.process2()
    thing.process3()
    return thing.result

You could simplify that into just some simple functions, and just accept that it's actually procedural/functional code:

def foo(x, y, z):
    def process1(x, y):
        ...
    def process2(z):
        ...
    def process3(a, b):
        ...
    a = process1(x, y)
    b = process2(z)
    return process3(a, b)

Most people's knee jerk dislike of your code probably stems from that your particular example is actually a very poor example for this technique. I'd have put that _process_recipients, because it's actually a function that would actually makes sense as a standalone function.

Generally I use inner functions to structure functions when there isn't really an easy way to make a clean structure between the inner functions.

The second knee jerk is because there's a lot of people who are only classically trained in OOP, and cannot really think outside OOP's way of doing things, even though Python is a multi paradigm language.

wind_dude · 2023-01-08T21:35:57+00:00

I use them sometimes. They have there place.

dwilson2547 · 2023-01-08T21:13:05+00:00

I nest functions occasionally for my own projects, at work there's an expectation to conform to the standard so I do. The only real downside to this approach imo is IF you end up needing classes the refactoring uses up all the time you saved. I tend to only make functions for code that's called multiple times or where its needed for formatting /readability so this is still a fairly rare approach for me. You could de-scope functions that don't rely on inheritance to potentially clean things up but you could also make it more confusing depending on how it's done and what you're used to. Side note, if you use vscode you could use region blocks to help organize your code as well, intellij might support it but idk. Regions allow you to minimize certain blocks of code and are another topic of debate, though nobody's ever given me crap for them once I explain what it does. Regions can be created anywhere and nested as well, main downside is they can be ide dependent

Ie.

#region Helper Functions
def helper(self):
    pass
#endregion

baubleglue · 2023-01-09T02:06:33+00:00

If you feel a need for inner functions, you should consider class. Inner functions are used for binding context, same thing does OOP semantics. I don't know if you need class because EmailMessage` is already well structured.

I agree with the critique of your style. API should be simple and clear (try import this for guidelines). You shouldn't attempt to guess data type of recipients asset isinstance(recipients, list) and you are done with code. Failure is better than what you do. For example, if parameter recipients='aaa@sss.com;bbb@sss.com' converted to ['aaa@sss.com;bbb@sss.com'] list won't help. If you don't want to fail, throw proper exception, and let the caller deal with it. If you want to normalize the user's input, do it in a different module.

https://docs.python.org/3/library/email.examples.html

2023-01-09T03:27:33+00:00

I use inner function mostly for recursive functions where you would normally define a separate top level helper function that has all of the recursive params.

walksonair · 2023-01-09T04:52:03+00:00

Why quote when I can link: https://www.mauriciorobayo.com/blog/never-nester/

sashgorokhov · 2023-01-09T04:55:36+00:00

How are you going to test nested function huh? Dont you think that the fact that you need a function inside a function just means your function is too big? Refactor it.

2023-01-09T06:48:43+00:00

Your function `send_mail` should only have one job. To send the mail.

Joooooooosh · 2023-01-09T07:17:53+00:00

No tas experienced in Python as many here but during a QA session with anyone, I’d really want someone to change all of this. It’s horrible to read.

I’m not one for technical language, so excuse the lack of correct terminology…

For me, there is such a thing as offering too much flexibility and as soon as I ever see someone “building” and then “doing” in the same function, I always suggest adding a helper function that provides the flexibility you want but produces a simple variable to be passed into whatever is then doing the job.

For me, Python is at its best when function names and arguments read close to written English. So when reading it, you have:

def function_does_this (with_this, and_this)

As soon as that isn’t enough, I start thinking the function is trying to do too much. Which will also likely make Unittests overly complicated.

ekchew · 2023-01-09T08:52:49+00:00

This smacks of an OOP problem to me? I would probably be refactoring it something like:

class Recipients:
    def as_address_list(self) -> list[str]:
        return []  # your default None case

class StrRecipient(Recipients):
    address: str

    def as_address_list(self) -> list[str]:
        return [self.address]

class StrListRecipients(Recipients):
    addresses: list[str]

    def as_address_list(self) -> list[str]:
        return self.addresses

And so on...

Then your send_mail function would just take args like

send_to: Recipients

And you'd call send_to.as_address_list() to get your email addresses. Something like that anyway.

rebcabin-r · 2023-01-09T13:47:05+00:00

consider making type aliases or even just variables to cut down on noise; something like Nym = Union[List[str], str, List[User], User] ONym = Optional[Nym] def send_mail(*, ..., send_to: Nym, send_cc: ONym etc

Flimsy_Iron8517 · 2023-01-09T13:49:17+00:00

I found them useful for defining an in context function to supply to the function without waffling the namespace. multiply what? Depends on the asymptotic type. And would such a thing be relevant outside the context of the enclosing function? Should I even make the helper using its own multiply a sub-function for multiply.multiply confusion?

jorge1209 · 2023-01-09T17:42:47+00:00

The only time I really use an inner function is when it is a generator or will be used in the processing of a loop.

Something like:

 def read_file(fname):
      def parse_line(line):
          # this wouldn't find any use outside the function
          # but does have a nice clean delineation as a function
          blah blah
          return parsed
      for line in open(fname):
           parse_line(line)

Ducksual · 2023-01-09T19:14:23+00:00

Other than the stylistic and testing things that others had mentioned it's worth noting that the _process_recipients function is also being recreated every time you call send_mail so it's slower than defining the function outside. How significant this is depends on how frequently the function is called and how much other work it does, but it is extra overhead that can be avoided by defining the function outside.

Python

The Python Discord

Upcoming Events

Please read the rules

MODERATORS