1
\$\begingroup\$

I have a container. This container has exactly this structure:

['unique_id_1'] => [
    'base' => Object,
    'style' => Object,
    'id' => 'unique_id_1',
    'type' => 'warning'
],
['unique_id_2'] => [
    'base' => Object
    'style' => Object,
    'id' => 'unique_id_2',
    'type' => 'normal'
]

It's important to know that the style key holds an object that is dynamically built, as in: based on certain data flags, it is either the default object, or a custom one and for this, I use a replacer object of some sorts:

public function replaceStyleForId( $id, StyleInterface $new_style )
{
    $this->replacement_data[$id] = [ 'id' => $id, 'replace_with' => $new_style ];
}

Where replace_with is my new style object that I'll use. Now, as you can tell, it also has an id. I use this to identify my original data in the container and say "aha, the replacer's telling me that any item with this ID must have its style object changed. Assuming I did replaceStyleForId( 'unique_id_2', new CustomStyle), my system would then, when it finds, in the original data, items with this ID, replace their style with CustomStyle.

So, we're dealing with two arrays here and I'm first, searching by either id or type to see if it's found, then perform operations if so.

Great! A way to safely plug & play objects even with PHP's limiting interfaces. But hold on. This is what my factory looks like when it's doing this exact thing:

    /**
     * Retrieves the data from the replacer. This data will solely be used to replace
     * style objects of certain notifications based on either type or ID.
     */
    $replacer_data = $this->replacer->getDataForReplacement();

    foreach( $notifications_data as $notification_data ) {
        if( $replacer_data ) {
            foreach( $replacer_data as $replacement_data_handle => $replacement_data_package ) {
                if( array_key_exists( 'type', $replacement_data_package ) ) {
                    if( $notification_data['type'] == $replacement_data_package['type'] ) {
                        $this->addBundledNotificationPackage(
                            new Base\BasicNotification( $notification_data, $notification_data['id'] ),
                            $replacement_data_package['replace_with']
                        );
                    } else {
                        $this->addBundledNotificationPackage(
                            new Base\BasicNotification( $notification_data, $notification_data['id'] ),
                            new Base\BasicNotificationStyle()
                        );
                    }
                }
                if( array_key_exists( 'id', $replacement_data_package ) ) {
                    if( $notification_data['id'] == $replacement_data_package['id'] ) {
                        $this->addBundledNotificationPackage(
                            new Base\BasicNotification( $notification_data, $notification_data['id'] ),
                            $replacement_data_package['replace_with']
                        );
                    } else {
                        /**
                         * If nothing is inside the replacer, then we simply just create the notification using the base objects.
                         */
                        $this->addBundledNotificationPackage(
                            new Base\BasicNotification( $notification_data, $notification_data['id'] ),
                            new Base\BasicNotificationStyle()
                        );
                    }
                }
            }
        } else {
            /**
             * If nothing is inside the replacer, then we simply just create the notification using the base objects.
             */
            $this->addBundledNotificationPackage(
                new Base\BasicNotification( $notification_data, $notification_data['id'] ),
                new Base\BasicNotificationStyle()
            );
        }
    }

Except, this is hell. First of all, I loop through all of my original data, then, for each loop, I check if there's data to replace and then I do 2 more checks to look at what I need to replace by. Is it by id, is it by type? If so, then go ahead and create my object package to pass it to the container as a final "after all these checks with the replacer, this is your final package".

The logic is sound and although it looks complex, it makes sense. I abstracted as much as I could so I don't repeat myself, but this is clearly not clean.

\$\endgroup\$
  • 1
    \$\begingroup\$ @mickmackusa Edited. Sorry. \$\endgroup\$ – coolpasta May 15 at 5:48
  • \$\begingroup\$ So $replacement_data_handle is always redundantly stored in $replacement_data_package['id']? \$\endgroup\$ – mickmackusa May 15 at 6:01
  • \$\begingroup\$ @mickmackusa Correct, the handle from the replacement data will always correspond to $replacement_data_package['id'] and furthermore, they could (this is a case my code handles) correspond to a notification's id. \$\endgroup\$ – coolpasta May 15 at 6:18
  • \$\begingroup\$ Can I just confirm that there is no benefit to potentially replacing twice for the same set of data? In other words, you have if block followed by another if block -- but would be just as accurate if the 2nd if was elseif, right? I am asking because when I suggest a refactor, I don't want to be fuzzy on the details. Also, are the array_key_exists() calls actually necessary? Is your incoming data inconsistently/unpredictably structured? \$\endgroup\$ – mickmackusa May 16 at 12:41
  • \$\begingroup\$ @mickmackusa There is a benefit if you believe it is, I can modify and set my data as I please, basically free to do anything. As for the call, yup, it is necessary, even if heavily documented on how the data should look, I just want to play defensively. \$\endgroup\$ – coolpasta May 16 at 13:54

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.