find matches between attributes from two different Objects

Question

I'm new in Java 8, and I'm trying to convert and optimize my method from java 6 to java 8. I want it to be less verbose, with fewer lines of code.

This method gets an object (userRequest) and tries to find from a list of other object type (User) the one with more matches between their attributes(for example, if the user requested has brown eyes, each user from the list who has brown eyes has one more match). If it results that there is more than one User, so the one with less age is chosen.

In this example it's only used a list of 3 users and only 3 or 4 attributes, but in the future it's intended to be more, so I'd like to optimize it for multiple users and attributes.

@Getter
@Setter
@AllArgsConstructor
@ToString
public class UserRequest{
 private String name;
 private String surname;
 private String company;
 private String eyesColor;
}
@Getter
@Setter
@AllArgsConstructor
@ToString
public class User{
 private Long id;
 private String name;
 private String surname;
 private String eyesColor;
 private int age;
}
@Service
public class UserServiceImpl implements UserService{
 
 @Autowired
 private UserRepository userRepository;
 
 @Autowired
 private UserFilters userFilters;
 
 public int getMostMatchingUserId(UserRequest userRequest){
 
 List<User> userList = userRepository.getUsers();
 
 List<User> mostMatchingUserList = userFilters.getMostMatchingUserList(userList, userRequest);
 
 return mostMatchingUserList.stream().min(Comparator.comparing(User::getAge)).orElse(mostMatchingUserList.get(0)).getId();
 }
}
@UtilityClass
public class UserFilters{
 public List<User> getMostMatchingUserList (List<User> userList, UserRequest userRequest) {
 List<User> newUserList = new ArrayList<>();
 int matchesInitialCount = 0;
 
 for(User user: userList){
 int userMatchesCount = countMatches(user, userRequest);
 if(userMatchesCount > matchesInitialCount){
 newUserList.clear();
 newUserList.add(user);
 matchesInitialCount = userMatchesCount;
 } else if(userMatchesCount == matchesInitialCount){
 newUserList.add(user);
 }
 }
 return newUserList;
 }
 
 private int countMatches (User user, UserRequest userRequest) {
 int matchesCounted = 0;
 
 if (Objects.nonNull(user.getName()) && user.getName().equals(userRequest.getName())) {
 matchesCounted++;
 } 
 if (Objects.nonNull(user.getSurname()) && user.getSurname().equals(userRequest.getSurname())) {
 matchesCounted++;
 } 
 if (Objects.nonNull(user.getAge()) && user.getAge() == userRequest.getAge() {
 matchesCounted++;
 } 
 return matchesCounted;
 }
}

Imagine you get a new userRequest ("John", "Jackson", "aena", "brown") to check which one of the next userList has the most matches:

User1 (111, "John", "Wine", "blue", 37);
User2 (222, "Maria", "Jackson", "brown", 25);
User3 (333, "Michel", "Jackson", "brown", 55);

The result is: User1 has one match and User2 & User3 have two matches each. The next validation checks who is the youngest one from User2 & User3: so User2 will be the chosen one.

• pacmaninbw: Simplify your methods as much as possible, try to follow the single responsibility principle which is violated by at least getMostMatchingUserList().
• BCdotWEB: The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?.
• coloma1984: I've already changed the title and made it more readable and simplified. Just my main purpose is to reach the goal optimized and adapted to Java 8. @BCdotWEB
• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! I rolled back your last edit. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). See the section What should I not do? on What should I do when someone answers my question? for more information

Answer 1

First off, a bug (or probably just a copy/paste error): countMatches is comparing age instead of eye color.

Then, any reason you are limited to Java 8? At the very least you should be using the LTS release 11, or better the current release 15.

Also, you have some minor formatting issues that don't confirm with standard Java conventions. Mostly it's wrong spacing: there should be spaces before { and between keywords (e.g. if) and (, but no spaces between method names and (.

Lombok

The annotations on the data classes could be most likely simplified to @Data @AllArgsConstructor. And you should even consider using @Value instead for immutable objects when ever possible, which can help avoiding bugs.

Spring DI

Generally autowiring private properties has been discouraged. You should prefer constructor injection. With Lombok's @RequiredArgsConstructor it becomes quite easy:

@Service
RequiredArgsConstructor
public class UserServiceImpl implements UserService{
 
 private final UserRepository userRepository;
 
 private final UserFilters userFilters;
 //....
}

Since you are injecting UserFilters it shouldn't be static i.e. @UtilityClass.

For a matter of fact I don't see why UserFilters is a separate class at all. Those methods don't look like they should be used outside of UserService, so they should belong as (private) methods in UserService.

Method UserServiceImpl.getMostMatchingUserId

The name isn't quite right and doesn't give enough information in my opinion. It's not a "getter", so it should be called find... and nt get..., the meaning "Most matching" needs to be defined in a comment and the word "youngest" needs to be added.

Also it seems quite limiting to have the method return just the user ID.

One problem in here is the use of orElse on the result of min(). min() only returns an empty Optional, if the stream is empty, so in that case .get(0) will throw an exception.

A final though here, is that normally one would expect that the repository (or usually the database behind it) should filter the list of results. If the database would do the filtering, then this would scale better with more users.

Method UserFilters.getMostMatchingUserList

Also not a "getter". filter... would be the most appropriate prefix here.

One could consider using a stream here, however in this specific case I don't think it would make the task much simpler/better to read. The only change I'd make here would be choose some better variable names: newUserList should be called filteredUserList and matchesInitialCount should be currentMatchesCount.

Method UserFilters.countMatches

This implementation is fine. Personally I'd leave it like this, and just add/change more property checks when needed, unless the list of properties needs to changed quickly or easily, for example, via configuration. It that case you need to work with reflection to find the properties.

• coloma1984: Thanks for your answer @RoToRa, it's been very useful. I'm limited to Java 8 cause this is an analogous code from a project already set in Java 8. My main question is: would it be possible to shorten method 'countMatches'? e.g. as you said, using Java Reflection? In the future, 'User' and 'UserRequest' may have 20, 30 or 50 attributes and it would make a very large and few optimized method. Would it be Java Reflections highly recommended in that case? or are there better options? Thanks.
• RoToRa: Reflection would make it really slow. Especially with many properties and users. One alternative would be to return the users as maps instead of objects from the repository.