Issue
I am working on a Spring Boot application and I am trying to use a more modern (Java 8+ constructs) but I am finding some difficulties trying to implement the following changes to my existing code.
I have the following service method:
@Override
public Optional<User> activateDeactivateUser(int userId, boolean isActive) throws NotFoundException {
Optional<User> retrievedUser = this.userRepository.findById(userId);
if(retrievedUser.isEmpty())
throw new NotFoundException(String.format("The user having ID or email %s was not found", userId));
return Optional.ofNullable(retrievedUser)
.map((user) -> {
log.info(String.format("****** USER E-MAIL *******", user.get().getEmail()));
user.get().set_active(isActive);
this.userRepository.save(user.get());
return user;
})
.orElseThrow(() -> new NotFoundException(String.format("The user having ID or email %s was not found", userId)));
}
As you can see this method return an Optional object. As first thing I am not sure that returning an Optional is a good practice (because if it is empty I am throwing and handling a NotFoundException).
But the main problem is that my User class is an entity class (I am using Hibernate so it contains the table mapping) I want to change the previous method in order to retrive an UserDTO object.
In order to convert an User instance to an UserDTO instance I have this ConversionService injected into my service class (I used it elsewhere)
@Autowired
ConversionService conversionService;
My main problem is that in my previous method I am using the map() operator. I have tried to change the previous service method in this way:
@Override
public UserDTO activateDeactivateUser(int userId, boolean isActive) throws NotFoundException {
Optional<User> retrievedUser = this.userRepository.findById(userId);
User userToBeUpdated = null;
if(retrievedUser.isEmpty())
throw new NotFoundException(String.format("The user having ID or email %s was not found", userId));
else
userToBeUpdated = retrievedUser.get();
return userToBeUpdated
.map((userToBeUpdated) -> {
log.info(String.format("****** USER E-MAIL *******", userToBeUpdated.getEmail()));
userToBeUpdated.set_active(isActive);
this.userRepository.save(userToBeUpdated);
return userToBeUpdated;
})
.orElseThrow(() -> new NotFoundException(String.format("The user having ID or email %s was not found", userId)));
}
My idea was to use the map() method in order to transform one object (my User object into an UserDTO object and save it on the DB applying a function).
First of all Eclipse it is giving me the following error on the map() method call:
The method map(( userToBeUpdated) -> {}) is undefined for the type User
Since I have few experience with stream I have no idea if it is a proper use case of the map() use. This because (also in the first method implementation, that worked fine) I am not implementing a real object transformation but I am changing the value of an object field and then I am saving it into the DB. In this second scenario (my second version of my service method) I have to transform from User to UsertDTO.
What am I missing? Is it legit to use map() for this use case or is it forcing the way that was intended the map()? What could be a good solution for this problem?
Solution
In the first example the if statement is entirely pointless. You can entirely drop it, since the map on the Optional will be executed if there is an actual value returned which is not null. Also, there is a code duplication for the NotFoundException.
@Override
public UserDTO activateDeactivateUser(int userId, boolean isActive) throws NotFoundException {
return this.userRepository.findById(userId)
.map((userToBeUpdated) -> {
log.info(String.format("****** USER E-MAIL *******", userToBeUpdated.getEmail()));
userToBeUpdated.set_active(isActive);
this.userRepository.save(userToBeUpdated);
return userToBeUpdated;
})
.orElseThrow(() -> new NotFoundException(String.format("The user having ID or email %s was not found", userId)));
}
Although, this code might work, the problem with this is that we have a side-effect inside the lambda in the map. It is recommended to have pure functions when we pass it as a higher-order function for a mapping operation. It would probably be better to go with something like this:
@Override
public User activateDeactivateUser(int userId, boolean isActive) throws NotFoundException {
User retrievedUser = this.userRepository.findById(userId)
.orElseThrow(() -> new NotFoundException(String.format("The user having ID or email %s was not found", userId)));
retrievedUser.set_active(isActive);
this.userRepository.save(retrievedUser);
return retrievedUser;
}
Now, your second example is more problematic. This snippet is invalid and logically makes no sense:
userToBeUpdated
.map((userToBeUpdated) -> {...})
User class is a hibernate entity, which most likely does not have a map method implemented on it, why would it? In Java usually streams and optionals offer implementation for a map. It is used apply some transformation to each object encapsulated by the stream/optional. You would want to check out the docs for streams and optionals to read more about them.
What I believe you want to do here is the following:
@Override
public UserDTO activateDeactivateUser(int userId, boolean isActive) throws NotFoundException {
User retrievedUser = this.userRepository.findById(userId)
.orElseThrow(() -> new NotFoundException(String.format("The user having ID or email %s was not found", userId)));
retrievedUser.set_active(isActive);
this.userRepository.save(retrievedUser);
return conversionService.convert(retrievedUser);
}
I expect that the ConversionService has something like a convert method, which takes an entity object and returns a DTO.
Some other notes unrelated to the question:
- Please adhere the Java coding standards. In Java, we do not use snake case for method calls. This
userToBeUpdated.set_active(isActive);should be something like thisuserToBeUpdated.setActive(isActive);
Answered By - Ervin Szilagyi
Answer Checked By - David Goodson (JavaFixing Volunteer)
