should "self" be changed?

[Steven D'Aprano]

On Fri, 29 May 2015 01:01 am, Marko Rauhamaa wrote:

> Anssi Saari <as at sci.fi>:
> 
>> Do you have an example of state pattern using nested classes and
>> python? With a quick look I didn't happen to find one in any language.
> 
> Here's an sampling from my mail server:


I haven't studied this in close detail, but first impressions is that this
is not well-written Python code. The obvious problems that come to mind:


> class SMTPServerConnection(ServerConnection):
>     #:     :     :
>     #:     :     :
> 
>     def __init__(self, server, sock, domain, peer):
>         super().__init__(server, sock)
>         conn = self
>         client_ip = peer[0]
> 
>         class STATE:
>             def __str__(self):
>                 return self.__class__.__name__

A minor criticism: each time you instantiate a new SMTP server connection,
it creates new STATE, IDLE etc. classes. That's wasteful and inefficient
unless you have a good reason for it, although you may not care if you only
have a single connection at a time.

More serious problem: by nesting the state classes, it is much harder to
test the state classes in isolation from a connection.


>             def handle_command(self, cmd):
>                 conn.log("handle_command (unexpected): {}".format(cmd))
>                 assert False

At first glance, this appears to be an abuse of `assert` and risks breaking
your code when running under -O which turns debugging (and hence asserts)
off. Since the intent is to make these abstract methods which must be
overridden, I'd prefer to define an AbstractMethodError, then:

    raise AbstractMethodError("unexpected %s" % cmd)

then do the logging elsewhere.

This has the advantage(?) of eliminating the need to refer to conn, which
means that the methods are no longer closures and the entire inner class
can be moved out of the SMTPServerConnection body.

[...]
>         class IDLE(STATE):
>             def handle_command(self, cmd):
>                 conn.log("handle_command: {}".format(cmd))

Using a closure for information hiding is a solid functional equivalent to
using private fields in an OOP context. However, "private" should be
considered an anti-testing idiom. I would seriously consider this entire
design to be an anti-pattern, and would prefer to one of two alternate
designs:

(1) Pull the state classes out of the SMTPServerConnection class. On
initialisation, each state takes a conn argument. All references to "conn"
are replaced by "self.conn". That's basically the conventional OOP
approach.

(2) Again, pull the state classes out of the SMTPServerConnection class,
except this time there is no need to instantiate the state classes at all!
Decorate all the methods with @classmethod, and have them take the
connection as an explicit argument. The caller (namely the
SMTPServerConnection  instance) provides itself as argument to the state
methods, like so:

    self.state = IDLE  # No need to instantiate.
    self.state.process_ehlo_or_helo(self, other, args, go, here)

This reduces implicit coupling, makes it explicit that the state methods
depend on the connection, avoids reference loops, and enables easy mocking
for testing.

The only downsides are that people coming from a conventional (e.g. Java)
OOP background will freak at seeing you using a class as a first class
value (pun intended), and it will be ever-so-slightly tiresome to have to
decorate each and every method with classmethod.

(Perhaps a class decorator is the solution to that second issue?)

The "design pattern" community is dominated by Java, where classes are not
values, so this idiom is impossible in Java and unlikely to have a DP name.
But it really should have one -- in a language where classes are themselves
values, there is no reason why a class *must* be instantiated, particularly
if you're only using a single instance of the class. Anyone ever come
across a named design pattern that involves using classes directly without
instantiating them?

I'm basically looking for a less inelegant term for "instanceless class" --
not so much a singleton as a zeroton.



-- 
Steven