4
\$\begingroup\$

Here's a piece of code from codebase I maintain/contribute to. It is a Spring REST controller, which exposes an endpoint to create user question. The question consists of title, description and reference to the user who asked it.

It feels it's a bit of problematic from the design standpoint. That said, the person who originally wrote this controller is more experienced than I am so I want to make sure. To me, the method below seems bloated and violating the Single Responsibility Principle (SRP)

@RequiredArgsConstructor
@Validated
@RestController
@RequestMapping("/api/v1/user/questions")
public class UserQuestionRestController {

    private final QuestionService questionService;
    private final TagService tagService;
    private final QuestionResponseDtoService questionResponseDtoService;

    @PostMapping
    public ResponseEntity<Void> create(@RequestBody @Valid QuestionRequestDto dto,
                                       @NotNull Authentication authentication) {
        Question question = QuestionMapper.toEntity(dto);
        Account currentUser = (Account) authentication.getPrincipal();
        question.setOwner(currentUser);
        List<Tag> tags = new ArrayList<>();
        if (dto.tagIds() != null) {
            tags = tagService.getByIds(dto.tagIds());
        }
        question.setTags(new HashSet<>(tags));
        questionService.create(question);
        return ResponseEntity.status(HttpStatus.CREATED).build();
    }
public final class QuestionMapper {

    private QuestionMapper() {
    }

    public static Question toEntity(QuestionRequestDto dto) {
        Question question = new Question();
        question.setTitle(dto.title());
        question.setDescription(dto.description());
        return question;
    }
}

First, the controller does so much more that simply receiving a request and sending a response. Second, there's an attempt to separate concerns with the QuestionMapper static method toEntity() which doesn't do the "DTO → entity" mapping in its entirety anyway. The owner and tags fields are set in the controller itself. That is, it's what seems to me a SRP violation happening twice

If I am right, and there's a need to move all that logic from the controller somewhere else, what that "somewhere else" should be? I could make QuestionMapper a @Component, add and @Autowire a service field, and then set all the fields in the toEntity() method (I also doubt it should be static in this case). Eventually, I ditched the QuestionMapper altogether and do the conversion with the help of a service instance.

@Validated
@RequiredArgsConstructor
@RestController
@RequestMapping("/api/v1/user/questions")
public class UserQuestionRestController {
    private final QuestionService service;
    @PostMapping
    public ResponseEntity<Void> create(@RequestBody @Valid QuestionRequestDto dto,
                                              @NotNull Authentication authentication) {
        Question question = service.mapFromDto(dto, authentication);
        service.create(question);
        return ResponseEntity.created(/* URL */).build();
    }
}

Would you agree that this is the better way to do it?

Here is the Question entity for completeness.

@AllArgsConstructor
@NoArgsConstructor
@Getter
@Setter
@Entity
@EntityListeners(AuditingEntityListener.class)
@Table(name = "questions")
public class Question {

    @Setter(AccessLevel.NONE)
    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "id", unique = true, nullable = false, updatable = false)
    private Long id;

    @Setter(AccessLevel.NONE)
    @NotNull
    @CreatedDate
    @Column(name = "created_date", nullable = false)
    private LocalDateTime createdDate;

    @Setter(AccessLevel.NONE)
    @NotNull
    @LastModifiedDate
    @Column(name = "modified_date", nullable = false)
    private LocalDateTime modifiedDate;

    @NotBlank
    @Column(name = "title", nullable = false)
    private String title;

    @NotBlank
    @Column(name = "description", nullable = false, columnDefinition = "text")
    private String description;

    @NotNull
    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "account_id", nullable = false, updatable = false)
    private Account owner;

    @NotNull
    @ManyToMany
    @JoinTable(name = "questions_tags",
            joinColumns = @JoinColumn(name = "question_id", nullable = false),
            inverseJoinColumns = @JoinColumn(name = "tag_id", nullable = false),
            uniqueConstraints = @UniqueConstraint(columnNames = {"question_id", "tag_id"}))
    private Set<Tag> tags = new HashSet<>();
\$\endgroup\$
9
  • 1
    \$\begingroup\$ The original controller method seems quite clear, does what needs to be done when a question is created, and returns a proper response. To my eyes, redesigning this to be "more clean" would be mostly just rearranging furniture. \$\endgroup\$
    – topsail
    May 21 at 2:06
  • \$\begingroup\$ @topsail what about SRP? \$\endgroup\$
    – Sergey
    May 21 at 2:26
  • \$\begingroup\$ Honestly, the original controller is too fat. It contains logic and it shouldn't. But there is something even worse. Your controllers (both) are aware of question entity which belongs to persistence layer which is more than just one layer off of HTTP layer. The controller should just call one inner service method restQuestionService.create(title, , description, tagIds, currentUser) which in turn calls any mappers, generic question service, generic tags service, and those in turn call persistence layer. Treat entities as internal part of persistence layer rather than general in memory model. \$\endgroup\$
    – slepic
    May 21 at 6:04
  • \$\begingroup\$ @slepic where can I read about it? Is there some authoritative source I can reference? \$\endgroup\$
    – Sergey
    May 21 at 7:47
  • \$\begingroup\$ @slepic also, isn't that method call too huge? It contains four arguments, and Robert Martin said you should keep the number of method parameters as close to zero as you can \$\endgroup\$
    – Sergey
    May 21 at 7:57

1 Answer 1

2
\$\begingroup\$

Regarding the mapper

public final class QuestionMapper {
    public static Question toEntity(QuestionRequestDto dto) {
        ...
    }
}

The way the mapper is implemented is an invitation to SRP violations. You have a mapper for a type, and the mapper is responsible for converting that type to whatever other object type there is a need for. What happens if you need to create a QuestionSummary object? By the way the mapper is named, the toQuestionSummary() method would go to QuestionMapper. With your design, the possible set of responsibilities for a given mapper are not restricted. I have seen the worst possible results for this approach and they were not pretty.

My preferred choice is to "reverse" the mapper. A QuestionMapper would be responsible for creating Question objects from whatever type there is. While this doesn't guarantee 1-to-1 mapping, it has worked really well for me and the mappers that have multiple mapping methods are a rarity.

My chosen name for all mapping methods is from. This ensures that the naming is always consistent between mappers and that naming conflicts are a sign of a SRP violation or other code smell. And there is no need to spend time figuring out the correct name for a mapping method (e.g. should your method be toDto, toEntity or toQuestionRequest?) The name of the input parameter for the from-method is always source. The name for the target object is always... target. These last two are a matter of taste, you could use "input" and "output" or whatever, as long as the choice is consistent throughout the codebase.

The mappers are components that get injected. A mapper may depend on another mapper, in case the mapped type is a complex object or contains common mappings. The dependencies are again injected. This allows efficient unit testing as the mappers can be easily mocked.

Your mapper would thus become:

@Component
public class QuestionMapper {
    public Question from(final QuestionRequestDto source) {
        final Question target = new Question();
        ...
        return target;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you! What do you think about abandoning the mapper and swapping it for a static factory method like (with more specific naming) DTO.fromEntity(entity)? QuestionMapper.from(dto) (if that's your proposal) kind of suggests that you get a QuestonMapper object as a result \$\endgroup\$
    – Sergey
    May 22 at 5:41
  • \$\begingroup\$ Static factory method has the same issues as your current implementation. It does not allow for trivial mocking for testing purposes. It also loads the controller with extra responsibilities. The context solves the naming issue quite well. It's not being called like a static method. Instead it becomes Question question = questionMapper.from(dto); \$\endgroup\$ May 22 at 7:41

Your Answer

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

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