prod #386
prod #386
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @chominju02, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 사용자가 시험 시간표를 PDF 파일로 조회하고 다운로드할 수 있는 새로운 기능을 도입합니다. 핵심적으로 백엔드 서비스, Apache PDFBox를 활용한 PDF 생성 로직, 그리고 관련 시험 및 사용자 정보를 가져오기 위한 새로운 데이터베이스 쿼리가 포함됩니다. 또한, 특정 과목명의 표시 방식을 개선하고 기존 크론 잡의 실행 스케줄을 조정했습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| List<String> subjects = examSubjectJpaRepository.findByExamApplicationId(examApplicationId) | ||
| .stream() | ||
| .map(ExamSubjectJpaEntity::getSubject) | ||
| .sorted(Comparator.comparingInt(Subject::ordinal)) | ||
| .map(Subject::getSubjectName) | ||
| .map(subject -> { | ||
| if (subject == Subject.SOCIETY_AND_CULTURE) { | ||
| return "사회·문화"; | ||
| } | ||
| return subject.getSubjectName(); | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
과목 목록을 가져와 포맷팅하는 이 로직은 GetPartnerExamTicketInfoProcessor와 새로 추가된 TimeTableService에도 중복되어 있습니다. 코드 중복을 피하고 유지보수성을 높이기 위해 이 로직을 중앙에서 관리하는 유틸리티 메서드나 서비스로 분리하는 것이 좋습니다.
| List<String> subjects = examSubjectJpaRepository.findByExamApplicationId(examApplicationId) | ||
| .stream() | ||
| .map(ExamSubjectJpaEntity::getSubject) | ||
| .sorted(Comparator.comparingInt(Subject::ordinal)) | ||
| .map(Subject::getSubjectName) | ||
| .map(subject -> { | ||
| if (subject == Subject.SOCIETY_AND_CULTURE) { | ||
| return "사회·문화"; | ||
| } | ||
| return subject.getSubjectName(); | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
과목 목록을 가져와 포맷팅하는 이 로직은 GetMemberExamTicketInfoProcessor와 새로 추가된 TimeTableService에도 중복되어 있습니다. 중복을 줄이고 향후 변경을 용이하게 하기 위해 이 로직을 공통 유틸리티로 리팩토링해야 합니다.
| private List<String> getSubjects(Long examApplicationId) { | ||
| return examSubjectJpaRepository.findByExamApplicationId(examApplicationId) | ||
| .stream() | ||
| .map(ExamSubjectJpaEntity::getSubject) | ||
| .sorted(Comparator.comparingInt(Subject::ordinal)) | ||
| .map(subject -> { | ||
| if (subject == Subject.SOCIETY_AND_CULTURE) { | ||
| return "사회·문화"; | ||
| } | ||
| return subject.getSubjectName(); | ||
| }) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
이 getSubjects 메서드의 로직은 GetMemberExamTicketInfoProcessor와 GetPartnerExamTicketInfoProcessor에도 중복되어 있습니다. 코드 중복은 유지보수를 어렵게 만들 수 있으므로, 이 로직을 별도의 유틸리티 클래스나 공통 서비스로 추출하여 재사용하는 것을 고려해 보세요. 예를 들어, Subject 이름을 포맷팅하는 책임을 가진 컴포넌트를 만들 수 있습니다.
| } catch (Exception e) { | ||
| throw new RuntimeException("Generate time-table PDF failed", e); | ||
| } |
There was a problem hiding this comment.
catch (Exception e)를 사용하여 모든 예외를 잡는 것은 잠재적인 버그를 숨길 수 있습니다. PDFBox 작업에서 주로 발생하는 IOException과 같이 더 구체적인 예외를 처리하는 것이 좋습니다. 이렇게 하면 코드의 안정성이 향상되고 오류의 원인을 더 쉽게 파악할 수 있습니다.
| } catch (Exception e) { | |
| throw new RuntimeException("Generate time-table PDF failed", e); | |
| } | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Generate time-table PDF failed", e); | |
| } |
| } catch (Exception e) { | ||
| throw new RuntimeException("Resource not found: " + classpath, e); | ||
| } |
There was a problem hiding this comment.
readAll 메서드에서도 catch (Exception e)를 사용하고 있습니다. 리소스 로딩 시에는 IOException이 발생할 가능성이 높으므로, 이를 명시적으로 처리하는 것이 좋습니다. 예외를 RuntimeException으로 다시 던지더라도, 더 구체적인 예외를 잡는 것이 코드의 의도를 명확하게 합니다.
| } catch (Exception e) { | |
| throw new RuntimeException("Resource not found: " + classpath, e); | |
| } | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Resource not found: " + classpath, e); | |
| } |
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to draw text", e); | ||
| } |
There was a problem hiding this comment.
drawText 메서드에서도 광범위한 Exception을 잡고 있습니다. PDPageContentStream의 텍스트 관련 작업은 IOException을 던질 수 있으므로, 이를 구체적으로 처리하는 것이 좋습니다. 이는 예외 처리의 정밀도를 높여줍니다.
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to draw text", e); | |
| } | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Failed to draw text", e); | |
| } |
| LEFT JOIN PaymentJpaEntity p on p.examApplicationId = ea.id | ||
| LEFT JOIN ExamJpaEntity e on ea.examId = e.id | ||
| LEFT JOIN UserJpaEntity u on ea.userId = u.id | ||
| LEFT JOIN ProfileJpaEntity pr on pr.userId = u.id |
There was a problem hiding this comment.
PaymentJpaEntity와 ExamJpaEntity에 대해 LEFT JOIN을 사용하고 있지만, WHERE 절의 p.paymentStatus = 'DONE'과 e.examDate = :examDate 조건 때문에 사실상 INNER JOIN처럼 동작합니다. JOIN으로 명시적으로 변경하면 쿼리의 의도가 더 명확해지고, 데이터베이스 옵티마이저가 더 효율적인 실행 계획을 세우는 데 도움이 될 수 있습니다.
| LEFT JOIN PaymentJpaEntity p on p.examApplicationId = ea.id | |
| LEFT JOIN ExamJpaEntity e on ea.examId = e.id | |
| LEFT JOIN UserJpaEntity u on ea.userId = u.id | |
| LEFT JOIN ProfileJpaEntity pr on pr.userId = u.id | |
| JOIN PaymentJpaEntity p on p.examApplicationId = ea.id | |
| JOIN ExamJpaEntity e on ea.examId = e.id | |
| LEFT JOIN UserJpaEntity u on ea.userId = u.id | |
| LEFT JOIN ProfileJpaEntity pr on pr.userId = u.id |
| public static TimeTableInfoResponse of( | ||
| String examNumber, | ||
| String userName, | ||
| List<String> subjects, | ||
| String schoolName | ||
| ) { | ||
| return new TimeTableInfoResponse( | ||
| examNumber, | ||
| userName, | ||
| subjects, | ||
| schoolName | ||
| ); | ||
| } |
There was a problem hiding this comment.
record는 자동으로 public 생성자를 제공하므로, 단순히 생성자를 호출하는 정적 팩토리 메서드 of는 불필요합니다. 특별한 로직이 없다면 이 메서드를 제거하고 new TimeTableInfoResponse(...)를 직접 사용하여 코드를 간결하게 만들 수 있습니다.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타