2
\$\begingroup\$

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);
    }
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Welcome to Code Review! Can you just tell us why you decided on using the Stream API for this? \$\endgroup\$ Mar 20 at 15:18
  • \$\begingroup\$ Hey Simon, as I am some what new to java, I was more or less just playing around with the streams api. I am looking to see if there is any benefit to handling the incoming data this way versus handling it in a more traditional approach that I have seen in tutorials.. Employee employee = employeeMapper.mapToEmployee(request); Employee savedEmployee = employeeRepository.save(employee); EmployeeDto dto = employeeMapper.mapToDto(savedEmployee); \$\endgroup\$ Mar 20 at 15:27

2 Answers 2

6
\$\begingroup\$

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.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ That is not valid Java. \$\endgroup\$
    – Bossie
    Mar 21 at 6:17
  • 3
    \$\begingroup\$ I think your :: should just be . \$\endgroup\$ Mar 21 at 7:52
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ i think your program flow is far more intuitive than the one from ShapeOfMatter. Your way reduces the cognitiv complexity by 3 which is so much more preferable. thank you for pointing this out (and man, i know some 'friends' that see nails everywhere with their new stream-hammer) \$\endgroup\$ Mar 26 at 6:30

Your Answer

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

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