I have written the following method that allows me to import from another API all the items stored there using Feign. The only issue is that the external API provides a size limit of 2000 and therefore I need to take pagination into account to get all the records.
I think that my solution is not very clean and maintainable and I'd like to use streams and newer Java features to make it shorter (like I did in the final part) but I am still learning and I cannot think of good solutions for the main part of the code.
@Service
public class ClientService {
@Autowired
private Client Client;
@Autowired
private ItemRepository itemRepository;
public List<Code> importItems() {
int itemsPerPage = 2000;
RestPage<Item> restPage = Client.getItems(itemsPerPage, 0);
List<Item> items = restPage.getContent();
int totalpages = restPage.getTotalPages();
List<Item> result = new ArrayList<Item>();
result.addAll(items);
for (int pageNumber = 1; pageNumber < totalpages; pageNumber ++) {
result.addAll(Client.getItems(itemsPerPage, pageNumber).getContent());
}
return result.stream()
.filter(itemo -> !itemRepository.existsByCode(item.getCode()))
.map(item -> itemRepository.save(new Code(item.getCodemsl)))
.collect(Collectors.toList());
}
}
Can you help me in improving this?
2 Answers 2
I see these lines in your code :
List<Item> items = restPage.getContent();
List<Item> result = new ArrayList<Item>();
result.addAll(items);
You can use the ArrayList(Collection<? extends E> c) constructor obtaining the same result:
List<Item> result = new ArrayList<>(restPage.getContent());
Your filtering code looks good to me and for the for loop adding elements to an existing list for me you made the right choice. Although it is possible to add new elements to an existing list using streams this is discouraged because easy cause of errors, see for example this thread.
You could basically turn the logic around, something like:
int totalpages = -1;
int currentPage = 0;
do {
RestPage<Item> page = Client.getItems(itemsPerPage, currentPage);
result.addAll(page.getContent());
totalpages = page.getTotalPages(); // provided, that *every* page contains the total number
currentPage++;
}
while(currentPage < totalpages); // or <=, I don't know that API
This way, you don't duplicate the fetch-code.
As streams are no goal in itself, I recommend keeping it this simple, as long as you don't have any problems with caching all results in memory at once. You could write a Spliterator which basically performs the loop-body internally, but this is a complex problem in itself.