0

Often times, through my framework's importing side of things, I have access to certain data that functions I'm about to use will certainly need themselves.

I have "resolvers" that these said functions can use to get the additional data they need but they're very heavy operations and I was thinking:

Since I already have access to that exact data the function would need, why not just pass it and not rely on these heavy-lifting resolvers to get it for me?

Meet "support arguments" - I can just pass these to an array and within my function, check if they're passed - if not, ask the resolvers to give them (since I need the data either way).

Here's my actual code:

/**
 * Retrieves the component activated before a target component.
 *
 * @internal Targets the 'active_history' in the mother array, as such, it doesn't look at the installed history at all.
 *
 * @param string $target_component_name The target component name, used as a starting point.
 * @param boolean $skip_check Flag to allow skipping of certain checks.
 * @param array $support_package A support package passed from outside callers that can speed up the execution of this function due to bypasses of checks.
 *
 * @return string|mixed Returns the string of the component found before our target on success and a mix of boolean / WP_Errors on failure.
 */
public static function getActiveComponentBeforeThisOne( $target_component_name, $skip_check = False, $support_package = null )
{
    $installed_components = get_option( 'currently_installed_demo_components' );

    //Support check 1.
    if( $skip_check == False ) {
        if( !self::determineIfDemoComponentIsInstalled( $target_component_name ) ) {
            return new \WP_Error(
                'component-not-installed',
                esc_html__( 'Component is not currently installed.', 'setup-theme' )
            );
        }
    }

    //Support check 2.
    $local_package = [
        'component_category' => ''
    ];

    if( isset( $support_package['component_category'] ) ) {
        $local_package['component_category'] = $support_package['component_category'];
    } else {
        $local_package['component_category'] = self::getComponentCategoryByName( $target_component_name );
    }

    //Do some things with $local_package['component_category'] here.
}

As you can see, the first support $skip_check allows me to set a flag to skip a heavy check that can and is, in 95% of the cases, performed right before this function is called.

This is trap one: In an event-driven system such as WordPress (even if the hooks are perfectly done), one could hit race conditions failures, especially within AJAX calls (which is where these are used). My check would do nothing then. Even if in the 99.9% cases where it works, there's that dreadful situation where it doesn't - and it ends up breaking everything.

The second support is $support_package. I am calling getActiveComponentBeforeThisOne from a context where I already have access to a lot of data, specifically component_category, so, for almost all cases, I can just provide it to my function, again, to skip these heavy checks that slow me down.

This is trap two: I could simply just decide to call getComponentCategoryByName where I also call getActiveComponentBeforeThisOne if, at that point, I see that component_category is not available to me. This would introduce clarity as to what's going on. So the real issue here is visibility and the second, perhaps as big an issue is separation of concerns. As it stands, my getActiveComponentBeforeThisOne function ends up doing way too much. A function that wrangels a lot of data is generally fine - it's not smart to separate, one-time use array operations into multiple functions because it quickly becomes a cluster-f to follow: good comments do just fine. But here, we clearly, here we have a monster function that does a lot in terms of checks, in fact, my functions would end up being 90%+ just these checks and the 10% actual "do thing X" functionality.

Am I just littering my code, crushing the data tier into my logic tier? What is the alternative?

It feels that, when I read my code, that I have to get through a lot of checks where I need to say "Ah, so it checks for this, this and this..." just to get to the actual "ah, it does this!".

I'm also dealing with very sensitive stuff where if I import / delete the wrong thing, my system fails and there's no other way to do it, so, checking at every step before running crucial operations is also needed.

coolpasta
  • 641
  • It's really hard to tell, because we don't have enough context about the callers of this function. Agreed, it looks a hot mess, though. – Greg Burghardt Dec 14 '18 at 15:00
  • @GregBurghardt What type of information should I provide to help? The callers are "main controllers" that make use of these functions mostly for keeping track of / updating things within my system. For example, my controllers are in charge of running imports and through there imports, these functions will be called to look up / update my "log array". – coolpasta Dec 14 '18 at 15:02
  • Ok. So the "controllers" are code you wrote, and therefore you have some ability to change how this function is called, or are we stuck with the arguments that are provided? – Greg Burghardt Dec 14 '18 at 15:03
  • @GregBurghardt Correct. The controllers only make use of these functions, as I said, these functions (as in my post) mostly deal with logging / changing the logs so that my system knows what happened, what to do next, how to "undo" things. I have an array with "active components" where I track everything within my system. Inside the controllers there's the main logic for the importing process which does multitude of things, I have full freedom to do whatever I desire, really - point of my question is to know if this is a known bad pattern. – coolpasta Dec 14 '18 at 15:09
  • What is the $installed_components variable? What kind of value is it? An array of strings? – Greg Burghardt Dec 14 '18 at 15:09
  • @GregBurghardt It's indeed an array that all of these functions, the one in my post included, can modify. It's the log array that I keep in a database. In my system, I can import multiple steps and whenever that happens, certain functions append / remove from it. Here's an example of what my array looks like now: https://i.imgur.com/MGj8YlP.png – coolpasta Dec 14 '18 at 15:15
  • What else can $support_package contain? – Greg Burghardt Dec 14 '18 at 15:41
  • @GregBurghardt Anything, really. The idea is that my function would use whatever comes from there BUT you need to look at this function's implementation / comments in order to know what parameters are used. – coolpasta Dec 14 '18 at 15:45

2 Answers2

2

This is definitely a trap. A steel bear trap. And the code's leg is caught in it. And a T-Rex has already chomped down on the code and is thrashing it back and forth Jurassic Park-style.

Mixing a boolean parameter that turns functionality on and off, and combining that with another argument that is a nebulous "support" array gives you such a tangled mess that I wouldn't even know how to recommend a fix. You appear to have three problems:

  1. A check is expensive
  2. You need to supply a category once in a while
  3. Other miscellaneous things

Fixing the "expensive" check

Basically you have too many sources of information, and fixing this is not easy given the current state of the code. You need to maintain state so you can cache these expensive checks. The first time you check if a component is installed, cache that boolean value some place so you don't need to do the expensive part of the check. Check once. Return the cached value on subsequent checks so you don't have to conditionally check. Always check. The difference is it's only the first check that is expensive.

If something checks before calling this procedure, that other thing needs a way to mark a component as installed before running this procedure, essentially pre-caching the result of the check for that component.

Supplying a category or default value

This is another easy fix. Ditch the $support_package array in favor of an explicitly named argument, defaulting to null:

public static function getActiveComponentBeforeThisOne( $target_component_name, $category = null)
{
    if (!isset($category)) {
        $category = self::getComponentCategoryByName( $target_component_name );
    }

    // ...

Other miscellaneous things

I think the answer here is "don't do it." Either create additional methods, or create another class to encapsulate this data and behavior. The $support_package array is definitely a code smell. It becomes a swiss army knife of data that drives a swiss army knife of functionality. It sounds like you are missing some classes that specialize in other things. Here you can leverage polymorphism in conjunction with abstract classes or interfaces to pass in different behavior, which can be determined by the caller.

This shouldn't be that big of a deal, since the caller is already passing different stuff based on different use cases anyhow.

Greg Burghardt
  • 37,083
0

What you should do is pass in a repository of the things that your function needs, and implement caching in that repository

ie

self::getComponentCategoryByName( $target_component_name )

If you have already called this once, somewhere up stream of the function, then it should no longer be a slow check. As inside this function you can implement your

if( isset( $alreadyLoaded[$target_component_name] ) )

array and simply return the value without doing the heavy lifting twice.

Of course if there is an issue of a possible race condition you should also implement locking within the function in order to ensure that subsequent calls to the function wait for the first calls slow load rather than loading for each call.

But because you encapsulate the cache and the loading in the same place this is simple.

Ewan
  • 75,506