5
\$\begingroup\$

I have a function that takes in a controller instance, a method, and parameters (this is for a little MVC experiment I'm working on). The purpose of the function is to execute the controller method with the given parameters.

However, there are a few caveats. First, if the controller has a "router" method, it will call that instead of the method that was passed in. Second, if any of the controller "annotations" return a response, that will also override the method. Then, if those cases fail, we return the response of the actual controller method passed in.

It looks like this:

    switch (true)
    {
        // Router methods get first priority!
        case ($controller->using_router):
            $response = call_user_func_array(array($controller, 'router'));
            break;

        // Annotation responses take precedence over method responses because they are often used for authentication.
        case ( ! is_null($response = Annotation\Runner::run(Annotation\Extractor::extract($controller, $method)))):
            break;

        // No router or annotation responses... we're ready to run the method!
        default:
            $response = call_user_func_array(array($controller, $method), $parameters);
    }

For some reason, this just feels dirty. I don't like the break after checking for an annotation response. But, I'm not sure of any other way to implement a response "priority".

Is there a better way?

\$\endgroup\$
  • \$\begingroup\$ I agree. But, just having a "break" (and nothing else) under a case wasn't sitting right with me. \$\endgroup\$ – TaylorOtwell Mar 10 '11 at 19:11
10
\$\begingroup\$

That's certainly a bastardization of the switch statement.

What you should use is an if...else if... group:

$response = null; //initialize
if ( $controller->using_router)
{
  $response = $controller->router();
}
else
{
  $response = Annotation\Runner::run(Annotation\Extractor::extract( $controller, $method ) );
  if ( is_null( $response )
  {
    $response = call_user_func_array( array( $controller, $method ), $parameters );
  }
}

Note: this code flows in a readable manner. The response is set, if the controller is using the router, the response is set to the output of $controller->router(), otherwise the response is set to the extracted controller method.

If that is null, the response is finally set to whatever $controller->$method(...) produces.

Code should always flow in a readable manner (unless it's minified, obfuscated, or encoded).

\$\endgroup\$
5
\$\begingroup\$

Code is almost always better when you return a value instead of holding on to a value to return later (imnsho). If you simply say

if ( $controller->using_router) {
  return $controller->router();
}

you've simplified the function significantly. The rest of it depends on whether the next result is null, so you will need somehow to hold onto it, but you're already looking at simpler code.

I don't think a switch statement is what you want here. It can be shoehorned to fit - but it's not really appropriate or descriptive of what you're trying to do.

\$\endgroup\$

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.