Write a method with same logic but deals with different objects

Question

Assume I have the following method that does the same logic but deals with different objects (It's more a pseudocode):

private <E> List<E> updateOrInsert(List<E> list) {
 if (list == null && list.size() == 0) {
 return list;
 }
 if (list.get(0) instanceof Explore) {
 List<String> numbers = new ArrayList<>();
 List<Explore> explorisList = (List<Explore>) list;
 explorisList.forEach(item -> {numbers.add(item.getNumber());});
 List<Explore> exploreDB = explorisRepository.findAllByNumberIn(numbers);
 Map<String, Explore> map = new HashMap<>();
 exploreDB.forEach(item -> {
 map.put(item.getNumber(), item);
 });
 explorisList.forEach(item -> {
 Explore itemMap = map.get(item.getNumber());
 if (itemMap != null) {
 item.setId(itemMap.getId());
 }
 });
 return (List<E>) explorisRepository.saveAll(explorisList);
 } else if (list.get(0) instanceof Capital) {
 List<String> codes = new ArrayList<>();
 List<Capital> listOfCompanies = (List<Capital>) list;
 listOfCompanies.forEach(item -> codes.add(item.getCode()));
 List<Capital> capitalDB = companyRepository.findAllByCodeIn(codes);
 Map<String, Capital> map = new HashMap<>();
 capitalDB.forEach(item -> {
 map.put(item.getCode(), item);
 });
 ((List<Capital>) list).forEach(item -> {
 Capital itemMap = map.get(item.getCode());
 if (itemMap != null) {
 item.setId(itemMap.getId());
 }
 });
 return (List<E>) companyRepository.saveAll(capitalSet);
//if statement continues but you got the picture
 } else {
 throw new UnsupportedOperationException("Unsupported operation for " + list.getClass().getName());
 }
 // ... etc.

As you can see each list has the same business logic but it deals with different objects.

Is it better to separate them to different methods? And why?

My argument for such an approach is I assume that a function/method should only represent one piece of logic. So, creating a different functions that do the same logic is a bad design.

What about maintainability?

I also got a comment that I am using Java Generics in a wrong way and I'm dealing with Java as a functional language.

• Ben Cottrell: So the pattern is identical for each type? It looks to me as if your problem is that Capital and Explore should both implement the same interface to expose their identity, but they (presumably) don't - e.g. item.getIdentity(). Similarly, it also looks as if you could generalise an interface to your repositories as well. It seems to me as if can solve the problem without a generic method, instead generalise the concept of object identity in your code using interfaces - both on the Capital/Explore objects and on the repository methods which begin findAllBy....

Answer 1

It looks to me as if the root of your problem is the fact that Capital and Explore don't provide a uniform way for your updateOrInsert method to be able to identify your Explore and Capital objects.

I would consider generalising this and putting those methods into an interface which Explore and Capital are able to implement. For example, instead of getCode and getNumber, it could be generalised as getIdentifier instead:

interface IEntity {
 String getIdentifier();
 void setId(int id);
 int getId();
}

Each of the classes which you need to support within this pattern might look a bit like this, to ensure that you can work with them using the generalised interface instead:

class Capital implements IEntity {
 @Override public void setId(int id) { _id = id; }
 @Override public int getId() { return _id; }
 @Override public String getIdentifier() { return getCode(); }
 public string getCode() { return _code; }
}
class Explore implements IEntity {
 @Override public void setId(int id) { _id = id; }
 @Override public int getId() { return _id; }
 @Override public String getIdentifier() { return getNumber(); }
 public string getNumber() { return _number; }
}

This provides you with a uniform way for the updateOrInsert method to work with each of the individual objects.

The next issue is with the repositories.

Given how closely related your updateOrInsert method is to your repositories, I would recommend that you put this method into an abstract class that each of the repositories can extend, which also includes signatures for the find and save methods:

For example:

abstract class RepositoryBase<T extends IEntity> {
 public abstract List<T> findAllByIdentifierIn(List<String> list);
 public abstract List<T> saveAll(List<T> list);
 public List<T> updateOrInsert(List<T> list) {
 /* TODO: Implementation here */
 }
}

By putting a constraint using <T extends IEntity>, the updateOrInsert method will be able to use IEntity.setId, IEntity.getId and IEntity.getIdentifier

This approach allows repositories for Explore and Capital to extend BaseRepository then take advantage of the common base class logic for updateOrInsert:

class ExplorisRepository extends RepositoryBase<Explore> {
 @Override public List<Explore> findAllByIdentifierIn(List<String> list) {}
 @Override public void saveAll(List<Explore> list) {}
}
class CapitalRepository extends RepositoryBase<Capital> {
 @Override public List<Capital> findAllByIdentifierIn(List<String> list) {}
 @Override public void saveAll(List<Capital> list) {}
}

These Repository classes extend RepositoryBase by providing the real type of Capital or Explore. That means updateOrInsert no longer needs to be concerned with different types because the Repository class will pass that information up to the base. It always knows the exact type it's working with, and no need to perform any casting or type checking because <T> will be strongly typed as Explore or Capital or any of the other entities:

 // implementation of RepositoryBase<T>.updateOrInsert()
 public List<T> updateOrInsert(List<T> list){
 if (list == null || list.size() == 0) return list;
 List<String> identifiers = new ArrayList<>();
 list.forEach(item -> {identifiers.add(item.getIdentifier());});
 List<T> itemsFromDb = this.findAllByIdentifierIn(identifiers);
 Map<String, T> map = new HashMap<>();
 itemsFromDb.forEach(item -> {
 map.put(item.getIdentifier(), item);
 });
 list.forEach(item -> {
 T itemMap = map.get(item.getIdentifier());
 if (itemMap != null) {
 item.setId(itemMap.getId());
 }
 });
 return saveAll(list);
 }

This will work for any of the classes which implement IEntity -- it can work with the generalised IEntity.getIdentifier method instead of needing to worry about the specifics surrounding getNumber vs getCode (the concrete classes themselves take care of this issue).

There's no need to duplicate/copy-paste the logic for updateOrInsert because it's already inherited by each of the repositories.

Answer 2

"got a comment that I am using Java Generics in a wrong way"

This is my impression too. If you put stuff in a generic list and you work with that list you typically should not have to deal with or worry about the type of the elements. You are examining elements and act upon their type. This defeats the purpose of using generics in the first place.

I think you should split things up and use explicit item types rather then generics. Then ask yourself what Explore and Capital really have in common, if anything. You may be better of with a common base type for Explore and Capital and use polymorphism (use a list of base type elements and work with that). But if there is no common base, it is better to have some code that looks similar than to try and force it into one thing when you are really not dealing with one thing.

Answer 3

There are many countable reason why you should not to do that way. I try to explain it what's wrong and what will you need to do also if you do your way:

1. DRY

When you do that way, it's against to Don't Repeat Yourself(DRY). You copy and paste same code for each type. By a good design, you can do it just one method which doesn't need to know type of data and there will be just one place to change when needed.

2. Limitation per Type

Another reason why you shouldn't do that way, you restrict your code by one logic per type. Assume that you need to get all Capital. When you check object type in your code and do your logic, you can not give flexibility to call another service.

public interface ICapitalService
{
 List<Capital> GetAll();
}
public class LocalCapitalService : ICapitalService
{
 public List<Capital> GetAll()
 {
 // get from db.
 }
}
public class RemoteCapitalService : ICapitalService
{
 public List<Capital> GetAll()
 {
 // get from web service
 }
}

Someone get capitals from remote service and others may want to get from db. But you restrict it by checking type of Capital and doing your specific logic in your code. You can't add another option for Capital.

3. You need to ensure just child class type is checking.

When you check type by instanceof, it will also return true for parent class. So, you may not reach deserved code block.

if (list.get(0) instanceof Car) {
 //...
}
else if (list.get(0) instanceof Volvo)
{
 //..
}

Because of Volvo is a car, you can't reach Volvo block and you need to manage manually.

4. Software Language Limitation

You are checking List of item types by getting first item of list.(list.get(0)). If the first item of list is null, instanceof will return false and logic won't work as expected and last else block will work.

• Steve Chamaillard: Better : do not check the class type at all, defer logic to it.