2

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?

<?php

interface Account {}

class PrivateAccount implements Account {}

class PublicAccount implements Account {}

class User {
    private $publicAccounts;
    private $privateAccounts;

    public function hasAccount(Account $account) {
        $haystack = [];

        if($account instanceof PrivateAccount) {
            $haystack = $this->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.

  • 4
    The invisible gorilla here is why is hasAccount doing the searching loop in PHP instead of letting the database do the search? The database can use indexes to search on IDs that will make this problem irrelevant. – Lie Ryan Jul 18 at 12:57
  • Thanks everyone, using the database to do the query will probably be the most efficient way. To know which table should I query against, I could add a method or would you recommend another way? I was trying to keep my Entity unaware of the data layer so this method should probably be in another layer. – Thibaud Jul 18 at 14:41
4

Yeah its a code smell to check the type of an object. The whole point of polymorphism is that you shouldn't have to know the type.

In your case the code will break if I pass in some other derived class of Account.

In terms of performance, searching the lazy loaded lists either way is going to be slow. Splitting it down into private and public accounts doesn't really help this in the general case as you may well find that all the users all have only private accounts for example.

Have the database do your FindAccountByUserIdAndAccountId logic, or cache lists of accountIds in the user object to remove this performance issue.

0

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).

  • As @LieRyan noted, if your issue is with ORM performance you might better tackle it using database mechanisms. Double dispatch would help against the code smell but if there is a way to eliminate the performance bottleneck without introducing the code smell in the first place that would be preferable. – Hans-Martin Mosner Jul 18 at 14:12
0

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?

0

I'm gonna go against the answers here and claim that instanceof in this case is okay.

Let's forget about PHP for a sec, this is what it would like in TypeScript:


type PublicAccount = {
 type: "public";
 publicAccounts: List<any>; // IDK the type
}

type PrivateAccount = {
 type: "private";
 privateAccounts: List<any>; // IDK the type
}

type Account = PublicAccount | PrivateAccount;

const hasAccount = (account: Account) => {
 switch(account.type) {
   case "public":
     // type-safe
     return account.publicAccounts.size > 0; /** Some logic **/
   case "private":
     // type-safe
     return account.privateAccounts.size > 0; /** Some logic **/
   default: 
     return false;
 } 

}

I would argue that this is just a sum algebraic data type, implemented in PHP, since it does not have this advanced typed key-value structure as TypeScript.

In TypeScript it is okay, so why wouldn't the same code (semantic-wise) in PHP be not okay?

As long as you know what you are doing, you should be fine.

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.