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

[Team-16] BE 2번째 PR입니다. #108

Closed
wants to merge 12 commits into from

Conversation

street62
Copy link

@street62 street62 commented Apr 8, 2022

안녕하세요 리뷰어님! 16조 백엔드를 맡은 Heyho, Lee입니다.

2번째 PR 보내드립니다.
첫 번째 PR(#30) 에서 남겨 주신 질문에 대해서 저희 나름대로 생각한 것들을 댓글로 달아 보았습니다. 확인해주시면 감사하겠습니다!

작업 내용

데이터베이스 연결

AWS EC2 서버의 MySQL 데이터베이스를 datasource로 하여 프로젝트와 연결했습니다.

투두리스트 태스크 추가 API 작성

TaskController.add() 메서드를 통해 유저가 투두리스트에 작성한 태스크를 DB에 추가하는 API를 작성했습니다.

API 명세 공유를 위한 Swagger 연결

iOS 팀과 원활한 API 명세를 공유하기 위해 Swagger를 도입해 TaskController.add() API와 연결했습니다.

그럼 이번에도 잘 부탁드립니다.
감사합니다!

street62 added 8 commits April 8, 2022 17:12
build.gradle 수정으로 JDBC와 MySQL Connector 의존성 추가
application.properties 수정으로 datasource 정보 추가. 이후 업데이트 필요.
aws 관련 설정은 별도 파일을 include 하도록 설정.
.idea 폴더는 전체 제외하도록 수정
할 일 목록에 Task를 추가하는 API 구현.
API 명세를 위한 Swagger 의존성 추가 및 초기 설정.
현재 TaskController.add()의 API 정보를 swagger로 연결한 상태.
@street62 street62 added the review-BE Improvements or additions to documentation label Apr 8, 2022
@street62 street62 requested a review from wheejuni April 8, 2022 08:23
Copy link

@wheejuni wheejuni left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다. 💯
몇 가지 코멘트 남겼으니 확인 부탁드립니다

id 'java'
}

group 'org.example'
group 'com.list'

Choose a reason for hiding this comment

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

group ID 정하는 방법은 혹시 알고 계시나요? 보통 도메인의 역순입니다.
코드스쿼드 프로젝트니까 kr.codesquad 이면 좋긴 하겠는데 이건 개인 취향이라 맘대로 하시면 되겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

참고해서 변경했습니다. 감사합니다!

@ApiResponse(code = 500, message = "서버에서 발생한 에러입니다.")
})
@PostMapping("/task")
public ResponseEntity<Object> add(@RequestBody Task task) {

Choose a reason for hiding this comment

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

Task 타입을 내려주는 거 같은데 왜 ResponseEntity<Object> 인지요?

Copy link
Author

Choose a reason for hiding this comment

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

리뷰 이후에 상태코드 200의 경우에도 body 없이 상태코드를 담은 헤더만 반환하는 방식으로 바꾸었는데요. 이 경우에 ResponseEntity의 제네릭 부분을 삭제(바디를 사용하는 응답이 없으므로)하면 인텔리제이에서 경고가 뜨는 것 같습니다. 그래서 우선은 Object로 두었는데 혹시 바디가 없는 경우에는 어떤 식으로 제네릭을 지정해주면 좋을까요?

Comment on lines 41 to 43
} catch (DataIntegrityViolationException j) {
j.printStackTrace();
return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();

Choose a reason for hiding this comment

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

서비스 클래스에서 캐치하고 결과를 내려야 할 로직으로 보입니다만.

Copy link
Author

Choose a reason for hiding this comment

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

말씀 주신 내용을 요청으로 들어온 Task 객체의 유효성을 체크하고 그에 맞는 responseEntity를 생성하는 역할이 컨트롤러가 아니라 서비스 단에서 이루어져야 한다는 의미로 이해했는데 맞을까요? 컨트롤러에서는 서비스에서 생성한 responseEntity를 전달받아 그대로 리턴하는 방식으로 변경했습니다.

응답코드가 200일 때에도 body에 Task 객체를 넣지 않고 응답 코드만 반환하도록 변경.
클라이언트 단에서는 응답코드만 확인하면 될 뿐 객체 리턴이 불필요하다고 판단함.
ResponseEntity를 Controller가 아닌 Service에서 생성하고, Controller는 해당 엔티티를 받아 리턴하는 형식으로 변경.
패키지명은 kr.codesquad.todo, build.gralde의 group ID는 kr.codesquad로 변경.
@street62 street62 force-pushed the dev-BE branch 3 times, most recently from 2dd6d86 to 2249d49 Compare April 13, 2022 13:42
@street62
Copy link
Author

머지가 조금 늦어져서 2번째 PR을 닫고 3번째 PR에 2번째 PR 커밋까지 넣어서 진행하려 합니다. 3번째 PR(#184) 확인해 주시면 감사하겠습니다!-!

@street62 street62 closed this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants