3

If I have to make a different database query depending on the presence or not of different parameters, which would be the correct design pattern to avoid too many if-else with the different combinations ? Let's say I have parameters a, b, c (the amount can grow in the future), I'm using repositories so I would have to make a call something like this

public Foo getFoo(String a, String b, String c){
   Foo foo;
   if(a!=null && !a.isEmpty() && b!=null && !b.isEmpty() && c!=null && !c.isEmpty())
      foo = repository.findByAAndBAndC(a,b,c);
   if((a==null || a.isEmpty()) && b!=null && !b.isEmpty() && c!=null && !c.isEmpty())
      foo = repository.findByBAndC(b,c);
   if((a!=null && !a.isEmpty()) && (b==null || b.isEmpty()) && c!=null && !c.isEmpty())
      foo = repository.findByAAndC(a,c);
   if((a==null || a.isEmpty()) && (b==null || b.isEmpty()) && !b.isEmpty() && c!=null && !c.isEmpty())
      foo = repository.findByC(c);
   if((a==null || a.isEmpty()) && (b==null || b.isEmpty()) && !b.isEmpty() && (b==null || b.isEmpty()))
      foo = repository.findOne();
   etc.
   .
   .
   .
   return foo;
}

How can that be better structured ?

Arielo
  • 81
  • 2
  • 9
  • A good start would be to use Apache StringUtils.isEmpty. – rghome Jun 01 '18 at 10:10
  • 1
    The pattern required is dynamic query building. You might be better off reconsidering the multitude of `findByX` methods on your repository, and instead using a specification or criteria pattern, which is typically what ORM's like Hibernate or SpringData would give you. – StuartLC Jun 01 '18 at 10:12
  • 1
    The granular repo design *is* the real problem here, IMO. – StuartLC Jun 01 '18 at 10:48

3 Answers3

8

At the beginning, I would propose you the Specification design pattern that :

is a particular software design pattern, whereby business rules can be recombined by chaining the business rules together using boolean logic. The pattern is frequently used in the context of domain-driven design.

but your actual code doesn't suit completely to that as you don't invoke the same method of the repository according to the case.
So I think that you have two ways :

1) Refactoring your repository to provide a single common method accepting a specification parameter and able to handle the different cases.
If you use Spring, you could look at the JpaSpecificationExecutor interface that provides methods such as :

List<T> findAll(Specification<T> spec)

Even if you don't use Spring, I think that these examples could help you .

2) If you cannot refactor the repository, you should look for another way and provide a abstraction level about which repository methods/parameters may be passed to.

Actually, you invoke a different method with different parameters according to the input parameters but in any case you return the same type of object to the client of the method : Foo. So to avoid conditional statements, polymorphism is the way to follow.
Each case to handle is finally a different strategy. So you could have a strategy interface and you could determine the strategy to use to return the Foo to the client.

Besides, as suggested in a comment : a!=null && !a.isEmpty() repeated multiple times is not a good smell. It makes much duplication and also makes the code less readable. It would better to apply this processing by using a library such as Apache common or even a custom method.

public class FooService {

    private List<FindFooStrategy> strategies = new ArrayList<>();

    public  FooService(){
      strategies.add(new FindFooByAAndBAndCStrategy());
      strategies.add(new FindFooByBAndCStrategy());
      strategies.add(new FindFooByAAndCStrategy());
      strategies.add(new FindFooByCStrategy());
    }

    public Foo getFoo(String a, String b, String c){

       for (FindFooStrategy strategy : strategies){
            if (strategy.isApplicable(a, b, c)) {
               return strategy.getFoo(a, b, c);
            }
       }
     }
}

Where FindFooStrategy is defined as :

public interface FindFooStrategy{
  boolean isApplicable(String a, String b, String c);
  Foo getFoo(String a, String b, String c);
}

And where each subclass defines its rules. For example :

public class FindFooByAAndBAndCStrategy implements FindFooStrategy{
  public boolean isApplicable(String a, String b, String c){
      return StringUtils.isNotEmpty(a) && StringUtils.isNotEmpty(b) &&
             StringUtils.isNotEmpty(c);
  }
  public Foo getFoo(String a, String b, String c){
      return repository.findByAAndBAndC(a,b,c);
  } 
}
davidxxx
  • 125,838
  • 23
  • 214
  • 215
0

This is not a complete answer. I will offer several suggestions to address the problem at hand.

Dealing with Null Values

To avoid checking whether a value is null, I suggest that you use a container class for your String query parameters with some method, say getValue() that returns parameter's value e.g., parameter='value' if the value is present or some default string value e.g., parameter like '%' if it's null. This approach follows the so-called, Null Design Pattern.

Dynamic Construction of Query

After doing this, it will no longer matter what values the parameters you passed have and you can just construct your condition iteratively such as:

for parameter in parameters:
    condition = "AND" + parameter.getValue()

Perhaps you can combine this with a generic method for querying that accepts arbitrary length condition such as:

repository.findBy(condition)

I am not 100% sure since I am typing this answer from the top of my mind but I think this approach works and should be able to address the problem mentioned in your post. Let me know what you think.

ultrajohn
  • 2,527
  • 4
  • 31
  • 56
0

You can make use of a enum defining bitmap-constants with a valueOf method:

public enum Combinations{
    A_AND_B_AND_C (0b111),
    B_AND_C       (0b110),
    A_AND_C       (0b101),
    C             (0b100),
    A_AND_B       (0b011),
    B             (0b010),
    A             (0b001),
    NONE          (0b000),
    ;

    private final int bitmap;

    Combinations(int bitmap){
        this.bitmap = bitmap;
    }

    public static Combinations valueOf(String... args){
        final StringBuilder builder = new StringBuilder();
        for(int i = args.length - 1; i >= 0; i--){
            final String arg = args[i];
            builder.append(arg != null && !arg.isEmpty() ? '1' : '0');
        }

        final int bitmap = Integer.parseInt(builder.toString(), 2);

        final Combinations[] values = values();
        for(int i = values.length -1; i >= 0; i--){
            if(values[i].bitmap == bitmap){
                return values[i];
            }
        }

        throw new NoSuchElementException();
    }
}

And another class which has a switch case statement:

public class SomeClass {

    public Foo getFoo(String a, String b, String c){
         switch(Combinations.valueOf(a, b, c)){
             case A_AND_B_AND_C:
                 return repository.findByAAndBAndC(a, b, c);

             case B_AND_C:
                  return repository.findByBAndC(b, c);

             /* all other cases */

             case NONE:
                  return repository.findOne();

             default:
                  // type unknown
                  throw new UnsupportedOperationException();
         }
    }
}

This may be a lot of work in the first place. But you'll be glad when you've done it. By using Bitmaps you can have a lot of combinations. The valueOf method takes care of finding out which combination actually should be taken. But what should happen after can't be done generically. So when adding another parameter d you'll get a lot more combinations which must be added to the enum.

All in all this solution is overkill for small amounts of parameters. Is still quite easy to understand, because the logic is split up into many small parts. You just still don't get around the big switch statement at the end though.

Lino
  • 19,604
  • 6
  • 47
  • 65