[Team-16] BE 3번째 PR입니다! #184
Conversation
build.gradle 수정으로 JDBC와 MySQL Connector 의존성 추가 application.properties 수정으로 datasource 정보 추가. 이후 업데이트 필요.
aws 관련 설정은 별도 파일을 include 하도록 설정.
.idea 폴더는 전체 제외하도록 수정
할 일 목록에 Task를 추가하는 API 구현.
API 명세를 위한 Swagger 의존성 추가 및 초기 설정. 현재 TaskController.add()의 API 정보를 swagger로 연결한 상태.
응답코드가 200일 때에도 body에 Task 객체를 넣지 않고 응답 코드만 반환하도록 변경. 클라이언트 단에서는 응답코드만 확인하면 될 뿐 객체 리턴이 불필요하다고 판단함.
ResponseEntity를 Controller가 아닌 Service에서 생성하고, Controller는 해당 엔티티를 받아 리턴하는 형식으로 변경.
패키지명은 kr.codesquad.todo, build.gralde의 group ID는 kr.codesquad로 변경.
sourceCompatibility를 11로 설정하여 서버의 자바 버전과 맞춤.
200 응답 시 body에 Task 객체 없이 헤더만 응답하는 것으로 변경하여 해당 내용 삭제.
컬럼명 등 변경된 DB 구조에 맞게 변경.
디버깅 용으로 작성한 출력문 삭제
TaskController.load() 메서드와 이어지는 service, repository의 메서드를 구현해 데이터베이스에서 전체 Task를 불러오는 기능 구현.
DB에 태스크를 저장할 시 Localdatetime.now()의 기준이 되는 타임존 값을 ZoneId.of()를 사용해 KST로 설정.
전체 태스크 조회 API에 대한 설명을 Swagger에 반영. 응답코드에서 400(BAD REQUEST) 내용 삭제.
Task 추가 API 성공 시 response body에 Task 객체를 반환하도록 하여 json으로 전달.
/task/status 경로에 patch 요청 시 해당 idx를 가진 Task의 status를 변경하도록 구현. Service 단에서 매개변수의 범위가 올바르지 않을 때는 BAD REQUEST를 리턴하도록 하는 검증 로직(isChangeInfoInvalid) 추가.
상태 수정 API의 Swagger 설명을 추가.
태스크 상태 변경 API(TaskController.move()에서 기존에는 @RequestParam으로 인자를 받아 처리했으나, 이 경우 json 형태로 요청을 보냈을 때 동작하지 않는다는 문제가 있었음. 따라서 TaskStatusChangeDto 클래스를 새로 만들고 해당 클래스의 객체를 @RequestBody 형태로 받는 방식으로 변경.
| @RestController | ||
| @Api(value = "TaskController v1") | ||
| public class TaskController { | ||
| TaskService taskService; |
There was a problem hiding this comment.
접근 제어자 누락입니다~ 반복적으로 말씀드리는 기본적인 컨벤션은 신경서주세요~
(제가 말씀드린 적이 없다 하더라도 다른 어떤 리뷰어라도 말씀을 드린 적이 있을 것 같습니다)
| @ApiResponse(code = 500, message = "서버에서 발생한 에러입니다.") | ||
| }) | ||
| @GetMapping("/task") | ||
| public ResponseEntity<Map<Integer, List<Task>>> load() { |
There was a problem hiding this comment.
Map<Integer... 형태로 리스폰스가 넘어가야 하는것이 이해가지 않는 부분이 있긴 합니다.
이런 상황을 위한 DTO가 설계되어야 하지 않나 생각이 듭니다.
| this.taskRepository = taskRepository; | ||
| } | ||
|
|
||
| public ResponseEntity<Task> createTask(Task task) { |
There was a problem hiding this comment.
Service 에서 ResponseEntity 를 넘기라는 의미가 아닙니다~ ㅠㅠ
Task 가 도메인 클래스인데 도메인 클래스가 들고 있는 모든 정보가 리스폰스로 넘어가야 하는 필요성에 대한 질문을 드린 것입니다.
Task 가 갖고 있는 정보 중에서도 사용자가 관심사를 가질 정보는 한정된다고 생각하고, 특히 각 API endpoint마다 노출되어야 할 정보는 다르다고 생각합니다. 그 상황마다의 DTO가 설계되어야 한다는 말씀이었고 프레젠테이션 레이어로 넘어갈땐 그 DTO의 형태로 넘어가야 한다고 생각한 것입니다.
당연히 그걸 받아서 그대로 내리든 ResponseEntity 로 감싸서 내리든 그건 프레젠테이션 레이어의 책임과 판단이고 서비스 클래스에서 해당 로직에 강 결합될 이유가 없습니다.
하여 여기서 ResponseEntity 에 의존하는 형태는 빼주셨으면 합니다.
| for (Task task : tasks) { | ||
| switch (task.getStatus()) { | ||
| case 1: | ||
| statusOne.add(task); | ||
| break; | ||
| case 2: | ||
| statusTwo.add(task); | ||
| break; | ||
| case 3: | ||
| statusThree.add(task); | ||
| break; | ||
| default: | ||
| return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
스위치문의 형태로 봤을때 Task.getStatus() 의 리턴 타입은 int 이면 안될 것 같습니다.
스위치문 없이 분기 로직이 수행되려면 객체가 어떤 정보들을 갖고 있어야 할까요?
| try { | ||
| task = taskRepository.changeStatus(idx, status); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
.printStackTrace() 는 많은 경우에 무의미합니다. 그리고 taskRepository.changeStatus() 가 던지는 예외가 정말 Exception 타입인지요? 그렇다면 그것도 문제입니다.
상황에 맞는 커스텀 예외를 적극 설계해 주시기 바랍니다.
| public boolean isChangeInfoInvalid(long idx, int status) { | ||
| return !(status == 1 || status == 2 || status == 3) || idx < 1; | ||
| } |
There was a problem hiding this comment.
status 를 어떤 타입으로 변경한 뒤 이부분도 반드시 리팩터링 되어야 할 겁니다.
|
|
||
| @RestController | ||
| @Api(value = "TaskController v1") | ||
| public class TaskController { |
There was a problem hiding this comment.
해당 코드가 보이진 않는데 여기에 답변 남깁니다. 만약 상태 코드만 응답결과로 필요하다면 @ResponseStatus 사용을 적극 추천 드리고, 모든 메서드의 리턴타입이 ResponseEntity<T> 여야 하는 것은 아닙니다.
|
@wheejuni Brian, 저희가 이번 미션을 2주 간 계속해서 페어 프로그래밍으로 진행 해와서 제가 조금 지쳐 오늘은 조금 일찍 마무리를 하게 되었습니다. 여태까지의 경험상 이 후의 미션을 진행하며 Todo 미션의 코드를 수정하는 것은 어려울 것 같다는 생각이 들어 메세지를 남깁니다. 구현 예정 이였던 요구사항 들을 멈추고, 리뷰로 남겨주신 내용들을 Lee와 함께 읽어보며 수정 방향에 대해 많은 이야기를 나누었고, 의도하셨던 바와 정확히 일치하는 방향은 아니더라도, 해당 키워드에 대한 내용을 검색하고 찾아보며 더 나은 구조에 대한 고민과 생각을 이야기 해볼 수 있었습니다. 다음 프로젝트를 진행할 때 배정 된 페어와 함께 남겨주신 리뷰에 대한 고민들을 공유하며 구조를 나눠보면 좋겠다는 이야기를 나누었고, 리뷰로 남겨주신 |
안녕하세요 리뷰어님. 16조 백엔드를 맡은 Heyho, Lee입니다.
3번째 PR을 보내드립니다.
저희가 저번 PR이 머지되지 않은 상태에서 해당 브랜치에 계속 커밋을 진행해서
기존 dev-BE 브랜치를 리셋하고 새로운 브랜치를 설정하는 과정에서 PR이 다소 늦어졌네요. 죄송합니다.
저번 PR(#108) change request 이후 머지가 안 된 상태여서 해당 PR 커밋도 붙어있는 상황인데요.
혼동이 있을 수 있어서 2번째 PR을 닫고 이번 PR로 통합했습니다! 이전 리뷰 반영 내용도 확인해주시면 감사하겠습니다.
작업 내용
2번째 PR 반영내역
질문사항 (#108 PR 코멘트 참조해주시면 됩니다!)
API 서버 AWS EC2에 배포
서버를 AWS EC2에 배포했습니다. EC2에서 직접 빌드하니 EC2가 뻗어버려서, 로컬에서 빌드한 jar 파일을 EC2에서 돌리는 방식으로 진행했습니다.
태스크 목록 받아오기 API 구현
TaskController.load()에서 시작해 service->repository로 이어지는 api입니다.
repository에서 전체 Task를 List 형태로 받아오고,
service에서 해당 리스트 내부에 있는 Task들의 status(1: todo, 2: doing, 3: done) 값을 확인해 각각 1, 2, 3을 key로 하고 List를 값으로 하는 해시맵의 value 부분에 append하는 방식입니다.
태스크 상태 수정 API 구현
TaskController.move()에서 시작해 service -> repository로 이어지는 API입니다.
repository에서 UPDATE 쿼리를 날린 이후, SELECT를 통해 변경된 Task를 받아와 클라이언트 단에 Response body로 넣어 리턴하는 방식입니다.
그럼 이번에도 잘 부탁드립니다. 감사합니다!