Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat[#124] 특정 태스크의 모립세트 불러오기 #125

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

eunseo5343
Copy link
Contributor

@eunseo5343 eunseo5343 commented Jul 17, 2024

📍 Issue

closes #124

✨ Key Changes

MsetApiController

@GetMapping("/mset/tasks/{taskId}")
//@Override
public ResponseEntity<BaseResponse<?>> getFromOtherTask(@PathVariable("taskId") Long taskId) {
TaskMsetLinkResponse response = taskMsetFacade.getFromOtherTask(taskId);
return ApiResponseUtil.success(SuccessMessage.SUCCESS, response);
}
}

TaskMsetFacade

@Slf4j
@Service
@RequiredArgsConstructor
public class TaskMsetFacade {
private final TaskService taskService;
private final MsetService msetService;
private final TaskMsetService taskMsetService;
public TaskMsetLinkResponse getFromOtherTask(Long taskId) {
Task task = taskService.getTaskById(taskId);
List<TaskMset> taskMsetList = taskMsetService.getByTaskId(taskId);
List<MsetDTO> msets = taskMsetList.stream() //
.map(taskMset -> msetService.getMsetById(taskMset.getMsetId()))
.map(mset -> new MsetDTO(mset.getId(), mset.getName(), mset.getUrl()))
.collect(Collectors.toList());
return TaskMsetLinkResponse.of(task, msets);
}
}

TaskMsetRepository

public interface TaskMsetRepository extends JpaRepository<TaskMset, Long> {
List<TaskMset> findByTaskId(Long taskId);
}

TaskService

public List<Task> getTasksByTodoTask(List<TodoTask> todoTaskList) {
List<Long> taskIdList = todoTaskList.stream().map(TodoTask::getTaskId).collect(Collectors.toList());
return taskRepository.findAllById(taskIdList);
}

  • 원래 있던거 사용..!

MsetSerive

public Mset getMsetById(Long msetId) {
return msetRepository.findById(msetId).orElseThrow(
() -> new NotFoundException(ErrorMessage.NOT_FOUND)
); }
}

  • 원래 있던거 사용..!

TaskMsetLinkResponse

public record TaskMsetLinkResponse(
Task task,
List<MsetDTO> msets
) {
public static TaskMsetLinkResponse of(Task task, List<MsetDTO> msets) {
return new TaskMsetLinkResponse(task, msets);
}
}

MsetDTO

@Getter
public class MsetDTO {
private final Long id;
private final String name;
private final String url;
public MsetDTO(Long id, String name, String url) {
this.id = id;
this.name = name;
this.url = url;
}
}

💬 To Reviewers

@eunseo5343 eunseo5343 self-assigned this Jul 17, 2024
@eunseo5343 eunseo5343 changed the title Feat/#124 Feat[#124] 특정 태스크의 모립세트 불러오기 Jul 17, 2024
@geniusYoo geniusYoo self-requested a review July 17, 2024 12:39
Copy link
Contributor

@geniusYoo geniusYoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼하게 리뷰 달아드렸으니, 전부 확인 후 반응을 남겨주세요!

구현은 전체적으로 잘 하셨네요!
리팩토링만 하면 더욱 깔끔한 코드가 될 것 같아요! 👍🏻

import lombok.Getter;

@Getter
public class MsetDTO {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dto는 클라이언트와 controller 레벨에서 주고받는 객체의 형태입니다.

여기서 MsetDTO라는 객체는 TaskMsetLinkResponse 안의 mset 객체를 표현하기 위한 VO 인것으로 보여지는데요, 조금 더 적절한 이름으로 수정해보는 것은 어떨까요?

예를 들어, MsetInTask 같은 것이 있겠네요!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 의견 감사합니다

private final String name;
private final String url;

public MsetDTO(Long id, String name, String url) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TaskMsetLinkResponse의 형태처럼 정적 팩토리 메소드 패턴을 이용한 것이 아닌, 매개변수가 있는 생성자를 이용한 특별한 이유가 있을까요?

또한, 매개변수를 모두 파라미터로 받는 생성자를 작성할 때는 Lombok의 @AllArgsConstructor가 있기 때문에 코드를 더 간결하게 만들 수도 있겠네요!

import lombok.Getter;

@Getter
public class MsetDTO {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

record가 아닌 class를 사용한 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고치겠슴니다....

public class TaskMsetService {
private final TaskMsetRepository taskMsetRepository;
public List<TaskMset> getByTaskId(Long taskId) {
return taskMsetRepository.findByTaskId(taskId);
Copy link
Contributor

@geniusYoo geniusYoo Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taskId가 존재하지 않는 경우를 핸들링해주는 코드가 필요할 것 같아요 !

Suggested change
return taskMsetRepository.findByTaskId(taskId);
taskMsetRepository.findByTaskId(taskId).orElseThrow(
() -> /* Error Handling */
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가적으로, orElseThrow()를 쓰려면 repository의 코드를 수정해야할텐데요 ! 같이 고민해보면 좋을 것 같네요 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 넵!

Copy link
Contributor Author

@eunseo5343 eunseo5343 Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀해주신대로

taskMsetRepository.findByTaskId(taskId).orElseThrow(
    () -> new NotFoundException(ErrorMessage.NOT_FOUND)
)

이렇게 바꾸고

repository는

Optional을 추가해줘야할 것 같군요!

List<TaskMset> taskMsetList = taskMsetService.getByTaskId(taskId);
List<MsetDTO> msets = taskMsetList.stream() //
.map(taskMset -> msetService.getMsetById(taskMset.getMsetId()))
.map(mset -> new MsetDTO(mset.getId(), mset.getName(), mset.getUrl()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 new 연산자로 객체를 무분별하게 만들지 않기 위해 우리는 패턴을 적용했었는데요 ...! 패턴을 적용해주면 좋을 것 같네요 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵넵...

public TaskMsetLinkResponse getFromOtherTask(Long taskId) {
Task task = taskService.getTaskById(taskId);
List<TaskMset> taskMsetList = taskMsetService.getByTaskId(taskId);
List<MsetDTO> msets = taskMsetList.stream() //
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map을 두번 엮어서 간결하게 코드를 표현해주신 것 아주 좋네요 :) !!


import java.util.List;

public record TaskMsetLinkResponse(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네이밍 좋네요 ! 👍🏻👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

근데, TaskMsetLinkResponse에는 Task는 제외해도 됩니다 :) 모립세트만 포함되면 됩니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 특정태스크의 모립세트 불러오기
2 participants