mariano / disque-php

PHP library for Disque, an in-memory, distributed job queue
MIT License
132 stars 19 forks source link

Refactor creating a connection from Connection\Manager into a ConnectionBuilder? #12

Closed Revisor closed 8 years ago

Revisor commented 9 years ago

While working on #8 and refactoring the Disque\Connection\Manager, I realized that, no, we cannot just set a connection object instead of a class name, because we need to create a new connection for each node separately.

We have in my opinion two options:

  1. Leave it as it is now - Manager gets a class name that conforms to the ConnectionInterface and creates its own connections as needed
  2. Or the Manager gets a ConnectionBuilder (name may change) and every time it needs a new connection, it asks the builder for one.

The status quo has these advantages:

  1. It's already there and it works.
  2. It's simple - just set a class name and go.

Refactoring into a ConnectionBuilder has these advantages:

  1. We can use type hinting instead of the "condition + exception" combo: Set a ConnectionBuilder instead of a class name, and further methods would require a ConnectionInterface
  2. The Manager is already doing too much. ConnectionBuilder could make the Manager simpler by taking over all the steps needed for the actual connection:
    • Manager::buildConnection() - the instantiation of a new connection object
    • Manager::doConnect() - the actual connection
    • Manager::getNodeConnection() - the initial HELLO

(There are multiple steps, that's why I suggest the name Builder instead of Factory)

The Manager's role would then be to manage node connections as a group, choose the best node, disconnect the inactive ones etc. That's a complicated enough task, even without having to handhold single connections.

This is more an internal refactoring, not much would change for the library user (unless they want to use a different connection object).

How does it sound to you?

mariano commented 9 years ago

@Revisor sounds perfect. In fact this is a change I had planned to introduce

mariano commented 9 years ago

(I mean +1 on introducing Manager)

mariano commented 9 years ago

Oh ffs, I mean +1 on introducing ConnectionBuilder :)

Revisor commented 9 years ago

Understood :) I'm on it.