Add ability to search books #6
Conversation
…s to specification. Added AbstractSpecificationProvider.
|
|
||
| public record BookSearchParametersDto( | ||
| String[] author, | ||
| String[] title, |
There was a problem hiding this comment.
I do not see much sense in getting several titles, we usually do a search with partial match of book title with some single search string that we receive here
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| public class TitleSpecificationProvider extends AbstractSpecificationProvider<Book> { |
There was a problem hiding this comment.
and so as a follow up to the comment above, I would override the method that you have in abstract spec provider with something that would use SQL LIKE to have a partial match search
| private static final int MIN_PRICE = 0; | ||
| private static final int MAX_PRICE = 1; |
There was a problem hiding this comment.
that's not just the price these are indexes
| private final List<SpecificationProvider<Book>> bookSpecificationProviders; | ||
|
|
||
| @Override | ||
| public SpecificationProvider<Book> getSpecificationProvider(String key) { | ||
| return bookSpecificationProviders.stream() | ||
| .filter(p -> p.getKey().equals(key)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new RuntimeException( | ||
| "Can't find specification provider for " + key)); | ||
| } |
There was a problem hiding this comment.
having a map here instead of a list would be more efficient, but that is not too important
| .getSpecification(new String[]{ | ||
| params.minPrice(), | ||
| params.maxPrice() | ||
| }); |
There was a problem hiding this comment.
I do not think it is a good approach to build an array and operate it instead of clean and structured data in object that we have. Let's remove AbstractSpecificationProvider and make SpecificationProvider.getSpecification() receive not a string[] but a whole BookSearchParametersDto where the impl classes will take only the data that they need to.
| } | ||
|
|
||
| private Specification<Book> getPriceSpecification(BookSearchParametersDto params) { | ||
| if (params.minPrice() != null && params.maxPrice() != null) { |
There was a problem hiding this comment.
what if we receive only the max price filter, should it be ignored?
| return Stream.of( | ||
| getSpecification(params.author(), BookSearchParameter.AUTHOR.getKey()), | ||
| getSpecification(params.title(), BookSearchParameter.TITLE.getKey()), | ||
| getSpecification(params.isbn(), BookSearchParameter.ISBN.getKey()), | ||
| getPriceSpecification(params) | ||
| ) | ||
| .filter(Objects::nonNull) | ||
| .reduce(Specification.where(null), Specification::and); |
…SpecificationProvider class, made changes in parameter specification providers. Change implementation of build() method id BookSpecificationBuilder.
| Map<String, Function<BookSearchParametersDto, Specification<Book>>> | ||
| specificationMap = Map.of( | ||
| AUTHOR, params -> params.author() != null ? getSpecification(params, AUTHOR) : null, | ||
| TITLE, params -> params.title() != null ? getSpecification(params, TITLE) : null, | ||
| ISBN, params -> params.isbn() != null ? getSpecification(params, ISBN) : null, | ||
| PRICE, params -> (params.minPrice() != null || params.maxPrice() != null) | ||
| ? getSpecification(params, PRICE) : null |
There was a problem hiding this comment.
this looks a bit too complicated, would be nice to simplify it somehow
There was a problem hiding this comment.
I understand but i really want to avoid 4 if in row)
| public Specification<Book> getSpecification(BookSearchParametersDto params) { | ||
| return (root, query, criteriaBuilder) -> criteriaBuilder.like( | ||
| root.get(getKey()), "%" + params.isbn() + "%"); |
There was a problem hiding this comment.
we could actually organize a search by 2 fields, like so:
some searchString param that we get in dto, is LIKE ISBN OR LIKE title but that is a feature for another day 😄
| private static final int DEFAULT_MIN_PRICE = 0; | ||
| private static final int DEFAULT_MAX_PRICE = Integer.MAX_VALUE; |
There was a problem hiding this comment.
A point for optimizing the queries. Would be nice to not send the default params when we did not receive them in DTO because they are redundant, like:
- if we do not receive
minPrice&maxPrice- query should not have that price filtration at all - if we receive only
minPrice- query should be justwhere price > minPrice - if we receive only
maxPrice- query should be justwhere price < minPrice
No description provided.