Skip to main content

Instanceof code smell [Resolved]

The use of instanceof might be a code smell and I am in front of the following code which seems ok. Would you consider that instanceof should not be used in such case? What would be the pattern to use?

privateAccounts;
        } else if($account instanceof PublicAccount) {
            $haystack = $this->publicAccounts;
        }

        foreach($haystack as $someAccount) {
            if($account->getId() == $someAccount->getId()) {
                return true;
            }
        }
        return false;
    }
}

To be more specific, $privateAccounts and $publicAccounts are objects that will be lazy loaded by an ORM from a relationnal database, so calling getId() is costly (each call results in a database request). PublicAccount and PrivateAccount are 2 tables from the database.

Using $haystack = array_merge($privateAccounts, $publicAccounts) would remove the use of instanceof but would have a performance cost.


Question Credit: Thibaud
Question Reference
Asked July 20, 2019
Posted Under: Programming
35 views
3 Answers

Instead of using instanceof you might implement a method isPrivate() on Account which would be implemented as return true; in PrivateAccount and return false; in PublicAccount.

Another option would be to use double dispatch:

User would implement methods hasPrivateAccount() and hasPublicAccount(), Account subclasses would implement isAccountOf() using one of these methods, and User's hasAccount() would call isAccountOf($this).


credit: Hans-Martin Mosner
Answered July 20, 2019

Why are the accounts in two different tables? Don’t they have many columns in common, if not all of them? Can’t you put common columns in a common table and then join to an ancillary table for the differences? If you need referential integrity, can’t you use one of the ancillary tables or use a two-column constraint (second being accountTypeID)? Or use a materialized indexed view?

I question why this code even needs to do something different. Isn’t privateAccount.accounts and publicAccount.accounts enough? How could you ever have publicAccount.privateAccounts or vice versa?


credit: ErikE
Answered July 20, 2019
Your Answer
D:\Adnan\Candoerz\CandoProject\vQA