Efficient usage of Java Stream API to convert one object to another

Question

I have a model of Author which has books, awards and pricing as shown below (Not showing getters and setter just to avoid verbosity)

Author: Has books and prices for the books

public class Author {
 private long id;
 private String name;
 private Set<Book> books;
 private Set<Price> prices;
}

Book: Can have optional awards

public class Book {
 private long id;
 private String name;
 private Set<Award> awards;
 private long pages;
 private String coverType;
}

Award

public class Award {
 private long id;
 private String name;
}

Price - Price of book (Can have multiple prices for same book.. Not showing all fields just to avoid verbosity)

public class Price {
 private long id;
 private long bookId;
 private double price;
}

Now if I have an Author with Book and his prices I am trying to get/convert it to price stats which has following model

BookPriceStats - Has statkey consisting of pages, coverType, price and award(which may or may not be part of the key based on if a book has got award) and set of prices itself(I have shortened price here just for avoiding verbosity )

public class BookPriceStats {
 private Statkey bookStatKey;
 Set<Price> prices;
}
public class Statkey {
 private long pages;
 private String coverType;
 private double price;
 private Award award;
}

I already have written logic to get booking price stats for books of author which are less than equal to certain price. This is working fine

 private static List<BookPriceStats> getBookPriceStats(Author author, double price) {
 var prices =
 author.getPrices().stream()
 .filter(pr -> pr.getPrice() <= price)
 .collect(Collectors.toSet());
 var books = author.getBooks();
 Map<Statkey, Set<Price>> statKeyPricingMap = new HashMap<>();
 for (Price pr : prices) {
 var pricingBooking =
 books.stream()
 .filter(book -> pr.getBookId() == book.getId())
 .findFirst();
 if (pricingBooking.isPresent()) {
 var awards = pricingBooking.get().getAwards();
 var statkey = new Statkey();
 statkey.setPrice(pr.getPrice());
 statkey.setPages(pricingBooking.get().getPages());
 statkey.setCoverType(pricingBooking.get().getCoverType());
 if (awards==null || awards.size()==0) {
 statKeyPricingMap.computeIfAbsent(statkey, k -> new HashSet<>()).add(pr);
 } else {
 awards.stream()
 .forEach(
 a -> {
 var statkeyWithAward = new Statkey();
 statkeyWithAward.setPrice(pr.getPrice());
 statkeyWithAward.setPages(pricingBooking.get().getPages());
 statkeyWithAward.setCoverType(pricingBooking.get().getCoverType());
 statkeyWithAward.setAward(a);
 statKeyPricingMap.computeIfAbsent(statkeyWithAward, k -> new HashSet<>()).add(pr);
 });
 }
 }
 }
 List<BookPriceStats> bookPriceStats = new ArrayList<>();
 statKeyPricingMap.keySet().stream().forEach(s -> {
 BookPriceStats bookingPriceStat = new BookPriceStats();
 bookingPriceStat.setBookStatKey(s);
 bookingPriceStat.setPrices(statKeyPricingMap.get(s));
 bookPriceStats.add(bookingPriceStat);
 });
 return bookPriceStats;
 }

As mentioned, the above logic works fine and I have tested it

 public static void main(String args[]){
 Award booker = createAward(1, "Booker");
 Award nobel = createAward(2, "Nobel");
 Book book1 = createBook(1, "Book 1", Set.of(booker,nobel), 100, "Hard Cover");
 Book book2 = createBook(2, "Book 2", new HashSet<>(), 150, "Paperback");
 Book book3 = createBook(3, "Book 3", Set.of(booker), 100, "Hard Cover");
 Price price1Book1 = createPrice(1, book1.getId(), 5D);
 Price price2Book1 = createPrice(2, book1.getId(), 8D);
 Price price3Book1 = createPrice(3, book1.getId(), 12D);
 Price price1Book2 = createPrice(4, book2.getId(), 6D);
 Price price2Book2 = createPrice(5, book2.getId(), 7D);
 Price price3Book2 = createPrice(6, book2.getId(), 15D);
 Price price1Book3 = createPrice(7, book3.getId(), 5D);
 Author author = createAuthor(
 1,"Author 1",
 Set.of(book1, book2, book3),
 Set.of(price1Book1, price2Book1, price3Book1, price1Book2, price2Book2, price3Book2, price1Book3));
 System.out.println(getBookPriceStats(author, 7D));
 }

Gives me the following correct output in the above example

[BookPriceStats
 {bookStatKey=Statkey{pages=100, coverType='Hard Cover', price=5.0, award=Award{name='Nobel'}}, 
 prices=[Price{id=1, bookId=1, price=5.0}]}, 
BookPriceStats{
 bookStatKey=Statkey{pages=100, coverType='Hard Cover', price=5.0, award=Award{name='Booker'}}, 
 prices=[Price{id=1, bookId=1, price=5.0}, Price{id=7, bookId=3, price=5.0}]}, 
BookPriceStats{
 bookStatKey=Statkey{pages=150, coverType='Paperback', price=6.0, award=null}, 
 prices=[Price{id=4, bookId=2, price=6.0}]}, 
BookPriceStats{
 bookStatKey=Statkey{pages=150, coverType='Paperback', price=7.0, award=null}, 
 prices=[Price{id=5, bookId=2, price=7.0}]}]

My question here is if there is a better way to convert the object with just Java and no libraries using java streams and optionals in a better way.

Note I posted it here because someone in stackoverflow suggested this is the correct place.. There is also one question on stackoverflow.

Answer 1

Here are a few comments on your code:

Business objects

If the business objects' model is given, there is nothing to do here. Otherwise, just the way you have described it:

Price - Price of book (Can have multiple prices for same book...

Why is prices a field of Author? It makes way more sense to let each book have a list of its prices. That way, you do not need to iterate all books for every price to find their matching book.

Code style

All in all, I'd say your code style is consistent and mostly makes use of good variable names. However, there is one thing seems a bit off to me, and that is the use of var:

At first, I thought you were using it on every variable, but you are actually not. Moreover, I think var is helpful whenever you are explicitly creating a new object:

List<Integer> myList = new ArrayList<>();
var myList = new ArrayList<Integer>(); // Here its useful

However, it makes code harder to read if you use it on an expression, as then you have to mentally evaluate the expression if you want to know the data type you are dealing with:

// Here, even if you just want to know the data type of prices, you first need to know author.getPrices()
// returns a collection of Prices, and then see the stream isn't doing any mapping, and collecting to a Set
var prices =
 author.getPrices().stream()
 .filter(pr -> pr.getPrice() <= price)
 .collect(Collectors.toSet());
// Here, at a glance, you can see prices is a Set of Prices
Set<Price> prices =
 author.getPrices().stream()
 .filter(pr -> pr.getPrice() <= price)
 .collect(Collectors.toSet());

Also, I'd say using one letter variable names in lambdas makes them harder to read:

// s is not a meaningful name
statKeyPricingMap.keySet().stream().forEach(s -> {
 BookPriceStats bookingPriceStat = new BookPriceStats();
 bookingPriceStat.setBookStatKey(s);
 bookingPriceStat.setPrices(statKeyPricingMap.get(s));
 bookPriceStats.add(bookingPriceStat);
 });
// Better semantics on the variable name
statKeyPricingMap.keySet().stream()
 .forEach(statKey -> {
 BookPriceStats bookingPriceStat = new BookPriceStats();
 bookingPriceStat.setBookStatKey(statKey);
 bookingPriceStat.setPrices(statKeyPricingMap.get(statKey));
 bookPriceStats.add(bookingPriceStat);
 });

Extra note: if you have a map and want to iterate both keys and values, use entry sets, so that you do not need to query the map for values:

statKeyPricingMap.entrtSet().stream()
 .forEach(statKeyPricingEntry -> {
 BookPriceStats bookingPriceStat = new BookPriceStats();
 bookingPriceStat.setBookStatKey(statKeyPricingEntry.getKey());
 bookingPriceStat.setPrices(statKeyPricingEntry.getValue());
 bookPriceStats.add(bookingPriceStat);
 });

I'd also say getBookPriceStats is not a good method name, as that implies the method is some sort of getter (or at least, it implies it does not have much logic to it). I'd say something along the lines of createBookPriceStats is more accurate to the actual behaviour of the method.

Stream usage

You are using foreach to do both mapping and collection. Instead of:

statKeyPricingMap.keySet().stream()
 .forEach(statKey -> {
 BookPriceStats bookingPriceStat = new BookPriceStats();
 bookingPriceStat.setBookStatKey(statKey);
 bookingPriceStat.setPrices(statKeyPricingMap.get(statKey));
 bookPriceStats.add(bookingPriceStat);
 });

I'd use:

statKeyPricingMap.keySet().stream()
 .map(statKey -> {
 BookPriceStats bookingPriceStat = new BookPriceStats();
 bookingPriceStat.setBookStatKey(statKey);
 bookingPriceStat.setPrices(statKeyPricingMap.get(statKey));
 })
 .collect(Collector.toList());

Algorithm

Regarding the algorithm, and if we do not change the business objects' model, the way I'd go about it is:

First, have either extract the logic to create a StatKey to another method, or to the StatKey itself, to remove logic from getBookPriceStats.

As a rule of thumb, if you are using a lambda with multiple expressions inside it, you are doing something wrong. It would probably be more readable to extract logic to another method.

Also, I'd use a map as such:

Map<Long, Set<Price>> bookIdToPricesMap

So that you just iterate over the whole prices set once.

Then, just by iterating over the books set, you already have all the information you need to create the list of BookPriceStats.

• Sandeep Nair: Upvote + Marked as answer. Thank you so much for taking time to review and giving feedback.