Object Oriented Design: Repositories and Inheritance

Question

I have an OO design concern. Let's say my models as following:

class Account {}
class LocalAccount extends Account {}
class SocialAccount extends Account {}

These Account entities are persisted and I could retrieve them (or do more operations on them) through a typical Repository. Let's say I have the following interface:

AccountRepository:

interface AccountRepository {
 getAll(): Array<Account> ;
}

And I have some use-cases where I need to work with Accounts where I basically would inject the AccountRepository, eg.

class FooUseCase {
 private AccountRepository accountRepository;
 constructor(AccountRepository accountRepository) {
 this.accountRepository = accountRepository;
 }
 public execute(): void {
 // Use case logic where I would call accountRepository.getAll()
 }
}

And at this point is where I am actually struggling. After some analysis, I thought about having specific AccountRepository implementations:

LocalAccountRepository:

class LocalAccountRepository implements AccountRepository {
 public getAll(): Array<LocalAccount> { /* implementation */ }
}

SocialAccountRepository:

class SocialAccountRepository implements AccountRepository {
 public getAll(): Array<SocialAccount> { /* implementation */ }
}

This would be alright, as far as a use-case would just need to work with LocalAccount or SocialAccount entities. But, what would happen if I would just needed to work with Account entities?

I feel I could implement an AccountRepository, that could just return a list of Accounts, but I feel that at the public getAll(): Array<Account> I would somehow have to add some kind of switch/if-else statement to be able to create each type of Account object... Which I think might violate some design principles (every time a new Account is added would have to extend the if-else, etc.).

AccountRepositoryImpl:

class AccountRepositoryImpl implements AccountRepository {
 public getAll(): Array<Account> {
 // Database access, query, etc.
 // results iteration
 let account: Account;
 if (result.type === AccountType.Local) {
 account = new LocalAccount(/* params */);
 } else if (result.type === AccountType.Social) {
 account = new SocialAccount(/* params */);
 }
 accounts.add(account);
 // iteration end
 return accounts;
 }
}

Any design improvement suggestions to my issues?

• Kristian H: I think the issue is that you have two implementations, but want to treat it as one? That is, you need to decide which repo to persist the account in, right? Wouldn't you just want to inject the repo into the account, and then have something like Account a = new SocialAccount(socialAccountRepo); /* stuff */ a.persist()
• Laiv: Have you considered the Account's class hierarchy (the inheritance) as the root of the problem Instead of the repositories?
• 9000: I wonder how your database query produces account records of both types. I would expect AccountRepository.getAll to just merge data from SocialAccountRepository.getAll and LocalAccountRepository.getAll, because they are separate, unrelated queries. (They could even be run in parallel.)
• Laiv: Are both Social and Local stored a part? Or are they stored in the very same storage? If yes, are both stored in the same collection/table/dataset? Are they supposed to be retrieved separatelly? Or are they mean to be always consumed as Accounts, regardless their concrete class? Ocassionally? Rarell?

Answer 1

Your else/if statement is essentially an implementation of the Factory Method Pattern. So if your code violates some design principles, then so does this well-known pattern.

Answer 2

As I get your code, you have three options:

1. Use your Factory Pattern Method implementation class AccountRepositoryImpl (It would be better if you use switch-case instead of if/else statements.) :

By factory method, caller class won't know anything about your AccountType.

Which clearly violates some design principles (every time a new Account is added would have to extend the if-else, etc.).

It is a factory method. If you think it violates something, probably you implement something wrong or you came where you don't want to. Consider @Robert Harvey answer.

2. Use generic class :

You can create your implementation class as generic class:

interface IAccountRepository<T> where T: Account
{
 T[] getAll();
} 
class AccountRepositoryImp<T> : IAccountRepository<T> where T : Account 
{
 public T[] getAll()
 {
 return new T[] { };
 }
}

Then you can call it like :

AccountRepositoryImp<LocalAccount> localAccountRep = new AccountRepositoryImp<LocalAccount>();
AccountRepositoryImp<SocialAccount> socialAccountRep = new AccountRepositoryImp<SocialAccount>();

3. Separate your repositories

Separate your repository implementations. If there is common methods for those accounts then you can use base interface and/or base class(abstract recommended). It's up to you. I give an example here by base interface :

interface IAccountRepository<T> where T: Account
{
 T[] getAll();
}
interface ILocalAccountRepository : IAccountRepository<LocalAccount>
{
 //other method(s) or properties.
}
interface ISocialAccountRepository : IAccountRepository<SocialAccount>
{
 //other method(s) or properties.
}
class LocalAccountRepository : ILocalAccountRepository
{
 public LocalAccount[] getAll()
 {
 return new LocalAccount[] { };
 }
 // other implementations.
}
class SocialAccountRepository : ISocialAccountRepository
{
 public SocialAccount[] getAll()
 {
 return new SocialAccount[] { };
 }
 // other implementations.
}

Note: I am sorry about programming language. It's C# example but what important is main idea.

• charliebrownie: thank you! don't worry about the language, as you very well said what is important is the concepts behind it!

Answer 3

As shown in your question, the accounts seem to know their types using some AccountType, so why not just pass that in when you need accounts of a certain type.

AccountsRepository {
 List<Account> getAll();
 List<Account> getByType(AccountType type);
}

If you need to return the accounts typed as the child implementations, I would question why, but you could use generics:

<T extends Account> List<T> getByType(Class<T> clazz);

Answer 4

I agree with other answers that there's nothing specifically wrong with this, it's one valid option. The other main way to deal with this is to use a Map to replace the switch statement. I'm assuming this is Java:

Updated The previous version did not guarantee the types were created. Here's one that inherently does.

public abstract class AccountType<E extends Account>
{
 private static final Map<Integer, AccountType<?>> types = new HashMap<>();
 public static final AccountType<LocalAccount> LOCAL = new AccountType<LocalAccount>(0) {
 @Override
 public LocalAccount create() {
 return new LocalAccount(/* params */);
 }
 };
 public static final AccountType<SocialAccount> SOCIAL = new AccountType<SocialAccount>(1) {
 @Override
 public SocialAccount create(){
 return new SocialAccount(/* params */);
 }
 };
 private final int value;
 private AccountType(int value)
 {
 this.value = value;
 if (types.put(value, this) != null) throw new IllegalStateException("type with identifier " + value + " is already registered");
 }
 public abstract E create();
 public static final Account create(int type)
 {
 // TODO prevent NPEs
 return types.get(type).create();
 }
}

Another way to accomplish this that I prefer in 1.8+ is to use factory methods like so:

Map<Integer, AccountType<?>> types = new HashMap<>();
types.put(0, LocalAccount::new);
types.put(1, SocialAccount::new);

You just need to figure out where you keep the map.

The use of the map eliminates the need to update your switch statement on adding a new AccountType. You just create a new one and set it's identifier.

You can also do something like this using typesafe enums but I think you'd have to use the valueOf method which takes the name of the enum. I don't believe there is a way to look up an enum with the ordinal without some code.