Here’s a bit of code (not mine). The method seems bloated, in violation of Single Responsibility Principle (SRP). However, a person who wrote this code is more experienced than I am so I want to make sure I am right – or wrong. You don’t have to know much about the app. It’s enough to know that it does basic REST stuff. In this case, saving a Question
instance in a database
@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;
}
}
@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<>();
First, the controller does so much more that simply receiving a request and sending a response. It almost spills over Robert Martin’s 15 line limit. 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). Or, I prefer it, ditch the QuestionMapper
altogether and do the conversion with the help of a service instance, roughly like this
@Validated
@RequiredArgsConstructor
@RestController
@RequestMapping("/api/v1/user/questions")
public class UserQuestionRestController {
private final QuestionService service;
private static final BASE_URI = "/api/v1/user/questions";
@PostMapping
public ResponseEntity<Void> create(@RequestBody @Valid QuestionRequestDto dto,
@NotNull Authentication authentication) {
Question question = service.mapFromDto(dto, authentication);
Question savedQuestion = service.create(question);
Long generatedId = savedQuestion.getId();
return ResponseEntity.created(URI.create(BASE_URI + "/" + generatedId)).build(); // btw, do you think I should bother with StringBuilder here?
}
}
What do you think? What is the “best practice” design when it comes to REST controllers?