1
\$\begingroup\$

I wrote the code and would like to change it to more readable and elegant:

@Repository
public class ProductRepositoryImpl implements ProductCustomRepository {

private static final int MORE_THEN_RESULT = 1;

@PersistenceContext
private EntityManager em;

public List<Product> findByFilter(Filter filter) {
    final CriteriaBuilder cb = em.getCriteriaBuilder();
    final CriteriaQuery<Product> cq = cb.createQuery(Product.class);
    final Root<Product> from = cq.from(Product.class);

    try {
        final List<Predicate> predicates = createPredicatesByFilter(cb, from, filter);
        if (!CollectionUtils.isEmpty(predicates)) {
            cq.where(cb.and(predicates.toArray(new Predicate[predicates.size()])));
        }
    } catch (RuntimeException e) {
        return Collections.emptyList();
    }

    cq.select(from);

    return em.createQuery(cq).getResultList();
}

private List<Predicate> createPredicatesByFilter(CriteriaBuilder cb, Root<Product> from, Filter filter) {
    final List<Predicate> predicates = new ArrayList<>();

    if (!StringUtils.isEmpty(filter.getName())) {
        predicates.add(cb.like(cb.lower(from.<String>get("name")), filter.getName().toLowerCase() + "%"));
    }

    final BigDecimal priceBefore = filter.getPriceBefore();
    final BigDecimal priceAfter = filter.getPriceAfter();

    if (priceBefore != null && priceAfter != null) {
        if (priceBefore.compareTo(priceAfter) != MORE_THEN_RESULT) {
            predicates.add(cb.between(from.<BigDecimal>get("price"), priceBefore, priceAfter));
        } else {
            throw new RuntimeException();
        }
    } else if (priceBefore != null) {
        predicates.add(cb.gt(from.<BigDecimal>get("price"), priceBefore));
    } else if (priceAfter != null) {
        predicates.add(cb.lt(from.<BigDecimal>get("price"), priceAfter));
    }

    if (!StringUtils.isEmpty(filter.getCategory())) {
        predicates.add(cb.like(cb.lower(from.<Category>get("category").<String>get("name")), filter.getCategory().toLowerCase() + "%"));
    }

    return predicates;
}
}
\$\endgroup\$
1
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. \$\endgroup\$
    – Jamal
    Commented Mar 13, 2016 at 22:14

1 Answer 1

2
\$\begingroup\$

In general this looks okay to me. In fact I have a similar pattern in an application with lots of optional filters. Newer Java 8 features which can make this a bit nicer (e.g. using Optional and method references).

  • Throwing/catching RuntimeException like that looks iffy. At least add a message to the exception so the reader knows how to interpret this.
  • Returning a List only to immediately convert it to an array doesn't sound that useful, then again, if it's being used from other places it might be okay. List also has isEmpty, better than having to call yet another static function for it.
  • CriteriaQuery.where is already using the conjunction, no need to add the and there. Note that the array can be passed for the varargs arguments:

    if (!CollectionUtils.isEmpty(predicates)) {
        cq.where(predicates.toArray(new Predicate[predicates.size()]));
    }
    
  • For the the string matching queries additional helper methods might be nice:

    private static startsWithLower(Expression<String> expression, String pattern) {
        return cb.like(cb.lower(expression), pattern.toLowerCase() + "%");
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.