#10 Question 패키지 리팩토링 및 테스트 작성 #11

LeeJaeYun7 · 2023-08-15T06:36:05Z

Question 패키지를 리팩터링하고, 테스트를 작성했습니다

f-lab-moony

고생 많으셨어요 ~ 코멘트 좀 남겨 두었는데, 확인 후 리뷰 재요청 해주세요 ~

f-lab-moony · 2023-08-17T11:03:42Z

src/main/java/com/example/interviewPrep/JpaAuditingConfiguration.java

+import org.springframework.data.jpa.repository.config.EnableJpaAuditing;
+
+@Configuration
+@EnableJpaAuditing


요거 메인 클래스에 선언해도 되지 않나요 ?

테스트 작성하는데 이걸 따로 선언해줘야 하더라고요
그래서 선언했습니다!

테스트 작성과 어떤 관계가 있는걸까요 ?

테스트 할 때 깨지는 부분이 있었는데...
다시 해보니 InterviewPrepApplication 클래스에 @EnableJpaAuditing 하는 걸로 해결되서
수정해서 커밋했습니다

f-lab-moony · 2023-08-17T11:04:20Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

+    }

-    @GetMapping("/single/{id}")
+    @GetMapping("/api/v1/question/single/{id}")


중간에 single 굳이 안써도 되지 않을까요 ~?

네 맞습니다. 그 부분은 수정해보겠습니다

반영 완료

f-lab-moony · 2023-08-17T11:05:16Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

-    @GetMapping("/all")
-    public ResultResponse<?> getAllQuestionsSize(){
-        return ResultResponse.success(questionService.getAllQuestionsSize());
+    @GetMapping({"/api/v1/question/{type}", ""})


type은 id의 성격이 아니라서 by-type 같이 추가 정보를 요구한다는걸 나타내주는게 좋을 것 같아요

그럼 @GetMapping({"/api/v1/question/by/{type}", ""})
이렇게 나타내는걸 말씀하시는 걸까요?

@GetMapping("/api/v1/questions/by-type/{type}") 이런식으로 말씀드렸어요 ~ api를 쓰는 사람 입장에서 endpoint는 메서드같은 역할을 하기때문에 메서드 이름처럼 의미가 잘 드러나게 하는게 좋을 것 같아요

p.s @ 을 그냥 쓰시면 다른 사람이 멘션되니까 `` 요거로 감싸주시는게 좋아요 ~

반영 완료

f-lab-moony · 2023-08-17T11:07:57Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

-    @GetMapping("/all")
-    public ResultResponse<?> getAllQuestionsSize(){
-        return ResultResponse.success(questionService.getAllQuestionsSize());
+    @GetMapping({"/api/v1/question/{type}", ""})


뒤에 빈 문자열은 무슨 뜻일까요 ?

type이 null일때를 의미하는 것 같은데... 굳이 필요하지 않을 것 같습니다

type이 null일때라고 생각하신 이유가 있을까요 ? 연결되는 endpoint가 복수개라는 뜻으로 보이는데, 추가 path 없이 host만 입력된 요청도 얘가 받겠다 라는 뜻이 아닐까요 ?

다시 생각해보니 말씀하신게 맞는 것 같습니다!

f-lab-moony · 2023-08-17T11:09:57Z

src/main/java/com/example/interviewPrep/quiz/question/dto/QuestionRequest.java

+import java.util.Objects;
+
+@Getter
+@Builder


빌더는 생성자에 달아주는걸 추천합니다 ~

넵 확인했습니다
https://johngrib.github.io/wiki/pattern/builder/

반영 완료

f-lab-moony · 2023-08-17T11:11:48Z

src/main/java/com/example/interviewPrep/quiz/question/service/QuestionService.java

-    public List<Question> findQuestionsByType(String type){
-        return questionRepository.findByType(type);
+    public Question findQuestion(Long id){
+        return questionRepository.findById(id).orElseThrow(() -> new CommonException(NOT_FOUND_QUESTION));


요청받은 id가 뭐였는지도 메세지에 드러나면 좋을 것 같아요 ~

id는 Quesiton의 id인데 이 부분도 반영해보겠습니다

뜻이 약간 잘못 전달된 것 같아서 .. 파라미터 명을 바꾸자는게 아니라 메세지에 드러났으면 좋겠다는 의견이었습니다 ~

반영 완료

f-lab-moony · 2023-08-17T11:15:04Z

src/main/java/com/example/interviewPrep/quiz/question/service/QuestionService.java

                        .type(q.getType())
                        .title(q.getTitle())
-                        .status(myAnswer.contains(q.getId()))
+                        .status(answers.contains(q.getId()))


contains 연산을 수행할때 List와 Set 중 뭐가 유리할까요 ?

ArrayList를 사용할 경우엔 O(N)이지만,
HashSet을 사용할 경우엔 O(1) 이므로, Set을 사용하는 편이 유리할 것 같습니다

반영 완료

f-lab-moony · 2023-08-17T11:19:52Z

src/main/java/com/example/interviewPrep/quiz/question/service/QuestionService.java

-        if(type==null) questions = questionRepository.findAllBy(pageable);
-        else questions = questionRepository.findByType(type, pageable); //문제 타입과 페이지 조건 값을 보내어 question 조회, 반환값 page
-        if(questions.getContent().isEmpty()) throw new CommonException(NOT_FOUND_QUESTION);
+        Page<Question> questions = findQuestionsByTypeAndPageable(type, pageable);


사실 지금 수준에선 문제가 없겠지만, offset 방식의 페이징은 서비스가 커지면 반드시 문제가 생겨요. more 방식 페이징에 대해서 한번 찾아보시고 습관 들이시는걸 추천합니다~

https://green-bin.tistory.com/m/23
넵 참고해보겠습니다

f-lab-moony · 2023-08-17T11:23:07Z

...java/com/example/interviewPrep/quiz/Question/controller/QuestionCreateWebControllerTest.java

    void setUp() throws Exception{

-        questionDTO = QuestionDTO.builder()
+        validQuestionRequest = QuestionRequest.builder()


검증하려는 데이터는 그 테스트 내부에서 정의되는게 테스트 가독성을 더 높일 수 있다고 생각합니다 ~

넵 확인했습니다

f-lab-moony · 2023-08-17T11:25:30Z

...java/com/example/interviewPrep/quiz/Question/controller/QuestionDeleteWebControllerTest.java

-        when(questionService.deleteQuestion(1L)).thenReturn(question);
-        when(questionService.deleteQuestion(1000L)).thenThrow(new QuestionNotFoundException(1000L));
+        given(questionService.deleteQuestion(1L)).willReturn(question);
+        given(questionService.deleteQuestion(1000L)).willThrow(new QuestionNotFoundException(1000L));


이거 이렇게 선언하면 아래 테스트들 잘 돌아가나요 ?

f-lab-moony

고생 많으셨습니다 ~
코멘트를 좀 남겨 두었는데, 요거는 머지 해둘게요 ~ 확인 후 반영 부탁드립니다 ~

f-lab-moony · 2023-08-24T14:57:37Z

build/reports/tests/test/index.html

 <td>
 <div class="infoBox" id="failures">
-<div class="counter">0</div>
+<div class="counter">1</div>


gitignore에 build 하위 항목을 추가해주시는게 좋을 것 같아요 ~

위에 .gradle 하위 항목도 추가해주시는게 좋을 것 같습니다 ~

f-lab-moony · 2023-08-24T14:58:47Z

src/main/java/com/example/interviewPrep/InterviewPrepApplication.java

+import org.springframework.data.jpa.repository.config.EnableJpaAuditing;

 @EnableCaching
+@EnableJpaAuditing


오 근데 auditing 원래 있지 않았나요 ?

f-lab-moony · 2023-08-24T14:59:12Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

+import java.util.List;

 @RestController
+@RequestMapping("/api/v1/questions")


f-lab-moony · 2023-08-24T14:59:31Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

-    @PostMapping("/api/v1/question")
-    public ResultResponse<?> create(@RequestBody @Valid QuestionRequest questionRequest){
-        return ResultResponse.success(questionService.createQuestion(questionRequest));
+    @PostMapping()


요럴땐 괄호 그냥 없애도 될거같아요 ~

f-lab-moony · 2023-08-24T15:00:39Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

-    public ResultResponse<?> create(@RequestBody @Valid QuestionRequest questionRequest){
-        return ResultResponse.success(questionService.createQuestion(questionRequest));
+    @PostMapping()
+    public ResponseEntity<Void> create(@RequestBody QuestionRequest questionRequest) {


요전에 말씀 드리긴 했는데 ResponseEntity 꼭 써야할까요 ~?

f-lab-moony · 2023-08-24T15:08:08Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

-    @GetMapping("/api/v1/question/all")
-    public ResultResponse<?> getTotalQuestionsCount(){
-        return ResultResponse.success(questionService.getTotalQuestionsCount());
+    @GetMapping("/all")


count가 질문의 갯수를 리턴한다면 /all 은 안어울리는 이름 같아요

f-lab-moony · 2023-08-24T15:09:09Z

src/main/java/com/example/interviewPrep/quiz/question/controller/QuestionController.java

-    @GetMapping("/api/v1/question/filter")
-    public ResultResponse<?> getFilterLanguage(){
-        return ResultResponse.success(questionService.findFilterLanguage());
+    @GetMapping("/filter")


filter 라고 하면 어떤 기능인지 알 수 있을까요?

f-lab-moony · 2023-08-24T15:10:56Z

src/main/java/com/example/interviewPrep/quiz/question/domain/Question.java

    }

-    public void change(String title, String type){
+    public static Question createQuestionByQuestionRequest(QuestionRequest questionRequest) {


의존성 방향을 바깥에서 안쪽으로만 허용해주시는 습관을 들이시는게 좋을 것 같아요. 클린 아키텍처에 대해서 한번 찾아보시는 걸 추천드립니다 ~

f-lab-moony · 2023-08-24T15:12:42Z

src/main/java/com/example/interviewPrep/quiz/question/dto/QuestionResponse.java

+        this.status = status;
+    }
+
+    public static QuestionResponse createQuestionResponse(Question question){


요런건 보통 이름을 그냥 of나 from 으로 쓰는 편이에요!

f-lab-moony · 2023-08-24T15:14:26Z

src/main/java/com/example/interviewPrep/quiz/question/service/QuestionService.java

-                        .title(q.getTitle())
-                        .status(answers.contains(q.getId()))
-                        .build());
+                .id(q.getId())


변수명을 줄여쓰지 않으시는게 좋아요 ~ oo씨 라고 지칭하는것과 거기! 라고 지칭하는 것 중 어떤게 더 명확한 표현일까 하는것과 같은 내용이라고 생각합니다 ~

#11 Answer 패키지 리팩토링

LeeJaeYun7 added 3 commits August 15, 2023 15:30

Question 패키지 Controller, Service 클래스 및 Test 리팩토링

a002506

remove unused imports

bdbfa44

QuestionController 클래스 create 메소드 URL 수정

9891eca

f-lab-moony requested changes Aug 17, 2023

View reviewed changes

LeeJaeYun7 added 5 commits August 21, 2023 14:00

PR 피드백 반영

dcb95c2

PR 피드백 반영2

7af8bc7

Question 패키지 리팩토링 & Question 인수 테스트 리팩토링

d25b475

AnswerRepository findMyAnswer 반환 타입 수정

8ccb696

QuestionRepositoryTest 버그 수정, JpaAuditingConfiguration 클래스 삭제

ab7978d

f-lab-moony approved these changes Aug 24, 2023

View reviewed changes

f-lab-moony merged commit ea653cc into main Aug 24, 2023

f-lab-moony added a commit that referenced this pull request Aug 24, 2023

Merge pull request #15 from f-lab-edu/fix/AnswerRefactoring

b18d888

#11 Answer 패키지 리팩토링

#10 Question 패키지 리팩토링 및 테스트 작성 #11

#10 Question 패키지 리팩토링 및 테스트 작성 #11

Uh oh!

Conversation

LeeJaeYun7 commented Aug 15, 2023

Uh oh!

f-lab-moony left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f-lab-moony Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f-lab-moony left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f-lab-moony Aug 20, 2023 •

edited

Loading