2

I'm writing a PHP web application (or actually a Symfony module). One part of it is a nested structure of ValueObjects (meaning: they are immutable and have to be validated on the creating). Such an object can have about ten plain properties and sometimes a couple of further DataObject properties.

Semantics: The classes represent the requests and responses of the Klarna Payments API.

The *Request objects are used to pass the data to the PaymentService, that needs it to send it to Klarna. The *Response objects are used for encapsulating the Klarna reponses.

The original data source for *Request objects can be the user session or an incoming call of another internal application (a shop system), that deals with the order processing (e.g. the shop notifies the payment module about a successful shipping and triggers so a capture request towards Klarna). The data source for *Response objects are typically the Klarna responses.

Currently the objects provide long constructors, that get a structured array data passed in and convert / map it to their plain properties and the nested objects (s. example below).

Currently the objects provide long constructors, that get a bunch of plain variables and (sometimes nested) objects passed in (s. example below).

Problems:

  1. The client of the service needs to know the complete structure of the *Request objects, that are actually more intended for the internal / payment module internal usage.
  2. Dealing with arrays as data transfer format (at least partially). (became obsolete due to some refactoring.)
  3. The instantiating bloats the code.
  4. The instance, which needs the object (e.g. for returning it a call response) is the instance, that have the data need for building this object.

How to refactor this to a more elegant solution? What design pattern might be suitable?

Some thoughts...

Factory Method? If so, should the Factory retrieve the data itself (e.g. from the session, the database etc.)? Or maybe a Simple Factory, that gets the type of the needed Request|Response passed to the create(...)?

UPDATE

The problem with the Factory Method pattern is the following: In my humble opinion a good factory should not force its client to provide the stuff need for creating an object -- it's the job of the factory. At the same time in my case sometimes it's possible (e.g., when the data for creation can be retrieved from the cache or the database) -- but sometimes the client itself has the data (e.g. as response of an outgoing API call or data coming in through a web callback). Meaning: some factories would look "healthy" and other ones would look like this:

class MyFooResponseFactory extends ResponseFactory
{
    public create(array $data) {...}
}

Builder? No, for the following reason:

The more I'm thinking about this problem the more I'm reasoning, that the core problem here is the fact, that the client is (or at least sometimes can be) the instance, that should be the client of the factory or the builder, whatever, is the instance, that provides the data for the result object, I want to build. And it's a neither the Builder, nor the Factory pattern can help here.

It's ValueObjects. So they should not provide any setters. So the Builder pattern cannot be applied here; at least no classical / straight forward implementation will be possible. (Or they need to become "normal" DataObjects.) Update: It's not a valid argument against the Builder pattern, since the Builder can first gather the data and then apply it once it instantiates the result object internally. So the builder's setters don't need to match to or call the object's setters.


Example, how it's implemented now:

DataObjects:

class KlarnaCreateSessionRequest
{
    public function __construct(
        string $acquiringChannel,
        KlarnaAttachment $attachment,
        KlarnaAddress $billingAddress,
        string $clientRoken,
        array $customPaymentMethodIds,
        KlarnaCustomer $customer,
        string $design,
        string $expiresAt,
        string $locale,
        string $merchant_data,
        string $merchant_reference1,
        string $merchant_reference2,
        KlarnaMerchantUrls $merchant_urls,
        KlarnaOptions $options,
        string $orderAmount,
        KlarnaOrderLines $orderLines,
        string $orderTaxAmount,
        KlarnaPaymentMethodCategories $paymentMethodCategories,
        string $purchaseCountry,
        string $purchaseCurrency,
        KlarnaAddress $shippingAddress,
        string $status
    ) {
        // assignments
        // ...
        // validation
        // ...
        if(! $dataIsValid) {
            throw new DataNotValidException('...');
        }
    }
}
class KlarnaAttachment {
    // ...
}
class KlarnaAddress {
    // ...
}
class KlarnaCustomer {
    // ...
}
class KlarnaMerchantUrls {
    // ...
}
class KlarnaOptions {
    // ...
}
class KlarnaOrderLines {
    // ...
}
class KlarnaPaymentMethodCategories {
    // ...
}

KlarnaPaymentService, where the data is actually needed:

class KlarnaPaymentService
{
    public function createSession(KlarnaCreateSessionRequest $klarnaCreateSessionRequest): void
    {
        // ...
    }
}

KlarnaPaymentProcessor, that gathers the data, builds a complete nested request object, and passes it to the service:

class KlarnaPaymentProcessor
{
    // properties
    // constructor
    // ...
    public function startPayment(): void
    {
        $this->klarnaPaymentService->createSession(
            $this->createKlarnaCreateSessionRequest()
        );
    }

    private function createKlarnaCreateSessionRequest(): array
    {
        // ... Some stuff to get the create session request data from the user session and/or other places.
        return new KlarnaCreateSessionRequest(
            'ecommerce',
            new KlaAttachment(
                '{"customer_account_info":[{"unique_account_identifier":"test@gmail.com","account_registration_date":"2017-02-13T10:49:20Z","account_last_modified":"2019-03-13T11:45:27Z"}]}',
                'application/vnd.klarna.internal.emd-v2+json'
            ),
            'string',
            new KlarnaAddress(
                'Attn',
                'London',
                'GB',
                'test.sam@test.com',
                'Andersson',
                'Adam',
                'string',
                '+44795465131',
                'W1G 0PW',
                'OH',
                '33 Cavendish Square',
                'Floor 22 / Flat 2',
                'Mr.'
            ),
            'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.ewogICJzZXNzaW9uX2lkIiA6ICIw',
            [
                0 => 'string',
            ],
            new KlarnaCustomer(
                '1978-12-31',
                'male',
                'string',
                'string',
                'LIMITED_COMPANY',
                'string',
                'Mr.',
                'organization',
                'string'
            ),
            'string',
            '2038-01-19T03:14:07.000Z',
            'en-GB',
            '{"order_speicif":[{"substore":"Women\'s Fashion","product_name":"Women Sweatshirt"}]}',
            'ON4711',
            'hdt53h-zdgg6-hdaff2',
            new KlarnamerchantUrls(
                'https://...',
                'https://...',
                'https://...'
            ),
            new KlarnaOptions(
                '#FF9900',
                '#FF9900',
                '#FF9900',
                '#FF9900',
                '5px'
            ),
            2500,
            new KlarnaOrderLines([
                0 => new KlarnaOrderLine(
                    'https://www.exampleobjects.com/logo.png',
                    '{"customer_account_info":[{"unique_account_identifier":"test@gmail.com","account_registration_date":"2017-02-13T10:49:20Z","account_last_modified":"2019-03-13T11:45:27Z"}]}',
                    'Running shoe',
                    new KlarnaProductIndentifiers(
                        'shoe-brand',
                        'Shoes > Running',
                        '4912345678904',
                        'AD6654412-334.22'
                    ),
                    'https://.../AD6654412.html',
                    1,
                    'pcs',
                    'AD6654412',
                    1900,
                    2500,
                    500,
                    475,
                    'physical',
                    2500
                ),
            ]),
            475,
            new KlarnaPaymentMethodCategories([
                0 => new KlarnaPaymentMethodCategory(
                    new KlarnaAssetUrls(
                        'https://x.klarnacdn.net/payment-method/assets/badges/generic/klarna.svg',
                        'https://x.klarnacdn.net/payment-method/assets/badges/generic/klarna.svg',
                    ),
                    'pay_later',
                    'Pay Later'
                ),
            ]),
            'GB',
            'GBP',
            new KlarnaAddress(
                'Attn',
                'London',
                'GB',
                'test.sam@test.com',
                'Andersson',
                'Adam',
                'string',
                '+44795465131',
                'W1G 0PW',
                'OH',
                '33 Cavendish Square',
                'Floor 22 / Flat 2',
                'Mr.'
            ),
            'complete'
        );
    }
}
12
  • 1
    'Currently the objects provide long constructors, that get a structured array data passed in and convert / map it to their plain properties and the nested objects.' Could you clarify this? Maybe a code example? – Rik D Dec 1 '20 at 7:26
  • @RikD Thank you for your comment. Just updated the question with a code example. – automatix Dec 1 '20 at 10:16
  • 2
    Does this answer your question? How can you decompose a constructor? – Greg Burghardt Dec 1 '20 at 15:26
  • 1
    It will sound controverting but I don't care. I think the Builder pattern is the way to go. For (at least) 2 reasons: #1 you can hide the needs of KlarnaCreateSessionRequest for countless dependencies, #2 the builder can be passed as an argument to different components so each component fills the data it knows. I got the feeling your main concerns are #1 and #3 and I agree. None of the two is addressable with factories. Bundling arguments will solve (in the best of the cases) only #3 and only partially. – Laiv Dec 1 '20 at 15:28
  • 1
    The KlarnaCreateSessionRequest class looks like it has lots of constructor arguments that should be related, but it is difficult to tease out how. I think that is the biggest code smell to sort out, and will likely lead you to a solution. How are these constructor arguments related? – Greg Burghardt Dec 1 '20 at 15:32

Your Answer

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

Browse other questions tagged or ask your own question.