code reorg: less inheritance, more delegation/composition #2782

New Issue

warner · 2016-04-27T21:27:45Z

warner commented

2016-04-27 21:27:45 +00:00

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.

In the comments on [PR270](https://github.com/tahoe-lafs/tahoe-lafs/pull/270), 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"](https://www.youtube.com/watch?v=3MNVP9-hglc) from [PyCon](wiki/PyCon)2013, 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.

warner added the

labels 2016-04-27 21:27:45 +00:00

warner added this to the undecided milestone 2016-04-27 21:27:45 +00:00

meejah commented

2016-04-27 21:42:49 +00:00

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.

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.

Sign in to join this conversation.