1
\$\begingroup\$

This is the code that joins the list with another one. Also makes use of a static utility to makes use of hashmap for id and name.

I've added null checks to safeguard the code with NPE.

The key purpose is to make this readable. Could someone help code review this?

public List<PGRatingResponse> collatePGRatingsbyVendor(List<PGRatingConfig> pgRatingSansPgname,
            List<Vendor> vendors) {
        

        return pgRatingSansPgname.stream().filter(Objects::nonNull).filter(o1 -> o1.getRating() != null).map(o1 -> {
            vendors.stream().filter(Objects::nonNull).filter(o2 -> (o2.getVendorId()).equals(o1.getVendorId())
                    && o1.getRating() != null && o1.getVendorId() != null).filter(Objects::nonNull).findAny();
            return new PGRatingResponse(o1.getVendorId(),
                    vendors.stream().filter(o -> o.getVendorId() != null && o.getVendorId().equals(o1.getVendorId()))
                            .findFirst().get().getVendorName(),
                    o1.getRating().stream().filter(Objects::nonNull)
                            .filter(o2 -> o2.getPgId() != null && o2.getValue() != null)
                            .map(o -> new RatingResponse(
                                    LookupTable.pgLookupTable().getOrDefault(o.getPgId() == null ? "5" : o.getPgId(), "Default"),
                                    o.getPgId() == null ? "5" : o.getPgId(), o.getValue()))
                            .collect(Collectors.toList()));

        }).collect(Collectors.toList());
    }
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review, please add an explanation about what the code does. \$\endgroup\$ Jun 22, 2021 at 11:12
  • \$\begingroup\$ Welcome to Code Review! Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Jun 22, 2021 at 12:17

1 Answer 1

7
\$\begingroup\$

One giant stream with nested streams with nested streams is very difficult to read. Streams are not a hammer, and not all problems are nails. Code should not try to ram things into streams when they make the card harder to read and understand.

The line breaks here are not where I'd expect them to be, and I think it makes the code harder to read.

You are filtering on not null in several places. Is it actually possible for the inputs to be null in all those places. Typically null values are not added to java collections objects, for instance. Anyplace where a value couldn't be null, the null check is just confusing.

Silently ignoring the null values in these places is probably a bug. Without context I can't be sure, but it's very sketchy. Do you really want these null values floating around, with objects containing them being silently ignored?

The stream of the vendors which ends with findAny() returns a value, but that value is ignored. It has no side effects. That stream does nothing and should be removed.

Variable names like o and o1 are hard to keep track of. Use meaningful variable names like vendor.

Prefer multiple filter() calls over using && in a filter().

Building a map once and looking up in it is more efficient and easier to read than streaming through a list inside a constructor argument.

Is "5" the default rating PgId everywhere? If so, why is the value nullable at all? Just assign it to "5" if it isn't assigned.

It's generally suggested that only the first letter of an acronym in a name is capitalized. So PgRatingResponse would be preferable to PGRatingResponse.

Building the RatingResponse is complex enough to be extracted into its own method.

If you made all these changes, your code might look more like:

public List<PgRatingResponse> collatePgRatingsbyVendor(
        List<PgRatingConfig> pgRatingSansPgname,
        List<Vendor> vendors) {

    Map<Long, String> vendorNamesById = vendors.stream()
            .filter(vendor -> vendor.getVendorId() != null)
            .collect(toMap(vendor -> vendor.getVendorId(), vendor -> vendor.getVendorName()));

    return pgRatingSansPgname.stream()
            .filter(Objects::nonNull)
            .filter(pgRatingConfig -> pgRatingConfig.getRating() != null)
            .map(pgRatingConfig ->
                new PgRatingResponse(
                    pgRatingConfig.getVendorId(),
                    vendorNamesById.get(pgRatingConfig.getVendorId()),
                    pgRatingConfig.getRating().stream()
                        .filter(Objects::nonNull)
                        .filter(rating -> rating.getPgId() != null)
                        .filter(rating -> rating.getValue() != null)
                        .map(rating -> responseFor(rating))
                        .collect(Collectors.toList())))
            .collect(Collectors.toList());
}

private RatingResponse responseFor(PgRating rating) {
    // These two variables need better names
    String lookupKey = rating.getPgId() == null ? "5" : rating.getPgId();
    String lookupValue = LookupTable.pgLookupTable().getOrDefault(lookupKey, "Default");
    return new RatingResponse(lookupValue, lookupKey, rating.getValue());
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I suggest moving the big lambda pgRatingConfig -> new PgRatingResponse(...) into a method, too. \$\endgroup\$
    – mtj
    Jun 23, 2021 at 4:21
  • \$\begingroup\$ There's no need to cram lots of streams operations into single huge chained expressions, starting with .stream() and containing everything up to .collect(). Just with any other Java expression, it's quite possible and useful to introduce well-named local variables for intermediate elements like streams or lambdas. \$\endgroup\$ Jun 23, 2021 at 9:24

Your Answer

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

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