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 abroadLocalhostOn a local networkAll connections must handle their data retrieval using SFTP as the data may be sensitive.Therefore, I coded an interface as follows:The Interfaceinterface 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 Implementationclass 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 questionsApart 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: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.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?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?
Abstraction for multiple connection methods
php;object oriented
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);}