#2782 new task

code reorg: less inheritance, more delegation/composition

Reported by: warner Owned by:
Priority: minor Milestone: undecided
Component: code Version: 1.11.0
Keywords: Cc:
Launchpad Bug:

Description

In the comments on PR270, meejah pointed out that Client.__init__ is doing a lot of work, and that it might be better to build these nodes with a function (rather than a class constructor) that is *given* the supporting objects (like an IntroducerClient, StorageServer, and !Tub), instead of creating those things itself. Glyph recently pointed me at an enlightening presentation known as The Talk from PyCon2013, that encourages composition over inheritance, which ties in.

I'm not exactly sure what this would look like, but we could start by either merging Node and Client into a single class, or passing Node in as an argument to the Client constructor. Then we might make a client-creating function that builds Storage Servers and Introducer Clients first, then passes them as arguments into the Client constructor.

Change History (1)

comment:1 Changed at 2016-04-27T21:42:49Z by meejah

The basic pattern for doing this refactoring would look like:

  • anything remotely complex that's created in an __init__ method becomes and arg
  • the "make complex thing" work moves from __init__ to some factory-method
  • repeat recursively ;)

So the last thing is the hard part, especially to avoid a "massive refactor everything" type of branch/PR. I think the first step would be to simply look at dependencies. I don't feel I know enough about Node / Client to know if merging them is appropriate etc. It's fine to leave them as a subclasses for now I think -- especially because Node only really has a couple dependencies in the above model: a tempdir and a tub. So, these would have to be passed into Client too so that super() can get called properly.

It should also be possible to "start slow" and move things out one or two dependencies at a time. From a quick grep it doesn't look like Node() is ever constructed by itself, so we should be able to move its dependencies into args-to-Client without making a create_node factory-method.

So a start would be:

  • introduce a create_client factory-method, taking basedir and tub
  • change Node's constructor to get rid of self.create_tub and (most of) init_tempdir
  • move (most of) Node.create_tub to a factory method (or even just make it @staticmethod?)

This would probably give a good idea of how this would look/work, and would even probably be land-able without anything else changing...From there, it would be a "small matter of programming" to do something similar for Client's dependencies, and a create_client() factory method.

Note: See TracTickets for help on using tickets.