Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
?>
share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

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.)

share|improve this answer
    
Thanks for this, going to give it a good look right away! –  Michal Skrzypecki Dec 2 '12 at 19:26
    
Everything looks great, this is helping. I'm just thinking about the soapGet. The idea initially was that i wanted to create a soapGet class, and then define the options. For example the getDetails, but i could imagine a getOverview aswell. –  Michal Skrzypecki Dec 2 '12 at 20:52
    
If you want other query methods put them in the ShipYard class as i did with the getDetails (GetYachtSpecs) the SoapGet is just gateway, a connection with authentication. –  Peter Kiss Dec 2 '12 at 21:18
    
I'm looking trough this but GetYachtSpecs $soapObjectDetails and the soapdetails execute lost me a bit. Could you maybe illitarte a bit more what you did in there. I think i get the concept, but i'm not sure. –  Michal Skrzypecki Dec 3 '12 at 19:33
    
There is some more issues that i am facing. For example the soap execute, returns a call_user_func_array() but the $result is still serialized and encoded, a string, but call_user_func_array expects an array. When i unserialize and decode the $result, it returns as an array and therefor can be returned with call_user_func_array. I'm still not getting why the is_callable is there, when returning the $result will return me the same value. –  Michal Skrzypecki Dec 3 '12 at 20:05
show 5 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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