Writing decision statement on controller layer

Question

We are developing a REST application based on an MVC architecture.

The service layer is returning Optional<T> where T could be any class.

So on the controller layer there is a conditional statement which tests if the result is Optional.empty, and then return a custom response with data []; else, return the actual data in a custom Response HTTP response.

return ABCService
    .getById("")
    .map("custom response with actual data ")
    .orElse(Collections.empty in Custom response)
    ;

Is it a bad practice to write this code in the Controller layer?

We are returning Optional<T> because we don't want to send null. If we don't use that condition on the Controller layer we have to remove Optional returning from service layer too, which I don't think is a good practice.

Can someone please explain why the code above is not good practice? What could be the consequences?

Answer 1

It really depends on how you're doing MVC. MVC does not dictate how M, V, and C communicate. Your code makes sense if you're doing this:

But not if you're doing this:

Which you should do largely depends on how you feel about Command Query Responsibility Segregation.

Telling me you're using MVC doesn't tell me much. It's an old design pattern that has been reinvented in many different ways. The only consistent thing about it at this point is the idea of separating 3 areas of responsibility.

---

The billion dollar mistake was to allow null into the typing system. Unless you use a language that fixes that you're living with nullable types.

Carefully not returning null is a different thing. You avoid returning null because the meaning of null isn't clear from context to context and the behavior it causes is usually unintended. Returning a null object lets you control the behavior and makes the meaning clear. This can be as simple as setting String to "" rather than null.

Using Optional lets you get a similar effect without defining a null object class because Optional is effectively a container. Rather than subjecting you to a null that blows up when you dot off of it, Optional acts like an ArrayList that contains nothing.

Optional doesn't stop null from existing. It just abstracts dealing with it away. It effectively stuffs your if (foo != null) checks down into Optional so you don't have to look at them. It also only works as long as you keep using Optional. The moment you break out of Optional, and directly touch what's in there, you're right back where you started, dealing with null.

Faithfully avoiding null in your returns may sound attractive but understand that just because something says it returns Optional doesn't mean they can't still screw you and sometimes return a naked null. The typing system can't make you that promise. That's the billion dollar mistake.