OO PHP, requesting a look if good practice

Question

I'm starting with OOP for php, i get the idea, but don't have someone physical near me to fallback on so I'm hoping i can do that here.

The following works but i would like to know if it's good practice, that is something that weights a lot for me.

Concept is that you can create an object, the data for this object is gotten trough different soap calls. I have (at this point) a class to create the details and a class to connect soap and get the details.

Here it is: http://pastebin.com/9bgKzATv

So again, just hoping someone is willing and has the time to give it a quick look and just shoot at it so i can learn.

I know i actually should use a dependency in the create, to the soap, but the way it's set up now does make the code cleaner and easyer...

EDIT: i see the code must be included and not a link:

###
# INDEX
###

<?php
function __autoload( $class )
{
    if( !defined( 'ROOT_DIR_CLASSES' ) )
    {
        define( 'ROOT_DIR_CLASSES', realpath( __DIR__ . DIRECTORY_SEPARATOR . 'classes' ) );
    }
    $directory = str_replace( '\\', DIRECTORY_SEPARATOR , $class );
    $file = ROOT_DIR_CLASSES . DIRECTORY_SEPARATOR . $directory . '.class.php';
    if( file_exists( $file ) )
    {
        include( $file );
    }
}

$objectId = 0123;
$soapUser = 'User';
$soapPassword = 'Pass';

use Vendor\Object\Details\Create as Yacht;

try
{
    $yacht = new Yacht( $objectId, $soapUser, $soapPassword );
    print '<pre>';
    print_r( $yacht->objectSpecs() );
}
catch( Exception $e )
{
    var_dump( $e );
}
?>

##
# CREATE DETAILS
##
<?php 
namespace Vendor\Object\Details;

class Create
{
    public  $objectId;
    private $_soapUser;
    private $_soapPassword;
    private $_soapCall;

    //construct our object
    public function __construct( $objectId, $_soapUser, $_soapPassword )
    {
        $this->objectId = (string) $objectId;
        $this->_soapUser = $_soapUser;
        $this->_soapPassword = $_soapPassword;      
        $this->_soapCall = new \Vendor\Soap\Call\SoapGet( $this->_soapUser, $this->_soapPassword );
    }

    //make the details array
    public function objectSpecs()
    {
        try
        {
            $soapObjectDetails = $this->_soapCall->GetDetails( $this->objectId );
        }
        catch( Exception $e )
        {
            throw $e;
        }

        if( is_array( $soapObjectDetails ) )
        {
            return $soapObjectDetails;
        }

        throw new \Exception('Requested object has no specs');
    }
}
?>

##
# SOAP CALLS
##
<?php 

namespace Vendor\Soap\Call;

class SoapGet
{   
    private $_soapUser;
    private $_soapPassword;
    public  $soapCall;
    public  $soapParams;
    public  $objectId;

    function __construct( $_soapUser, $_soapPassword )
    {
        $this->_soapUser = $_soapUser;
        $this->_soapPassword = $_soapPassword;

        if( !isset( $this->_soapUser ) OR empty( $this->_soapUser ) )
        {
            throw new \Exception('No SOAP user given');
        }

        if( !isset( $this->_soapPassword ) OR empty( $this->_soapPassword ) )
        {
            throw new \Exception('No SOAP password given for user ' . $this->_soapUser);
        }

        $this->soapParams = array(
                'user' => $this->_soapUser,
                'password' => $this->_soapPassword,
        );

        $this->soapCall = new \SoapClient(
            null,
            array(
                'location' => 'http://localhost/webservice/webservice.php',
                'uri' => 'http://localhost/webservice.php',
                'trace' => 1,
            )
        );
    }

    function getDetails( $objectId )
    {
        if( !isset( $this->soapParams ) OR empty( $this->soapParams ) )
        {
            throw new \Exception('soapParams are empty');
        }

        $this->soapParams['yachtId'] = $objectId;

        //Soap expects a string encoded objectid
        $object = $this->soapCall->__soapCall( 'getDetails', $this->soapParams );
        $object = base64_decode( $object );
        $object = unserialize( $object );
        return $object;
    }
}
?>

Answer 1

Code base

I have removed the namespaces for cleaner view of the situation.

class SoapCredentials {
    private $_soapUser;
    private $_soapPassword;

    function __construct( $_soapUser, $_soapPassword ) {
        if(empty( $this->_soapUser ) ) {
            throw new \Exception('No SOAP user given');
        }

        if(empty( $this->_soapPassword ) ) {
            throw new \Exception('No SOAP password given for user ' . $this->_soapUser);
        }

        $this->_soapUser = $_soapUser;
        $this->_soapPassword = $_soapPassword;
    }

    public function UserName() {
        return $this->_soapUser;
    }

    public function Password() {
        return $_this->_soapPassword;
    }
}

class SoapGet {   
    private $_credentials;
    private $_soapCall;

    function __construct(\SoapClient $client, SoapCredentials $credentials) {
        $this->_credentials = array(
                'user' => $credentials->UserName(),
                'password' => $credentials->Password(),
            );

        $this->_soapCall = $client;
    }

    public function Execute($funcName, array $params = array(), $callback = NULL) {
        //TODO: check keys in $params for match one of the credentials key!!!

        $requestParams = array_merge($params, $this->_credentials);

        $result = $this->_soapCall->__soapCall( 'getDetails', $this->soapParams );

        if (\is_callable($callback)) {
            return call_user_func_array($callback, array($result));
        }

        return $result;
    }
}

class ShipYard {
    private $_soapGet;

    public function __construct(\Vendor\Soap\Call\SoapGet $soapGet) {
        $this->_soapGet = $soapGet;
    }

    public function GetYachSpecs($objectId) {
        $soapObjectDetails = $this->_soapGet->Execute('getDetails', array('yachtId' => $objectId), function ($object) {
            return unserialize( base64_decode( $object ) );
        });

        if( !is_array( $soapObjectDetails ) ) {
            //return an empty array instead it that's okay
            throw new \Exception('Requested object has no specs');
        }

        //create a Ship object and return that not an associative array (on fail, return null)
        return $soapObjectDetails;
    }
}

Usage

$objectId = 0123;
$soapUser = 'User';
$soapPassword = 'Pass';


$soapCall = new SoapClient(
            null,
            array(
                'location' => 'http://localhost/webservice/webservice.php',
                'uri' => 'http://localhost/webservice.php',
                'trace' => 1,
            )
        );

$credentials = new SoapCredentials($soapUser, $soapPassword);

$get = new SoapGet($soapCall, $credentials);

$context = new ShipYard($get);

$context->GetYachSpecs($objectId);

Your code

In your code you are hard coding things when you are using the new operator in your classes, to avoid this use dependency injection as i did. Your SoapGet class has 3 responsibilities: checking credentials, connecting and executing calls, do this is separate classes: one class for validating the credentials, injecting the SoapClient instance and then SoapGet can execute the requests.

Hard coding

Never fix parameters like connection data, parameter names in "generic" classes!

Exception handling

try {
    //anything
}
catch(Exception $e) {
    throw $e;
}

This is bad it makes the code dirty. Do not write these kind of try-catch block, they are unnecessary and this one especially bad becouse it will not work at all. When you are not in the global namespace in your catch block must reference to the base Exception type with the namespace qualifier (\Exception) if you want to catch all exception (if you got an Exception class in your custum namespace then it will work but please rename it a custom one).

(My code may contain typo-s.)