2
\$\begingroup\$

I'm trying to improve my OOP skills in PHP. One of my projects requires me to get user info from one API and send it to another API with a different structure.

I will use a simplified situation, but basically I have a big user array from one API. I need to break this into sub objects (customer and address) and serialize the data ready to be sent as a string over SOAP to another API.

I have the following user classes:

class User_customer{

    public $data;
    public $new_user_obj;

    public function __construct($data){
        $this->data = $data;
        $this->set_data();
    }

    public function set_data(){
        $this->new_user_obj->ID = $this->data->id;
        $this->new_user_obj->FirstName = $this->data->first_name;
        $this->new_user_obj->LastName = $this->data->last_name;
    }

    public function serialize_data(){
        $serializer = new XMLSerializer();

       // Notice I send a second param below - just indicating that sometimes
       // Although very similar, these params could be different per object
        return $serializer->serializeObj($this->new_user_obj, true);
    }
} 

class User_address{

    public $data;
    public $new_user_obj;

    public function __construct($data){
        $this->data = $data;
        $this->set_data();
    }

    public function set_data(){
        $this->new_user_obj->ID = $this->data->id;
        $this->new_user_obj->Address = $this->data->home_address;
        $this->new_user_obj->Postcode = $this->data->postal_code;
    }

    public function serialize_data(){
        $serializer = new XMLSerializer();

        // Notice no second param below
        return $serializer->serializeObj($this->new_user_obj);
    }
} 

And then in my main handling class, I'd call each section similar to:

$data = get_from_api();
$User_customer = new User_customer($data);
$User_address = new User_address($data);

send_to_other_api($User_customer->serialize_data(), $User_address->serialize_data());

Obvious alarm bells are ringing looking at this. I know that:

  • These classes are very similar, and there is a lot of code duplication.
  • I'm sending the same big data array with a lot of 'waste' to each class
  • I'm calling 'new XMLSerializer' so that means there is some 'coupling'
  • And no doubt a ton of other things

My instinct tells me I should have a parent class that these two inherit from, but it also looks like I should use an interface as both classes use the same methods. Also, maybe I should send an XMLSerializer object into the constructor when initiating each class (dependency injection?)

Any suggestions for 'the correct' way to write the above code would be helpful.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi. Welcome to Code Review! Hypothetical possibilities are really not what this site does. You should give us your actual code (or at least a reasonable subset). Perhaps the answer would be to rewrite XMLSerializer to not require a second parameter. \$\endgroup\$ – Brythan Mar 4 '15 at 18:34
2
\$\begingroup\$

Below I have written how I tackled your code. I hope the approach is clear from the explanations and code example. Feel free to use this approach yourself.

When I took a closer look at your code I began looking for similarities which could easily be extracted into its own class. This in turn made some of the harder things to extract easier. I have tried (I am writing from a tablet :D) to write down my implementation of your problem. It has been a little hard though because I properly lack some context, so feel free to modify the code example if you decide to use it.

The first thing I changed was your dependency of the XMLSerializer into a dependency for a common interface. This will help you if you some time in the future decide to change the serialization format into JSON instead of XML. With a common interface you can swap the actual implementation during the method call as long as the new implementation also adhere to the common interface. This will also help mitigate your tight coupling concerns.

interface Serializer {

    /*
     * I could not see the use of the second argument from your
     * code, so I added an extra one just in case:D 
     */
    public function serializeObj($object, $someUnknownFlag);

}

I then extracted the serialization logic and generation of the value object into its own class. This helped mitigate your code duplication concerns. When you read through the code example you might find that specifying which parameters are required is too much duplication, but I could not find a more flexible solution without making this too 'magical'.

I also think I corrected a possible bug in your code since the property new_user_obj is not instantiated anywhere.

How the class works are documented with comments throughout the code. I feel this gives you some freedom by having the serialization logic be free of how the actual data is fetched and let the extending class worry about that instead. This also makes it easier to add or remove required data parameters and/or change the behavior of the concrete class (UserCustomer, UserAddress etc.) without altering the serialization logic.

abstract class UserDataSerializer {

    private $data     = [];
    private $required = [];

    /**
     * @param array $data     An associative array of data that can be serialized.
     * @param array $required A sequential array of required data parameters.
     */
    public function __construct(array $data, array $required)
    {
        $this->data     = $data;
        $this->required = $required;
    }

    /**
     * Serializes the passed user data into the appropriate format.
     *
     * @param Serializer $serializer Here I have used an interface instead of a concrete implementation.
     *                               This will allow you to change to way the objects are serialized from
     *                               XML to maybe JSON by substituting to serializer implementation with
     *                               another serializer implementing the same interface.
     *
     * @return string Returns the data serialized into a string.
     */
    final public function serialize(Serializer $serializer)
    {

        /*
         * This method is made FINAL to ensure child classes doesn't
         * overwrite the implementation.
         */

        /*
         * By first executing the setData method now, we ensure
         * we only use instantiate a new object when it is actually needed.
         */
        $obj = $this->setData($this->data);

        return $serializer->serializeObj($obj, true);

    }

    private function setData(array $data) {

        /*
         * Check if all required parameters are present in the
         * associative array of data.
         *
         * First we calculate the differences from the required parameters
         * and the array keys (which are our names) from the data array.
         */
        $difference = array_diff($this->required, array_keys($data));

        /*
         * If the difference array is bigger than zero we are missing
         * one ore more required parameters.
         */
        if(count($difference) > 0) {
            throw new InvalidArgumentException('Insufficient data parameters provided. Missing: ' . implode(', ', $difference));
        }

        /*
         * We create a value object by using the ArrayObject and declaring
         * the access method for properties as class properties. This is done with
         * the STD_PROP_LIST flag.
         */
        return new ArrayObject($data, ArrayObject::STD_PROP_LIST);

    }

}

I have then extracted the specific logic regarding the required data and how the data is fetched to the class extending the UserDataSerializer.

class UserCustomer extends UserDataSerializer {

    public function __construct(array $data)
    {
        /*
         * We make a sequential array of required parameters for the
         * customer user type.
         */
        $required = ['id', 'firstname', 'lastname'];

        /*
         * Configure the parent class by declaring required data parameters
         * and providing the corresponding values.
         */
        parent::__construct($data, $required);

    }

    /*
     * Here you can implement specific methods for the
     * customer user type.
     */

}

class UserAddress extends UserDataSerializer {

    /*
     * Here the constructor take individual arguments and you can
     * decide which should be serialized and which of them should be
     * used for other purposes related to the class.
     */
    public function __construct($id, $address, $postcode, $otherInto)
    {
        /*
         * We make a sequential array of required parameters for the
         * address user type.
         */
        $required = ['id', 'address', 'postcode'];

        /*
         * We generate a data array of with the required parameters as keys
         * and the provided arguments as values.
         */
        $data = array_combine($required, [$id, $address, $postcode]);

        /*
         * Configure the parent class by declaring required data parameters
         * and providing the corresponding values.
         */
        parent::__construct($data, $required);

    }

    /*
     * Here you can implement specific methods for the
     * address user type.
     */

}

The usage is now a little different. In your code the passed data is fetched from an object, where my example uses an associative array. This is just because it is more convenient, when checking if all required parameters are present. Usage is as follows:

$serializer = new XMLSerializer(); // This implements the common Serializer interface.

$customer = new UserCustomer([
  'id'        => 123,
  'firstname' => 'awesomeness',
  'lastname'  => 'himself'
]);

$customer->serialize($serializer);

$serializer = new JSONSerializer(); // New serializer implementation.

$address = new UserAddress([
    'id'       => 123,
    'address'  => 'Cool Street 1',
    'postcode' => 'over 9000'
]);

$address->serialize($serializer);

Regarding your concern for using the same big data array.

I'm sending the same big data array with a lot of 'waste' to each class

The first and properly the best way to avoid this, is to only pass the necessary information to the constructor like I have done in the UserAddress class. Usage would be:

$address = new UserAddress(
        $data->id, 
        $data->address, 
        $data->postcode, 
        $data->otherInto
    );

This can also be changed by only extracting the required information for each class like you do. But as the data is set in the constructor we should take care not doing too much work inside it (it may already be too much for some). We could extract the gathering of only the necessary information into its own private method. I have named it prepare. I am also assuming the big data array is an object of type User, but it could be anything.

private function prepare(User $user)
{

    return [
        'id'       => $user->getId(),
        'address'  => $user->getAddress(),
        'postcode' => $user->getPostcode()
    ];

}

You must then alter your constructor. Here the User value object is passed as an argument.

public function __construct(User $user)
{
    /*
     * We make a sequential array of required parameters for the
     * address user type.
     */
    $required = ['id', 'address', 'postcode'];

    /*
     * We prepare the required data by extracting only the
     * necessary information from the user object.
     */
    $data = $this->prepare($user);

    parent::__construct($data, $required);


}

Other things

I have changed your naming convention to CamelCase.

I have changed the way data is fetched from the big data array to individual methods. This help ensure encapsulation of the data, by only providing setters for the properties which CAN be altered.

I have a bit of trouble understanding the reason to why the class UserAddress exist. To me an address consists of a string which could be a property inside a class. If you want more detail it could be an associative array where each element is given a value during instantiation of the class.

private $address = [
    'number'   => null,
    'street'   => null,
    'city'     => null,
    'postcode' => null,
    'country'  => null
];

I hope this can help guide you, happy coding!

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for taking the time to look at this and for such a thorough response. I have just skimmed over it but everything makes great sense and I'm looking forward to re-factoring my code based on this, tomorrow. \$\endgroup\$ – GeoffHorsey Mar 4 '15 at 23:02
  • \$\begingroup\$ @GeoffHorsey - I am glad you could use it. I am looking forward to a response as this is my interpretation and there certainly are other ways to solve the problem. Feel free to ask any questions and I will try to elaborate. \$\endgroup\$ – AnotherGuy Mar 5 '15 at 22:35

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.