Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Step3] 수강신청(DB 적용) #132

Merged
merged 118 commits into from
May 29, 2023
Merged

[Step3] 수강신청(DB 적용) #132

merged 118 commits into from
May 29, 2023

Conversation

moto6
Copy link

@moto6 moto6 commented May 29, 2023

안녕하세요 리뷰어님 [Step3] 수강신청(DB 적용) PR 올립니다!

  • 이번 PR 에서 다룬 내용들은 아래와 같습니다

비기능적 요구사항

  • 학습목표 달성하기 : 도메인 구조 유지하면서 DB 테이블과 매핑
  • 도메인 객체에 로직 구현에 집중하자. DB 조회성능은 후순위이다
  • 도메인 구조를 유지하는게 관건이다. DB 쿼리가 성능이 하락하는 trade-off 를 감수하더라도

기능적 요구사항

  • 과정(Course) 관련 CRUD Repository method 구현
  • 강의(Session) 관련 CRUD Repository method 구현
  • 과정(Course)은 기수 단위로 여러 개의 강의(Session)를 가지는 Repository method 구현
  • 이외 Domain Class 에 대한 CRUD Repository method 구현

이외 추가사항

마지막으로 질문 ✋🏻 하나 있습니다!

  • 저어.. 이거 다른 분들이 올리신 PR 보면 대략 커밋 10개 내외인데, 저만 커밋이 많은거같아서.. 혹시 제가 뭘 너무 어렵고 복잡하게 진행했거나..? 아니면 뭘 잘못하고 있는건지 궁금합니다 ㅠㅡㅠ;....

moto6 added 30 commits May 23, 2023 10:52
Copy link

@brainbackdoor brainbackdoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3단계 미션 진행하느라 고생하셨어요
JdbcTemplate 때문에 고민한 흔적이 여기저기 보이네요;;
일단 커밋이 많이 보이는건.. 이전 단계 커밋도 PR에 추가된거 같네요. 그 외에도 기능 단위로 묶어서 커밋하거나 (누락된 코드 커밋 등), 리팩토링 커밋들 제외하면 3단계 미션을 위한 커밋은 그리 많지는 않은걸로 보여요.

3단계 미션은 도메인 객체를 깨트리지 않으며 디비와 매핑하는게 주요 요구사항이에요.
이 부분은 충실히 수행하신거 같아요. infrastructure layer에 너무 힘 안쓰셔도 될거 같네요 ㅎ
(JPA나 mybatis와 같은 기술보다는 침투력이 적어 사용했던 거라, jdbcTemplate 쿼리 성능이나 구조가 본질은 아니었어요. 욕심이 있으시다면, 현재 반복적으로 발생하는 코드를 한번 더 wrapping할 수는 없을지 고민해보는 것도 좋겠네요. 그 코드가 발전해가면 query mapper와 유사한 형태가 되어가는걸 볼 수 있을거에요)

미션에서 요구하는 부분은 충분히 달성한 것으로 보여 여기서 머지할게요~
좋은 밤 보내세요~

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
}

private void validateImageUrl(String imageUrl) {
if(imageUrl==null || imageUrl.isBlank()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(imageUrl==null || imageUrl.isBlank()) {
if (imageUrl == null || imageUrl.isBlank()) {
  • code format 은 습관화하셔요~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵! 습관화하겠습니다!

@brainbackdoor brainbackdoor merged commit 3aeb285 into next-step:d-h-k May 29, 2023
@moto6
Copy link
Author

moto6 commented May 30, 2023

@brainbackdoor 넵 리뷰 감사합니다! 다음 스텝 에서 이번 코드리뷰사항까지 반영해서 PR 올리겠습니다!!

즐거운 한주 되시기 바라겠습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants