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<>();