Rewriting dispatch method

Question

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();
}

Answer 1

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.