Writing a clean Predicate Builder

Question

I have an inner class for filtering:

private class RegSpecification implements Specification<Reg> {
 private final transient RegFilterDTO filter;
 private transient Predicate predicate;
 private transient CriteriaBuilder cb;
 public RegSpecification(RegFilterDTO filter) {
 this.filter = filter;
 }
 @Override
 public Predicate toPredicate(Root<Reg> root, CriteriaQuery<?> cq, CriteriaBuilder cb) {
 // this.cb = cb;
 this.predicate = cb.conjunction();
 Join<Reg, Acct> userJoin = root.join(acct, JoinType.INNER);
 predicate = eqEnumVal(predicate, cb, root.get(regType), RegType.class, filter.getRegType());
 predicate = eq(predicate, cb, userJoin.get(Acct_.id), filter.getAcctId());
 predicate = like(predicate, cb, root.get(regNumber), filter.getRegNumber());
 return predicate;
 }
}

And I made a helper class, to be used by other class as well who implements filtering by Specification (methods are public since some services are in other packages):

public class PredicateBuilder {
 public static Predicate like(Predicate predicate, CriteriaBuilder cb, Path<String> path, String value) {
 if (StringUtils.isNotEmpty(value)) {
 return cb.and(predicate, cb.like(cb.lower(path), "%" + value.toLowerCase() + "%"));
 }
 return predicate;
 }
 public static <T> Predicate eq(Predicate predicate, CriteriaBuilder cb, Path<T> path, T value) {
 if (value != null) {
 return cb.and(predicate, cb.equal(path, value));
 }
 return predicate;
 }
 public static <T extends Enum<T>> Predicate eqEnumVal(Predicate predicate, CriteriaBuilder cb,
 Path<T> path, Class<T> enumType, String name) {
 if (StringUtils.isNotEmpty(name)) {
 return eq(predicate, cb, path, Enum.valueOf(enumType, name));
 }
 return predicate;
 }
}

Question is
Can I use private local variables in my RegSpecification so I won't have to use predicate = ... every time I call the helper class and some generic method to pass the predicate and cb to the helper class (so I won't have to pass the predicate and cb in my helper class methods)?

Where I could just do something like:

PredicateBuilder.with(predicate, cb)
 ...
 like(userJoin.get(Acct_.id), filter.getAcctId());
 eq(userJoin.get(Acct_.id), filter.getAcctId());
 like(root.get(regNumber), filter.getRegNumber());
 ...
.build();

in my toPredicate method.

Or any suggestions to make the code cleaner?

(Should I also annotate the helper class with @Component?)

• Peilonrayz: Hi sophie. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the question & answer style of Code Review. As such we have rolled back your latest edit. Please see what you may and may not do after receiving answers.

Answer 1

I would suggest that the PredicateBuilder actually follows a builder like pattern, where methods are cascaded to create the object. Currently the predicate object is updated with seaparate calls to the static method in the PredicateBuilder. Rather than that you could follow the below approach which is more in line with a "builder".

public class PredicateBuilder {
 private Predicate predicate;
 private CriteriaBuilder cb;
 private List<Consumer<PredicateBuilder>> tasks = new ArrayList<>();
 
 private PredicateBuilder(){
 }
 
 public static PredicateBuilder with(Predicate p, CriteriaBuilder c) {
 PredicateBuilder instance = new PredicateBuilder();
 instance.predicate = p;
 instance.cb = c;
 return instance;
 }
 public PredicateBuilder tasks(List<Consumer<PredicateBuilder>> tasks) {
 this.tasks = tasks;
 return this;
 }
 public Predicate build() {
 for(Consumer<PredicateBuilder> task : tasks) {
 task.accept(this);
 }
 return this.predicate;
 }
 
 public PredicateBuilder like(Path<String> path, String value) {
 if (StringUtils.isNotEmpty(value)) {
 CriteriaBuilder c = this.cb;
 this.predicate = c.and(this.predicate,
 c.like(c.lower(path), "%" + value.toLowerCase() + "%"));
 }
 return this;
 }
 public <T> PredicateBuilder eq(Path<T> path, T value) {
 if (value != null) {
 CriteriaBuilder c = this.cb;
 this.predicate = c.and(this.predicate, c.equal(path, value));
 }
 return this;
 }
 public <T extends Enum<T>> PredicateBuilder eqEnumVal(Path<T> path, Class<T> enumType, String name) {
 if (StringUtils.isNotEmpty(name)) {
 return eq(path, Enum.valueOf(enumType, name));
 }
 return this;
 }
} 

Then you could invoke as below:

List<Consumer<PredicateBuilder>> tasks 
 = List.of(pb -> pb.eqEnumVal(root.get(regType), RegType.class, filter.getRegType()),
 pb -> pb.eq(userJoin.get(Acct_.id), filter.getAcctId()),
 pb -> pb.like(root.get(regNumber), filter.getRegNumber()));
return PredicateBuilder.with(predicate, cb)
 .tasks(tasks)
 .build();

This way you could provide tasks if you want them to execute conditionally or directly invoke the methods.

• sophie: Thank you! I am having trouble though with the tasks. I had to do .tasks(new Runnable[] {...}) for it to compile. Also, I have warnings like: Static member 'PredicateBuilder.tasks(java.lang.Runnable[])' accessed via instance reference and Static member 'PredicateBuilder.build()' accessed via instance reference
• sophie: I am having a problem with the instance being static - if I run 2 consecutive queries, the predicate are the same now.
• Gautham M: @sophie I have updated the code. Not much changes - instance and tasks are now removed. All methods except with are made non-static. Replace every usage of instance with this (except in the with method). And changes to how the builder is invoked. I believe it looks more clean than the first version. :-)
• Peilonrayz: Hi Gautham M. Answers must make at least one insightful observation, and must explain why the change is better than the OP's code.
• Gautham M: @sophie updated. you could do it using consumer instead of runnable. check this stackoverflow.com/q/12462079/7804477 to know why i used a list instead of varargs.