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\$
  • \$\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 Mar 13 '16 at 22:14
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() + "%");
    }
    
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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