Using java streams to handle incoming API requests

Question

I am using the java stream API to handle incoming requests my employee service.

A few questions I had are:

1. Does using the streams API in the way I'm using it below affect the performance of the application?

2. I have not seen any tutorials using the streams API in the way I am using it. So is it bad practice to use streams this way?

3. Does using streams this way make the code less readable?

        public Response<EmployeeDto> saveEmployee(NewEmployeeRequest request) {
            boolean isEmployeeEmailExisting = employeeRepository.existsByEmail(request.email());

            if (isEmployeeEmailExisting) {
                log.error("Employee with email {} already exists", request.email());
                throw new ApplicationException(HttpStatus.CONFLICT, "Employee Already Exists", null);
            }

            EmployeeDto employeeDto = Stream.of(request)
                .map(employeeMapper::mapToEmployee)
                .map(employeeRepository::save)
                .map(employeeMapper::mapToDto)
                .findFirst()
                .get();

            return Response.successfulResponse(HttpStatus.CREATED.value(), "Successful", employeeDto);
    }

Answer 1

A "stream" is like a list or a queue; what you're doing is intelligible, but kind of silly because there's only ever one item.

Judging from the alternative you post in the comments, it looks like the insight you're missing is that you can nest function calls.

EmployeeDto employeeDto = employeeMapper.mapToDto(
    employeeRepository.save(
        employeeMapper.mapToEmployee(request)));

Judging from your function names, there are probably a lot of other things that could be refactored too. For future reference, here on Code Review it's pretty normal to post very large blocks of code, sometimes multiple whole files, so that responders can see the whole context of what you're trying to do.

Answer 2

It is true that when you buy yourself a fine hammer, you want to go hitting all kinds of things. And when you buy an angle grinder, the world seems to be full of things that need to be cut apart. Same is with streams, when you learn about them, it seems like every problem must be solved with streams.

But this is not right. Streams are a tool for solving a very specific set of problems and your problem is not one of them.

First of all, whoever reads the code, immediately thinks that there are a multitude of values being processed. Because that is the set of problems streams are used for.

Then they notice the save operation and realize that there's going to be trouble if one of the saves fail.

Then they see the findFirst and start to scream in their heads about terminal operations and how that stream is never going to work correctly.

At some point they go back and realize that it's just a stream of one object and they wasted all this energy for nothing. Because of the stream the code became confusing and less readable.

There is nothing wrong with just laying out method calls next to each other. They're still 100% valid and good stuff. It's really efficient, because there's no unnecessary method calls needing variables stuffed into the stack and no throwaway objects being created.

final Employee employee = employeeMapper.mapToEmployee(request);
final Employee savedEmployee employeeRepository.save(employee);
final EmployeeDto employeeDto employeeMapper.mapToDto(savedEmployee);

Sure you could nest the method calls, but that's going to be a pain in the ass when one of the method calls throw an exception and you have to start debugging it.

On a related matter, I recommend splitting the employeeMapper to two classes. One that maps the request to Employee and another that maps an Employee to EmployeeDTO. This follows single responsibility principle and helps you keep your classes small and manageable.