[yeon] 미션5: XHR과 AJAX#127
[yeon] 미션5: XHR과 AJAX#127kimnayeon0108 wants to merge 14 commits intocodesquad-members-2021:kimnayeon0108from
Conversation
Add JsonProperty annotation on fields in Answer, Question, User to convert java object to Json data. Change AnswerController to ApiAnswerController. Add javascript code to use ajax code.
Added Result class to represent response.
Added countOfAnswer field.
Added AbstractEntity that have fields every domain contains.
Raname method name and location of calling method.
wheejuni
left a comment
wheejuni
left a comment
There was a problem hiding this comment.
어느덧 5단계까지 다 구현하셨네요. 미션 초반에 미션을 어려워하시던 기억이 나는데, 벌써 5단계 리뷰라니 뭔가 감동적이네요(?).
서비스 클래스의 메소드들에 대한 의견 좀 남겨 봤습니다. 서비스 클래스 설계하기 쉽지 않죠. 경험이 많이 쌓여야 능숙해지는 부분이라 당연한 현상입니다. 그 전까지 도전적인 시도를 많이 해보셨으면 좋겠어요.
피드백 확인 부탁드릴게요.
| public boolean checkValid(User user, Answer answer) { | ||
| if (answer.isAnswerWriter(user)) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
간단한 건데 이렇게 고치면 좋을 것 같아요.
| public boolean checkValid(User user, Answer answer) { | |
| if (answer.isAnswerWriter(user)) { | |
| return true; | |
| } | |
| return false; | |
| return answer.isAnswerWriter(user); |
그리고 이 메소드를 퍼블릭으로 노출해야 할 이유가 궁금해요. 만약 이 메소드만 받아다 쓰는 외부 객체가 있다면, 그건 로직 설계가 좀 잘못된 것 같기도 하거든요. 작성자 여부를 판별한 뒤에 후속 동작이 있을 것 같아서요.
| Answer answer = new Answer(loggedinUser, question, contents); | ||
| return answerService.save(answer); |
There was a problem hiding this comment.
새로운 답변 객체를 생성하는 것까지 AnswerService 에서 처리했으면 어땠을까 싶네요.
그리고 새로운 답변을 등록하기 위한 정보를 모아 AnswerCreateDto 같은 클래스를 구성해봤어도 좋은 시도였을 것 같아요.
| Answer answer = answerService.findAnswer(id); | ||
| if (!answerService.checkValid(loggedinUser, answer)) { | ||
| return Result.fail("자신의 댓글만 삭제할 수 있습니다."); | ||
| } |
There was a problem hiding this comment.
AnswerService 설계상의 아쉬운점이 여기서 보이네요. 별도의 if 분기를 타게 구성하기보단 AnswerService 에서 .findForDelete(Long id, User sessionUser) 같은 메소드 제공해줬으면 어떨까 싶어요.
유저 판별이 실패하면 빈 옵셔널 등을 리턴할 수 있게요.
|
|
||
| Question question = questionService.findQuestion(questionId); | ||
| answerService.delete(question, answer); |
There was a problem hiding this comment.
AnswerService에서 QuestionService를 갖는것이 괜찮을까요...??? 이전에 AnswerService에서 QuestionRepository를 주입받고 있어서 어색하다는 피드백을 받고 이렇게 수정했는데 만약 서비스에서 다른 서비스를 주입받는게 어색하지 않다면 answerService에서 일괄적으로 처리할 수 있게 구현 가능할거같아요!
There was a problem hiding this comment.
@wheejuni 제가 태그없이 질문을해서 못보셨나봐요ㅠㅠ 말씀드린 방법은 지양한다고 하셨으니... 다른 방법을 찾아보는게 좋겠군요.
| @PostMapping | ||
| public String create(User user) { | ||
| userService.save(user); | ||
| logger.info("user : {}", user); |
fix return type, method name of findAnswerForDelete function and the way to handle IllegalAccessException.
…differentiate it from exception regarding answer delete, IllegalAccessException.
…eck valid function.
|
@wheejuni 피드백 주신 부분중에서 서비스에서 다른 서비스 의존성 주입받는 부분 질문드린것을 제외하고 수정 완료했습니다! 수정하다보니 UserController 에서도 코드를 수정할 부분이 보여서 오래걸렸습니다😅 delete 부분에서 댓글이 본인의 댓글이 아닌 경우 IllegalAccessException을 발생시켜서 GlobalExceptionHandler에서 Result객체를 반환해서 Ajax 방식으로 에러 알림창을 띄우도록 했습니다. |
|
@kimnayeon0108 서비스에서 다른 서비스 의존성 주입받는 부분 이 어떤 거였죠? |
2 participants
안녕하세요! yeon입니다. 미션5 완료하고 코드리뷰 요청드립니다.
배포링크
이번미션은 진행하면서 AJAX나 자바스크립트 등 생소한 것들이 나와서 시간이 더 오래걸린듯 합니다.
미션 진행하면서 마주했던 문제에 대해 말씀드리면,
Answer엔티티의 question필드와 Question엔티티의 answers필드의 순환참조 문제가 발생했습니다. 검색을 통해
@JsonManagedReference와@JsonBackReference를 어노테이션을 붙이면 해결할수 있다는 글을 보고 수정했지만 또 다른 문제를 마주했습니다.브라우저 콘솔에 Uncaught TypeError: Cannot read property 'id' of undefined 이런 에러가 떴습니다. 직렬화에 포함시킬 변수(Answer의 question)에
@JsonManagedReference를 추가해줘야 했는데 반대로 추가해준 것이 문제였습니다.조원들과 공유하며 이래저래 문제를 해결하고, JSON 직렬화에 대해 학습해보았지만 사용한 어노테이션이 내부적으로 어떤 동작을 하는지는 아직 잘 모르겠습니다😰
바쁜시간에 코드리뷰에 시간 내주셔서 감사합니다🙇♀️