HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Rewriting dispatch method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
methoddispatchrewriting

Problem

Is it possible to rewrite this piece of code any better when it comes to efficiency and/or readability?

public function dispatch()
{
    $key = array_search($this->reqUri, $this->routes);

    if ($key !== false) {
        if (isset($this->methods[$key])) {
            if ($this->methods[$key] === $this->reqMethod) {
                call_user_func($this->callables[$key]);

                return;
            } else {
                $this->defaultCallable();

                return;
            }
        }

        call_user_func($this->callables[$key]);

        return;
    }

    $this->defaultCallable();
}

Solution

The array_search will iterate over every element in $this->routes. That's not scalable. It would be better to reorganize $this->routes so that you can lookup keys more directly, so that your code will work like this:

if (isset($this->routes[$this->reqUri])) {
    $key = $this->routes[$this->reqUri];
    // ...
}


In this arrangement, the lookup should be in \$O(1)\$ time or close, even with large number of routes.

Do you really need the !== false here?

if ($key !== false) {


Can't it be simply:

if ($key) {


In the innermost if, since you return in both the if and the else, you could move the return statement outside:

if (isset($this->methods[$key])) {
    if ($this->methods[$key] === $this->reqMethod) {
        call_user_func($this->callables[$key]);
    } else {
        $this->defaultCallable();
    }
    return;
}


In terms of readability, I find it slightly better this way:

if ($key) {
    if (isset($this->methods[$key])) {
        if ($this->methods[$key] === $this->reqMethod) {
            call_user_func($this->callables[$key]);
            return;
        }
    } else {
        call_user_func($this->callables[$key]);
        return;
    }
}

$this->defaultCallable();


That is, the $this->defaultCallable() call appears only once instead of twice, everything else still being the same.

Code Snippets

if (isset($this->routes[$this->reqUri])) {
    $key = $this->routes[$this->reqUri];
    // ...
}
if ($key !== false) {
if ($key) {
if (isset($this->methods[$key])) {
    if ($this->methods[$key] === $this->reqMethod) {
        call_user_func($this->callables[$key]);
    } else {
        $this->defaultCallable();
    }
    return;
}
if ($key) {
    if (isset($this->methods[$key])) {
        if ($this->methods[$key] === $this->reqMethod) {
            call_user_func($this->callables[$key]);
            return;
        }
    } else {
        call_user_func($this->callables[$key]);
        return;
    }
}

$this->defaultCallable();

Context

StackExchange Code Review Q#56272, answer score: 4

Revisions (0)

No revisions yet.