My code nests too deep. Is there a better way?

Question

This particular code I wrote in another question recently, and I'm not sure it's optimal. I couldn't find a less-indented way of doing this though. Is there?

def msg_generator(self):
    ''' Provides messages until bot dies '''
    while self.alive:
        for msg in self.irc.recv(self.buffer).split(('\r\n').encode()):
            if len(msg) > 3:
                try: 
                    yield Message(msg.decode())
                except Exception as e:
                    self.log('%s %s\n' % (except_str, str(e)))

I keep hearing that nesting too much is bad, but this seems to be necessary. It's currently four indentations deep.

Answer 1

Off the top of my head, you can do this:

def msg_generator(self):
    ''' Provides messages until bot dies '''
    while self.alive:
        for msg in self.irc.recv(self.buffer).split(('\r\n').encode()):

            if len(msg) <= 3:
                continue

            try: 
                yield Message(msg.decode())
            except Exception as e:
                self.log('%s %s\n' % (except_str, str(e)))

Or you could refactor into functions.

def msg_generator(self):
    ''' Provides messages until bot dies '''
    while self.alive:
        for msg in self.irc.recv(self.buffer).split(('\r\n').encode()):

            if not len(msg) > 3:
                continue

            yield handle_message(msg)

def handle_message(msg):
    try: 
        return Message(msg.decode())
    except Exception as e:
        self.log('%s %s\n' % (except_str, str(e)))

Or use something like this:

from itertools import imap

def msg_generator(self):
    ''' Provides messages until bot dies '''
    while self.alive:
        it = iter(self.irc.recv(self.buffer).split(('\r\n').encode()))
        return imap(handle_message, (msg for msg in it if len(msg) > 3)

def handle_message(msg):
    try: 
        return Message(msg.decode())
    except Exception as e:
        self.log('%s %s\n' % (except_str, str(e)))

The last option is not perfect, because if there is an exception then the func will return None which is not a real message, so You could also filter that afterwards using filter() or have the other end handle None msg's.

Answer 2

the problem with the code above is not so much that it is nested too deeply, it is more that the code is too extensional, not enough intentional. by this i mean that while the cogs and wheels and levers that you need to assemble your machinery are in place and (presumably) do work, there are too few labeled modules in sight—there are too many concerns 'complected' in too many implicit, unlabeled ways, resulting in hard-to-read code (for that 'complect' part, do you the favor to listen to http://www.infoq.com/presentations/Simple-Made-Easy —it's a very enlightening talk, and it puts the finger on the problems with your code).

let me give an example:

...
for msg in self.irc.recv(self.buffer).split(('\r\n').encode()):
...

i take it you're talking about 'messages' here. so why not call that variable message? why abbrv. that word? message is so much clearer.

now it's for message in xxx. what is xxx? well, you already know. in Pythonic English, it is true, most of the time, that for apple in appples; for pear in pears should be a good, reasonably semantic way of writing things: the for ... in loop singles out elements, one at a time, out of a collection of elements, so for <singular> in <plural> is ok most of the time. which gives us

...
for message in messages:
...

where do those messages come from? according to your code, they are the result of doing self.irc.recv(self.buffer).split(('\r\n').encode()).

now that's a bummer. for starters, ('\r\n').encode() is definitely the same as '\r\n'.encode(), which is one level of parentheses less. now this is certainly a constant, so why compute it anew on each turn? it might also be better to decode all of the buffer return by irc.recv() instead of first splitting and then decoding line-by-line.

but that's secondary to the point in question. importantly: whatever low-level code is needed to give you the next list of lines from irc, it should probably not appear in this spot. rather, something like

messages = self.irc.get_messages()

or maybe

messages = <SOME_LIBRARY>.get_irc_messages( self.irc )

would be appropriate. the details are, of course, subject to many contentious discussions. personally, i try to not conflate my data—stateful objects of mostly generic types with no functionality tacked unto them—on the one hand and my libraries—stateless collections of named pieces of functionality—on the other. the Simple Made Easy presentation linked above makes that exactly same point, btw, and manages to argue convincingly, too. (aside: of course this flies into the face of current mainstream OOP thinking—but google for Yegge's Execution in the Kingdom of Nouns if you believe OOP ≡ good n'importe quois.)

the present method is all about answering messages, not receiving them. think of it like a Yours Truly column in a newspaper: here we are in the office that thinks up and compiles answers to be sent out to the readers; those who open the incoming papermail, pick up the telephone and sift through the email have a whole different job taking place in another office. therefore, how to open an envelope should not be something you should implement in this spot. outsourcing is required.

whether or not you want to make that messages variable explicit or conflate it to an inline API call depends on a number factors like space available and whether you want to recycle that return value at another point in the routine. this looks fine to me:

...
for message in self.irc.get_messages():
...

the for ... in will transparently work with generators and list-returners alike, which is good.

always think of your code in terms of the most general, yet unbloated, most modularized, yet not 'atomized' ways.

if i was to write test cases against your code, i would definitely single out that irc.recv part and test it separately. think of your code in this way: what are good modules, each with a single concern and a reasonable level of isolation, sound call parameters, that make for good testability? what are reasonable names for the part of my code?—if you keep to this, your code will just stop nesting too deeply, all by itself.