1
\$\begingroup\$

The PHP Mess Detector of my IDE warns about a Cyclomatic Complexity of 14 (threshold is 10). To me this code doesn't seem to hard to follow. Would you refactor it in some way to lower the metric?

  1. Exit if $payload isn't an array or has a number of elements not equal to 1
  2. If $payload is array or ArrayAccess build a proper checker/setter
  3. If $payload is stdClass do the same as 2
  4. Use control options and checker/setter to actually set the value inside $payload

The method:

public function onBeforeEncrypt(CryptDecryptEvent $event)
{
    // Get the actual parameters of the call
    $payload = $event->getPayload();

    // Avoid errors when __soapCall is invoked directly with custom arguments
    if (!is_array($payload) || 1 !== count($payload)) {
        return;
    }

    // Reference to the payload (by reference just in case it's an array)
    $payload = &$payload[0];

    // Shortcuts to control options
    $paramName      = $this->options['name'];
    $paramValue     = $this->options['value'];
    $allowOverride  = $this->options['allowOverride'];
    $nullLikeNotSet = $this->options['threatNullLikeNotSet'];

    // Dynamically build checker and setter based on the type
    $checker = $setter = null;

    // Payload is array or ArrayAccess implementation inside main array
    if (is_array($payload) || $payload instanceof \ArrayAccess) {
        $checker = function ($k) use ($payload, $nullLikeNotSet) {
            return isset($payload[$k]) || (null === $payload[$k] && $nullLikeNotSet);
        };

        $setter = function ($k, $v) use (&$payload) { $payload[$k] = $v; };
    }

    // Payload is stdClass inside the main array
    if ($payload instanceof \stdClass) {
        $checker = function ($p) use ($payload, $nullLikeNotSet) {
            return isset($payload->$p) || (null === $payload->$p && $nullLikeNotSet);
        };

        $setter = function ($p, $v) use ($payload) { $payload->$p = $v; };
    }

    // Set the value if we can override or parameter is abset
    if ($checker && $setter && ($allowOverride || !$checker($paramName)) {
        $setter($paramName, $paramValue);
    }
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

You could easily extract the 4 functions you have within if statements by having

arrayChecker()
arraySetter()
classChecker()
arraySetter()

And then have something like

//Exit immediately if override is not allowed ( why wait ? )
//Personally, that would be the first line for me
if(!$allowOverride)
  return;

// Payload is array or ArrayAccess implementation inside main array
if (is_array($payload) || $payload instanceof \ArrayAccess) {
  if(!$arrayChecker($paramName)) {
    $arraySetter($paramName, $paramValue);
  }
}

// Payload is stdClass inside the main array
if ($payload instanceof \stdClass) {
  if(!$classChecker($paramName)) {
    $classSetter($paramName, $paramValue);
  }
}  

One more thing, you re-use the payload variable. For me, what you get from ->getPayload() is a payload of an array with something in the first position. ( crate plus the goods ). To then re-use the variable to point just to the goods seems wrong to me.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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