필요없는 TODO 삭제, 질문을 위한 TODO 추가 #75
Conversation
Update README.md
Update README.md
| @@ -21,12 +21,14 @@ public class VoteController { | |||
|
|
|||
| // like, unlike, 자기상태, 수정, 삭제 | |||
|
|
|||
| // TODO : GET 메서드로 요청할 때, Query Parameter, Path Variable로 보낼지에 대해 고민입니다. | |||
| @GetMapping("/vote") | |||
| public ResponseEntity<VoteEntity> getVote(@PathVariable Long questionId, HttpServletRequest httpServletRequest) { | |||
There was a problem hiding this comment.
이 부분에 관해서는 REST API 및 RESTful의 컨셉에 대해서 생각해볼 필요가 있습니다.
https://ko.wikipedia.org/wiki/REST
REST API에서 path variable을 사용하는 경우엔 해당 경로에 해당하는 자원이 서버에 존재해야합니다. 그렇지 않은 경우에 404 Not found 에러를 만나게될 것이구요. 이 부분에서 적절한 questionId 를 사용한 경우엔 데이터를 받아봐야 하기 때문에 이 경우엔 path variable이 적절해보입니다만, 다른 부분은 어떨지 직접 고민 해보시면 좋을것 같아요
| * 로그인 시 -> 지금 로직 그대로 | ||
| * 로그인 안했을 시 -> loginId == null, | ||
| */ | ||
| // TODO: 세션에서 로그인한 유저id를 가져왔을 때 로그인한 유저 체크하는 로직이 이 방식이 맞을까요? | ||
| public VoteEntity getVote(Long questionId, HttpServletRequest httpServletRequest) { |
There was a problem hiding this comment.
이 방식대로 하더라도 의도한대로는 동작 할것으로 보입니다! 다만, 이렇게 직접 메서드에서 체크하는 경우, 세션체크의 누락 및 동일하지 않은 세션 체크 로직이 사용될 문제의 소지가 있습니다. 세션체크를 하는 서비스를 따로 만들거나 혹은 Spring에서 제공하는 HandlerInterceptor등을 이용해서 구현하는편이 더 나아보입니다
| @@ -36,6 +36,7 @@ public QuestionController(QuestionService questionService) { | |||
| // ex) /question?nickname=김영한 | |||
| // 2. 만약 검색 조건이 2개 이상 들어오면, 예외를 발생 | |||
|
|
|||
| //TODO : 페이지네이션 관련 로직을 서비스로 빼야하는 것은 알고 있지만, 서비스로 비즈니스 로직을 분리할 지 모르겠습니다. | |||
| @PostMapping("") | |||
There was a problem hiding this comment.
시간이 더 걸리더라도 비즈니스 로직을 분리하는것을 추천 합니다. 시간은 부족한데 일단은 코드가 동작하니 냅두자~ 라고 마인드를 가지게 된다면, 추후 모든 작업에서 그런 태도를 가질수 있게되죠. 이는 좋지 않은 습관입니다. 특히, 정말 어려운 작업이라면 어쩔수 없겟지만, 비즈니스로직을 서비스로 분리하는것 자체는 간단한편에 속하는 일이기 때문에 이 부분은 꼭 지키셨으면 하는 바램입니다.
|
먼저 질문에 대한 코멘트를 남겨두었고, 전체적인 리뷰 추가로 작성할예정입니다~ |
|
불필요한 파일 및 주석들이 남아있음 코딩 스타일 관련
테스트 코드가 없음 |
|
일단 눈에 띄는것은 이정도네요~ 추가적인 코멘트가 필요한 부분이 있다면 말씀 해주세요~ |
No description provided.