[6주차 미션] 구현 완료했습니다. 리뷰 부탁드립니다. #77

java-saeng · 2022-04-20T09:21:22Z

이번 주 미션 내용

ajax 통신을 이용하여 답글 생성, 삭제 기능 구현

api 통신

이번 미션에서는 답글 생성, 삭제 기능을 ajax 통신을 하여 구현했습니다

이전에 mvc 패턴을 이용하여 ssr로 구현한다면 답글을 생성, 삭제할 때 요청을 보내고 나서 해당 정보를 다시 가져오기 위해 http 요청을 또 한번 보내게 됩니다

하지만 fetch api를 통해 생성, 삭제할 때 요청을 보내면서 콜백 함수를 통해 해당 페이지의 정보를 가져오기 위해 다시 한번 요청 보낼 필요가 없어집니다

이로 인해 필요 없는 요청을 줄이는 이점이 있습니다

Service와 Controller layer 분리

저번에 리뷰 받았던 컨트롤러가 무거워지면서 Service와 Controller 분리를 해보았습니다

controller에서는 view로부터 데이터를 받아 service에게 데이터를 전송
service는 controller로부터 받은 데이터를 비즈니스 로직을 통해 요청에 맞게 처리
다시 controller에게 가공된 데이터를 보내면서 controller는 model에 데이터를 나타냄

위와 같은 흐름대로 코드를 작성하려고 노력했습니다

느낀 점

프론트 안하길 잘했다..

@JsonIgnore

- ajax 통신을 위해 scripts 수정 - Question과 Answer가 양방향 연관관계로 json으로 변경 시 순환참조 발생 따라서 Question에 @JsonIgnore 추가 - AnswerController의 create 메서드 반환형을 view가 아닌 Answer를 반환

@controller

- ajax 통신을 위해 scripts 수정 - AnswerRepository에 answer를 조회 시 question을 같이 가져오는 @entitygraph 사용 - AnswerController에서 view가 아닌 객체를 반환하므로 @controller -> @RestController 변경

Result - 답글 삭제 시 Result 반환하기 위해 생성 AnswerController - 답글 삭제 시 Result 반환 AnswerRepository - 지연로딩으로 인한 JSON 파싱 처리 위해 쿼리 메서드 추가

1. user1의 답글을 user2가 삭제하려고 시도 2. 에러 발생 3. user2가 자신의 답글을 삭제하려고 시도 4. 에러 발생 3.4 오류 해결

Question - 생성자 추가 QuestionController - service layer와 분리

서로 다른 에러 발생 시 다른 뷰를 보여야하기 위해 exception 추가 QuestionDeleteException - 삭제할 수 없는 exception 추가 QuestionEditException - 수정할 수 없는 exception 추가

AnswerController - ApiAnswerController와 AnswerService로 분리

답글의 개수가 모든 게시글에서 공유가 되는 버그 해결 - 1번 게시글의 답글이 2개, 2번 게시글의 답글이 2개일 경우 모든 게시글의 답글이 4개라고 측정됨

QuestionService에서 질문 삭제 상태 변경 시 해당 질문에 있는 답글들의 삭제 상태 변경을 Question 에서 수행

java-saeng · 2022-04-20T09:26:44Z

src/main/java/codesquad/qua/QuestionController.java


-        if (user == null || !isQuestionMatchUser(user, savedQuestion)) {
+        try {


Question에 대한 Exception을 customizing 한 이유입니다

QuestionEditException은 해당 질문자가 아니기 때문에 삭제할 수 없는 경우,
QuestionDeleteException은 다른 User의 답글이 존재하여 삭제할 수 없는 경우입니다

처음에는 boolean으로 처리하려다가 두 가지 경우가 있게 되어(=두 가지 view)
exception을 만들었습니다

이미 보셨을 수도 있지만 한번 읽어보시면 좋을 거 같아 공유드립니다!
https://tecoble.techcourse.co.kr/post/2020-08-17-custom-exception/

오 맞습니다 저도 사용자 정의 예외를 사용하기 전에 올려주신 링크를 봤습니다.
링크에 대해서 보면 굳이 나눌 필요 없이, IllegalArgumentException, IllegalstateException에 따라 다른 뷰를 보여줄 수 있지만, 이때는 명시적으로 보여주는게 낫지 않을까 라는 생각을 했습니다

java-saeng · 2022-04-20T10:05:11Z

src/main/java/codesquad/user/User.java

-    }
-
-    @Override
-    public boolean equals(Object o) {


저번에 말씀해주신 equals에 대한 리뷰 생각해봤습니다

말씀대로 관계형 DB에서는 "식별자(id)" 가 고유한 값으로, 해당 식별자가 동일하다면 같은 데이터라고 인식하는데 equals를 overriding한다면 모든 필드의 값이 동일해야 같은 객체라고 판단합니다

equals를 굳이 overriding할 필요없는 상황에서 overriding을 한다면 HashSet이나 HashMap에 영향을 주어 예상치 못한 상황이 발생할 수도 있습니다

따라서 User를 식별하기 위해서는 "id"로 충분하기 때문에 equals를 overriding하지 않아도된다 라는 생각입니다

https://docs.jboss.org/hibernate/stable/core.old/reference/en/html/persistent-classes-equalshashcode.html

java-saeng · 2022-04-20T15:23:58Z

src/main/java/codesquad/qua/QuestionService.java

+                                 .orElseThrow(NoSuchElementException::new);
+    }
+
+    private boolean canDeleteQuestion(Question deletedQuestion, User user) {


canDeleteQuestion은 질문을 삭제할 수 있는지 판단하는 메서드입니다

답글이 없을 경우 삭제 가능

답글이 있으나 작성자 모두 질문 작성자와 같을 경우 삭제 가능

해당 메서드는 Question이 삭제될 수 있어?라고 묻고, Question이 삭제될 수 있어/없어를 판단합니다
그래서 저는 이 부분이 Question에게 물어보고 Question이 결정하는 것이라는 생각이 들어 Question 클래스에 있어야할까라는 생각을 했습니다.

하지만 이 논리라면 아래 isQuestionMatchUser(질문 작성자와 로그인 된 User가 같은지 판단하는 메서드)도 Question에 있어야하지 않을까라는 생각을 했지만, 그렇게 되면 Question 클래스가 너무 무거워지지 않을까라는 생각도 들었습니다

리뷰어님은 어떻게 생각하시는지 궁금해서 comment 남겨봅니다!

저는 코드 재사용성 측면에서 생각해봤을 때 이 로직이 해당 Service에서만 사용되는 거라면 Question이 무거워지는 것을 고려해서 Service에서 담당하도록 구현할 것 같습니다. 반면 이 Service를 벗어난 영역에서도 사용되는 로직이라면 Question에서 담당하는 것이 중복을 줄이는 방법이지 않을까 생각합니다!

QuestionController -> QuestionService

기존 Question에 Setter를 없애서 form에서 데이터를 가져오지 못한 것 해결

기존 User에 Setter를 없애서 form에서 데이터를 가져오지 못한 것 해결

equals를 overriding 하지 않아 같은 id를 가져도 User가 같지 않다는 에러 해결

Bellroute

고생하셨습니다!

QuestionService 구현이 Controller에서 수행될 view 관련된 로직을 많이 의식해서 작성되었다는 생각이 들었는데 이 부분에 대해서 한번 생각을 나눠보면 좋을 거 같습니다. QuestionService는 Question 관련 비즈니스 로직을 수행하는 역할과 책임을 가지고, QuestionService를 참조하는 객체가 누구인지는 Service의 관심사가 아니라고 생각합니다. 요구사항 변경으로 QuestionController를 ApiQuestionController로 변경하게 된다면 (혹은 다른 Controller에서 이를 참조한다면) 기존 Controller의 로직에 의존적인 Service 또한 변경해야하는 비용을 치르게 되지 않을까 생각이 됩니다.

Bellroute · 2022-05-04T06:51:10Z

src/main/java/codesquad/answer/AnswerService.java

+public class AnswerService {
+
+    private final AnswerRepository answerRepository;
+    private final QuestionRepository questionRepository;


Answer의 비즈니스 로직을 담당하는 AnswerService에서 Question관련 데이터가 필요할 때 QuestionRepository를 직접 참조하는 방법도 있지만, QuestionService를 통해 참조하도록 할 수도 있을 거 같은데 어떤 차이가 있을까요?

Bellroute · 2022-05-04T06:56:40Z

src/main/java/codesquad/answer/AnswerService.java

+
+        if (user == null) {
+            return Result.fail("로그인 하세요");
+        }


Answer를 db에서 조회하는 로직 다음 세션 유저 체크가 이뤄지고 있습니다. 개선할 점이 있어보이는데 어떻게 생각하시나요?

오,, 이 부분은 DB 접근은 무거운 작업이기 때문에 세션을 체크하고 나서 DB 접근하는 것이 좋아보입니다
감사합니다!

이 부분만 그런게 아니라 다른 곳도 그렇게 작성되어있네요!!

Bellroute · 2022-05-04T06:58:09Z

src/main/java/codesquad/answer/ApiAnswerController.java

+
+    @PostMapping("/questions/{question-id}/answers")
+    public Result<ResponseAnswerDto> create(@PathVariable("question-id") Long questionId, @RequestBody RequestAnswerDto requestAnswerDto, HttpSession session) {
+        return answerService.create(questionId, requestAnswerDto, session);


이 요청의 응답 상태코드는 어떻게 나갈까요?

create메서드는 성공, 실패 여부없이 Result 를 반환하기 때문에 200 OK가 나갈 것이라고 생각합니다!

Bellroute · 2022-05-04T07:10:20Z

src/main/java/codesquad/qua/QuestionService.java

+    public boolean setCreateForm(HttpSession session) {
+        User user = SessionUtil.getUserBySession(session);
+
+        return user != null;


Service객체가 HttpSession를 알아야할 필요가 있을까요?

현재 요청에 대한 세션 정보가 존재하는지 판단하는 메소드로 보이는데, 현재 메소드명이 실제 역할을 충분히 설명하지 못하는 것 같습니다.

Bellroute · 2022-05-04T07:11:41Z

src/main/java/codesquad/qua/QuestionService.java

+
+        if (user == null) {
+            return false;
+        }


세션에 유저 정보가 포함되야만하는 모든 메소드에서 해당 로직이 작성되고 있는데 어떻게 하면 중복을 줄일 수 있을까요?

Bellroute · 2022-05-04T07:18:23Z

src/main/java/codesquad/qua/QuestionService.java

+        return questionRepository.findAll();
+    }
+
+    public void setUpdateForm(long id, HttpSession session) {


메소드명을 보고 Question에 대한 비즈니스로직을 수행하는 객체에서 Form까지 관여하는 건가라는 생각이 들었습니다.

update하기 위해서는 조건이 필요한데 이 조건을 검증하는 것도 Question에 대한 비즈니스로직이라고 생각해서 작성했습니다.
혹시 메서드명을 수정하는게 좋아보일 것 같은 말씀이실까요~~??

Bellroute · 2022-05-04T07:27:10Z

src/main/java/codesquad/qua/QuestionService.java

+                                 .orElseThrow(NoSuchElementException::new);
+    }
+
+    private boolean canDeleteQuestion(Question deletedQuestion, User user) {


저는 코드 재사용성 측면에서 생각해봤을 때 이 로직이 해당 Service에서만 사용되는 거라면 Question이 무거워지는 것을 고려해서 Service에서 담당하도록 구현할 것 같습니다. 반면 이 Service를 벗어난 영역에서도 사용되는 로직이라면 Question에서 담당하는 것이 중복을 줄이는 방법이지 않을까 생각합니다!

Bellroute · 2022-05-04T07:32:05Z

src/main/java/codesquad/answer/AnswerService.java

+
+        answer.changeDeletedFlag();
+
+        return Result.ok(answer.toResponseAnswerDto());


ResponseAnswerDto를 Result로 한번 더 감싸는 방식으로 구현하셨는데 어떤 이유가 있을까요?

미션 명세에서 Result를 반환하라고 해서 이렇게 작성했습니다!

Bellroute · 2022-05-04T07:39:35Z

src/main/java/codesquad/user/SingUpUserDto.java

+@Getter
+@Setter
+@AllArgsConstructor
+public class SingUpUserDto {


오타가 있는거 같습니다!

Bellroute · 2022-05-04T07:43:50Z

src/main/java/codesquad/qua/QuestionController.java


-        if (user == null || !isQuestionMatchUser(user, savedQuestion)) {
+        try {


이미 보셨을 수도 있지만 한번 읽어보시면 좋을 거 같아 공유드립니다!
https://tecoble.techcourse.co.kr/post/2020-08-17-custom-exception/

기존 코드에선 DB에서 정보를 가져오고, 세션 체크를 하여 필요없는 DB 커넥션 발생 세션 체크를 먼저 하고 DB 접근을 하여 불필요한 DB 접근을 줄이기

java-saeng added 16 commits April 16, 2022 23:58

feat : ajax 통신으로 답글 삭제

44d36f0

- ajax 통신을 위해 scripts 수정 - AnswerRepository에 answer를 조회 시 question을 같이 가져오는 @entitygraph 사용 - AnswerController에서 view가 아닌 객체를 반환하므로 @controller -> @RestController 변경

feat : Result 반환

6201077

Result - 답글 삭제 시 Result 반환하기 위해 생성 AnswerController - 답글 삭제 시 Result 반환 AnswerRepository - 지연로딩으로 인한 JSON 파싱 처리 위해 쿼리 메서드 추가

feat : lombok 의존성 추가

22a20f7

fix : 답글 삭제 기능 수정

9b47425

1. user1의 답글을 user2가 삭제하려고 시도 2. 에러 발생 3. user2가 자신의 답글을 삭제하려고 시도 4. 에러 발생 3.4 오류 해결

feat : service layer 추가

e99a965

chore : 메서드 이름 변경

2fc3998

feat : service layer 분리

fd0f752

Question - 생성자 추가 QuestionController - service layer와 분리

feat : exception customizing

b88e4dc

서로 다른 에러 발생 시 다른 뷰를 보여야하기 위해 exception 추가 QuestionDeleteException - 삭제할 수 없는 exception 추가 QuestionEditException - 수정할 수 없는 exception 추가

feat : service layer 분리

70ded2a

AnswerController - ApiAnswerController와 AnswerService로 분리

fix : 답글 개수 에러 수정

df10b19

답글의 개수가 모든 게시글에서 공유가 되는 버그 해결 - 1번 게시글의 답글이 2개, 2번 게시글의 답글이 2개일 경우 모든 게시글의 답글이 4개라고 측정됨

feat : 답글 생성 시 Result 반환

837c0a8

refactor : 질문 삭제 상태 변경 시 "질문 삭제 상태" 메서드 extract

7ea56a7

QuestionService에서 질문 삭제 상태 변경 시 해당 질문에 있는 답글들의 삭제 상태 변경을 Question 에서 수행

feat : 회원가입 시 DTO 사용

18331b5

fix : equals, hashcode 삭제

8c605ad

style : code formatting

0a731ff

java-saeng commented Apr 20, 2022

View reviewed changes

java-saeng added 4 commits April 23, 2022 00:08

refactor : @transactional 위치 변경

5358c4a

QuestionController -> QuestionService

fix : Question 수정 에러 해결

933acf5

기존 Question에 Setter를 없애서 form에서 데이터를 가져오지 못한 것 해결

fix : User 수정 에러 해결

349926b

기존 User에 Setter를 없애서 form에서 데이터를 가져오지 못한 것 해결

fix : User 동일성 판단 에러 발생

c76123b

equals를 overriding 하지 않아 같은 id를 가져도 User가 같지 않다는 에러 해결

Bellroute reviewed May 4, 2022

View reviewed changes

refactor : 오타 수정 및 DB 접근 줄이기

ae2071f

기존 코드에선 DB에서 정보를 가져오고, 세션 체크를 하여 필요없는 DB 커넥션 발생 세션 체크를 먼저 하고 DB 접근을 하여 불필요한 DB 접근을 줄이기

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[6주차 미션] 구현 완료했습니다. 리뷰 부탁드립니다. #77

[6주차 미션] 구현 완료했습니다. 리뷰 부탁드립니다. #77

java-saeng commented Apr 20, 2022

java-saeng Apr 20, 2022

Bellroute May 4, 2022

java-saeng May 6, 2022

java-saeng Apr 20, 2022

hyukjin-lee Apr 20, 2022

java-saeng Apr 20, 2022

Bellroute May 4, 2022

Bellroute left a comment

Bellroute May 4, 2022

Bellroute May 4, 2022

java-saeng May 6, 2022

java-saeng May 6, 2022

Bellroute May 4, 2022

java-saeng May 6, 2022

Bellroute May 4, 2022

Bellroute May 4, 2022

Bellroute May 4, 2022

java-saeng May 6, 2022

Bellroute May 4, 2022

Bellroute May 4, 2022

java-saeng May 6, 2022

Bellroute May 4, 2022

Bellroute May 4, 2022


		if (user == null \|\| !isQuestionMatchUser(user, savedQuestion)) {
		try {


		answer.changeDeletedFlag();

		return Result.ok(answer.toResponseAnswerDto());

[6주차 미션] 구현 완료했습니다. 리뷰 부탁드립니다. #77

Are you sure you want to change the base?

[6주차 미션] 구현 완료했습니다. 리뷰 부탁드립니다. #77

Conversation

java-saeng commented Apr 20, 2022

이번 주 미션 내용

api 통신

Service와 Controller layer 분리

느낀 점

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bellroute left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment