#15 findByCompany 메소드 추가 with QueryDSL #21

LeeJaeYun7 · 2023-08-29T00:19:27Z

findByCompany 메소드를 QueryDSL을 적용해서 추가했습니다

f-lab-moony

고생 많으셨어요 ~
코멘트를 좀 남겨 두었는데, 확인 후 리뷰 재요청 부탁드립니다 ~

f-lab-moony · 2023-09-02T15:29:17Z

src/main/java/com/example/interviewPrep/quiz/filter/JwtAuthenticationFilter.java

        String header = request.getHeader(HttpHeaders.AUTHORIZATION);

-        if (header != null && header.startsWith("Bearer")) {
+        if (header != null && header.startsWith("Bearer") && !header.endsWith("null")) {


오 .. 추가된 조건은 왜 그런거에요 ?

이게 회원가입할 때는 header가 Bearer null 이라는 문자열로 전달되는데,
그러한 경우에 저 if문에 진입하지 않아야 하므로(왜냐면 access토큰이 없으므로)
저런 조건을 추가했습니다

f-lab-moony · 2023-09-02T15:30:59Z

src/main/java/com/example/interviewPrep/quiz/member/service/MemberService.java

+        String type = memberRequest.getType();
+        String role = memberRequest.getRole();
+
+        Role memberRole = Role.USER;


이거는 요청을 받을때부터 enum 값을 받는게 낫지 않을까요 ? 지금 상황에서 USER / MENTOR가 아닌 다른 롤이 들어오면 어떻게 되나요 ?

지금 상황에서 Role은 USER/MENTOR 밖에 없기 때문에, 다른 롤이 들어오는 경우는 따로 없습니다!
이런 식으로 회원가입 요청에 전달되는 것으로 생각했습니다

enum 값으로 JSON으로 전달하려면 프론트에서 Role 클래스를 선언해야 하지 않나요?

f-lab-moony · 2023-09-02T15:32:43Z

src/main/java/com/example/interviewPrep/quiz/member/service/MemberService.java

        }

-        Member member = createMemberEntity(email, password, nickName);
+        Member member = Member.builder()


request의 필드를 굳이 다 꺼내서 여기서 만들 필요가 있을까요 ? request에서 Entity를 만드는 책임을 가져가는 건 어떨까요 ?
추가로, getter를 지양하는 이유에 대해서 한 번 찾아보시면 좋을 것 같아요 ~

넵 수정했습니다

반영 완료

f-lab-moony · 2023-09-02T15:33:44Z

src/main/java/com/example/interviewPrep/quiz/member/service/MemberService.java


    public boolean isDuplicatedEmail(String email) {
-        return memberRepository.existsByEmail(email);
+        Member member = memberRepository.findByEmail(email).orElse(null);


굳이 null로 빼서 확인 할 필요가 있을까요 ? Optional에서 isEmpty 로 검증해도 되지 않을까요 ?

넵 수정했습니다

반영 완료

f-lab-moony · 2023-09-02T15:39:17Z

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

+
+        List<Question> questions = findQuestionsByCompany(companyId);
+        if (questions.isEmpty()) {
+            throw new CommonException(NOT_FOUND_QUESTION);


요거는 약간 정하기 나름이긴 한데, List를 반환받는 api에서 하나도 안나와서 예외를 반환하는게 맞을까요 ? 빈 리스트를 반환하는게 맞을까요 ~?
클라이언트를 웹페이지로 가정하면, 하나도 없는 경우 에러메세지 혹은 에러페이지를 사용자에게 보여주게 될 것이고, 빈 리스트인 경우 빈 그리드로 표현하는 정도로 처리가 될 것 같아요 ~

애초에 저 API 요청 자체가 지정된 Company에 대해서 하는 것인데,
사실 어느 쪽으로 해도 상관은 없을 것 같습니다!
예외를 반환하든, 빈 그리드든 둘 다 잘못된 요청에 속하게 될 것 같습니다

f-lab-moony · 2023-09-02T15:41:11Z

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

+        List<Question> questions = new ArrayList<>();
+        for (QuestionCompany questionCompany : questionCompanies) {
+            Long questionId = questionCompany.getQuestion().getId();
+            Question question = questionRepository.findById(questionId).orElseThrow(() -> new CommonException(NOT_FOUND_QUESTION));


이렇게 하면 쿼리가 몇번 날아가게 될까요 ?

question의 수만큼 날아가게 될 것 같습니다.
question이 100개면 100번, 200개면 200번 이렇게 날아가게 될 것 같습니다

Inner join 적용해서 쿼리 수정했습니다

반영 완료

f-lab-moony · 2023-09-02T15:41:37Z

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

+
+    public List<QuestionResponse> makeQuestionResponses(List<Question> questions) {
+        return questions.stream().map(
+                        q -> QuestionResponse.builder()


q 같은 변수는 지양하는게 좋아요 ~ 그냥 question이라고 써주는게 좋을 것 같아요 ~

넵 수정했습니다

반영 완료

f-lab-moony · 2023-09-02T15:42:29Z

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

+                                .type(q.getType())
+                                .difficulty((q.getDifficulty()))
+                                .freeOfCharge(q.isFreeOfCharge())
+                                .build())


QuestionResponse에 정적 팩토리 메서드를 정의해보면 어떨까요 ? 이 부분도 메서드 레퍼런스로 줄여볼 수 있을 것 같아요 ~

넵 확인했습니다

f-lab-moony · 2023-09-02T15:43:18Z

...ava/com/example/interviewPrep/quiz/questionCompany/repository/QuestionCompanyRepository.java

+import com.example.interviewPrep.quiz.questionCompany.domain.QuestionCompany;
+import org.springframework.data.jpa.repository.JpaRepository;
+
+public interface QuestionCompanyRepository extends JpaRepository<QuestionCompany, Long>, QuestionCompanyCustomRepository {


#21 Answer 패키지 테스트 리팩토링

LeeJaeYun7 added 4 commits August 29, 2023 09:15

findByCompany 메소드 추가 with QueryDSL

09db7ab

findByName 파라미터명 수정

241a4f1

QuestionResponse 클래스에 id 추가

5803706

유저 회원가입 시 버그 수정

34b9f22

f-lab-moony requested changes Sep 2, 2023

View reviewed changes

LeeJaeYun7 and others added 2 commits September 4, 2023 19:00

PR 피드백 반영

4111497

Merge branch 'main' into feat/questionByCompany

88df92c

LeeJaeYun7 merged commit a1e3d61 into main Sep 4, 2023

LeeJaeYun7 added a commit that referenced this pull request Oct 3, 2023

Merge pull request #28 from f-lab-edu/fix/AnswerTest

3380852

#21 Answer 패키지 테스트 리팩토링

#15 findByCompany 메소드 추가 with QueryDSL #21

#15 findByCompany 메소드 추가 with QueryDSL #21

Uh oh!

Conversation

LeeJaeYun7 commented Aug 29, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants