2
\$\begingroup\$

Coding to the interface, rather than the implementation. Here is what I'm doing in simple terms.

Note: Although written using PHP, this is more of a general design / abstraction question that developers using any language could help answer.

I'm writing an application that can handle different types of connection to gather it's data. The connection could be:

  • A server somewhere abroad
  • Localhost
  • On a local network

All connections must handle their data retrieval using SFTP as the data may be sensitive.

Therefore, I coded an interface as follows:

The Interface

interface ConnectionInterface 
{
    /**
     * __construct instantiates connection object with settings
     */

    /**
     * connect() attempts connection using settings set up within constructor
     */
    public function connect();

    /**
     * runCommand() executes a cmd and retrieves a string using the connection resource
     */
    public function runCommand($command);

    /**
     * ping() checks that the actual host exists, before trying to connect
     */
    public function ping($ip);
}

Above is the interface I created before coding anything else.

Here is one concrete implementation that requires SSH2 via PECL to be installed.

SSH2 Implementation

class SSHConnection implements ConnectionInterface
{
    public $config;
    public $conn;


    /**
     * Instantiates SSHConnection Object with settings
     */
    public function __construct($ip, $port, $username, $password)
    {
        // Check SSH2 extension loaded. If not, throw exception / exit

        // Set class variables to that of those passed into constructor
    }

    /**
     * Attempt SSH connection using settings set up within constructor
     * 
     * @return mixed True on success, false on exception
     * @throws \Exception if can't find server or can't connect
     */
    public function connect()
    {
        extract($this->config);

        if (!$this->ping($ip)) 
        { 
            return false; 
        }

        if (@!$conn = ssh2_connect($ip, $port))
        {
            throw new \Exception('Unable to connect to the server');
            return false;
        }

        if (@!ssh2_auth_password($conn, $username, $password)) 
        { 
            throw new \Exception('Incorrect server login username / password');
            return false;
        }

        $this->conn = $conn;

        return true;
    }

    /**
     * Simple server ping using exec(), used in $this connect()
     */
    public function ping($ip)
    {
        // Ping.. obviously..
    }

    /**
     * Execute a command and retrieve a string using the connection resource
     * 
     * @param string $command The command to run (example: 'ln -s')
     * @return mixed An object containing arrays data
     */
    public function runCommand($command) 
    {
        // Use $this->conn to run a shell command
    }

    /**
     * If there is a connection, runs exit on it then unset
     */
    public function __destruct()
    {
        // If connection, ssh_exec 'exit' then unset($this->conn)
    }
}

My questions

Apart from giving a load of general suggestions about why and how to improve this code, I have a few more specific questions that I hope could be added to your answer:

  1. Is it wrong to perform any sort of calculations within the constructor? For example, if the SSH2 extension isn't loaded, isn't the constructor the clearest place to put the check for this? Someone suggested creating a "test if you can run this class" command line script, but... honestly I think that someone trying to create this object should not be allowed to if they don't have the required dependency.

  2. Should the ping() function be here? Ping is only required when using a server abroad, not really if on localhost. Should there be a check here? Should this be moved somewhere else entirely? How should I go about it?

  3. What about the actual data retrieval? This is just the connection. Should I insert the data retrieval within this class? Or create a new DataRetrieval object which uses the connection object?

Your help will be greatly appreciated. This is not a work project, just a personal one and I asked this question to learn - I know it'll work, but I want it to work well. So what would you do and why?

\$\endgroup\$
1
\$\begingroup\$

Is it wrong to perform any sort of calculations within the constructor?

Opinion is somewhat divided on this matter. I think it is generally accepted that a constructor should not contain business logic because it makes your code much more difficult to mock for testing, however this particular case you show comes down to how you define that term "business logic".

Personally I believe that checking for global dependencies (in this case, checking for the existence of the SSH2 extension) is acceptable. Ideally you would not need to be checking for any global environmental state, you would simply inject a state object, but because of the way PHP's extension system works that's not really possible. Obviously injecting state object is possible, but that would still need to check global state, so you haven't really gained anything, except possibly in terms of SRP - but this would require an added layer of abstraction to separate the protocol from the connection. It's up to you whether you think this is worthwhile.

The alternative is to put this check in a separate method and require that the consumer call it explicitly. However, to me this is at odds with the idea of the interface and is defining and exposing the underlying implementation.

There are only two cases that need to be mocked: the dependencies exist, or they don't. There's no real logic that needs to be tested here. What you definitely should not do is automatically connect in the constructor, but I think that dependency validation in the constructor is harmless.

However, I know there are others who would disagree with me on this point.

Should the ping() function be here?

No, the ping function should not be there. We have already had a conversation about this in chat, but sum it up in a sentence: The ping function is there to validate that the host is connectable, this should be done internally by the connect() method and not by a separate external API call. This is exposing part of your implementation in your interface, exactly what you are trying to avoid.

What about the actual data retrieval?

It depends. If the class contains a send() mechanism, it should also contain the retrieve() mechanism. But it may be that this should be divided up a bit more:

/**
 * Value object that just holds the connection parameters like host, port, protocol
 *
 * It may be that this is simply a concrete implementation and an interface isn't required
 */
interface ConnectionParameters
{
    // ...
}

interface Connector
{
    /**
     * Uses a parameters object to create a connection object
     *
     * @param ConnectionParameters $parameters The parameters to use
     *
     * @return Connection The created connection
     *
     * @throws \RuntimeException When the connect operation fails
     */
    public function connect(ConnectionParameters $parameters);
}

/**
 * Represents an active connection
 */
interface Connection
{

    /**
     * Get the connection parameters used to create the connection
     *
     * This is optional, but personally I believe it makes sense to carry this
     * information with the connection. Obviously in order for this to be implemented
     * the object will need to be passed in by the Connector.
     *
     * Some may say this is inviting LoD violations and that the association, if
     * required, should be carried by the consumer.
     *
     * @return ConnectionParameters Parameters used to create the connection
     */
    public function getParameters();

    /**
     * Send data from a buffer
     *
     * @param DataBuffer $buffer Buffer that holds data to send
     * @param int        $length Number of bytes to send (<0: drain buffer)
     *
     * @return int Number of bytes sent
     */
    public function send(DataBuffer $buffer, $length = -1);

    /**
     * Receive data into a buffer
     *
     * @param DataBuffer $buffer Buffer to populate with received data
     * @param int        $length Number of bytes to receive (<0: all pending data)
     *
     * @return int Number of bytes received
     */
    public function recv(DataBuffer $buffer, $length = -1);

    /**
     * Close the connection
     */
    public function close();
}

/**
 * Represents a store of data that can be transmitted via the connection
 *
 * You may wish to add other methods to this interface, for example an fgets()
 * equivalent. Arguably though, that might be a case for extending this interface:
 * This assumes all data is binary, you might want to have TextBuffer extends DataBuffer
 */
interface DataBuffer
{
    /**
     * Read some data from the buffer
     *
     * @param int $length Number of bytes to read (<0: drain buffer)
     *
     * @return string Data from buffer
     */
    public function read($length = -1);

    /**
     * Write some data to the buffer
     *
     * @param string $data   Data to write
     * @param int    $length Number of bytes to write (<0: all pending data)
     *
     * @return int Number of bytes written
     */
    public function write($data, $length = -1);
}
\$\endgroup\$
  • \$\begingroup\$ Quick question Dave - Is the intent here to create the Connector Object, then pass it via dependency injection into the constructor of the Connection object? \$\endgroup\$ – James Apr 9 '13 at 9:09
  • \$\begingroup\$ @Jimbo The idea of the connector is to act as a service creator. So you could have a SSH2ExtnConnector and a cURLSSH2Connector, which would perform their relevant connection tasks and instantiate different implementations of Connection. Which one you would use would depend on what is available on the current platform. By doing it this way you can simply inject the correct Connector implementation into the code that consumes the SSH2 service, and the underlying implementation is effectively abstracted. \$\endgroup\$ – DaveRandom Apr 9 '13 at 9:13
0
\$\begingroup\$

Consider creating a static method within the class that performs all requirement checks and either returns the constructed object or NULL to indicate there was a failure in meeting all requirements.

class SSHConnection implements ConnectionInterface
{
    //.... your class functions
    public static function SSHConnection GetInstanceIfRequirementsMatched(/*...args*/)
    {
        $myObj = NULL;
        $bIsAllGood = TRUE;

        //run checks for installed features, example:
        if(!isset($_SERVER['SSH'])) { $bIsAllGood = FALSE; }

        if($bIsAllGood)
        {
             //initialize object, run connection obj, etc.
             $myObj = new SSHConnection(/*...args*/);
             $response = $myObj->InitConnection();

             if(!$response) 
             { 
                 /*throw new Exception("Could not connect...!");*/ 
                 $myObj = NULL; 
             }
        }

        return $myObj;
    }

    protected function __construct(/*...args*/) { }

    /*Basically your connection() method*/
    protected function InitConnection() { return 0; }
}

Note: for this, I'd also suggest making your constructor protected instead of public to enforce usage of your static method outside the class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.