collections icon indicating copy to clipboard operation
collections copied to clipboard

instanceof checks in `ExpressionVisitor`

Open rela589n opened this issue 3 years ago • 0 comments

Hello. I am wondering why does \Doctrine\Common\Collections\Expr\ExpressionVisitor::dispatch restricts $expr to be instance of predefined classes.

Doesn't it violate key principles of visitor pattern which leverage polymorphism and double dispatch in order to avoid such hard-coding of instanceof checks.

public function dispatch(Expression $expr)
{
    switch (true) {
        case $expr instanceof Comparison:
            return $this->walkComparison($expr);

        case $expr instanceof Value:
            return $this->walkValue($expr);

        case $expr instanceof CompositeExpression:
            return $this->walkCompositeExpression($expr);

        default:
            throw new RuntimeException('Unknown Expression ' . get_class($expr));
    }
}

CMIIW, foregoing code can be replaced with single line like this:

public function dispatch(Expression $expr)
{
    return $expr->visit($this);
}

Moreover, this method seems useless to me as we can (and should) call visit directly in the client code. If not, then what is the reason of Expression interface and implementations of visit method if those are not used at all?

rela589n avatar Jan 29 '22 10:01 rela589n