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

[JDBC 라이브러리 구현하기 - 1단계] 이오(이지우) 미션 제출합니다. #300

Merged
merged 9 commits into from
Oct 1, 2023

Conversation

LJW25
Copy link

@LJW25 LJW25 commented Sep 28, 2023

안녕하세요 말랑!
리뷰 잘부탁드립니다 😊
이번 단계에서는 구현한 코드가 많지 않아서 코멘트 남길 부분은 별로 없네요.
즐거운 한가위 되세요! 😃

@LJW25 LJW25 self-assigned this Sep 28, 2023
Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

안녕하세요 이오~
명절 잘 보내고 계신가요??
정말 빠르게 구현해 주셨는데 제가 리뷰가 너무 늦었네요 ㅠㅠ
코드 너무 잘 짜주셔서 많이 피드백 드릴게 없었습니다!
몇가지만 남겼으니 천천히 확인해주시면 감사하겠습니다 :)

public UserDao(final DataSource dataSource) {
this.dataSource = dataSource;
}
private final RowMapper<User> rowMapper = (rs, rowNum) ->

Choose a reason for hiding this comment

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

static으로 정의하지 않으신 이유가 있으실까요?

Copy link
Author

Choose a reason for hiding this comment

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

staic이어야 한다는 생각을 미처 못했는데, 필드가 변하지 않으니 static인 편이 더 적절하겠네요! 반영하였습니다.
static을 반영하면서 static final이 되었는데, 말랑은 RowMapper도 상수로 취급하는게 옳다고 생각하시는지 궁금합니다!
(저는 우선 상수라고 생각하여 네이밍을 uppercase로 변경하였습니다.)

Choose a reason for hiding this comment

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

저는 음.. 변하지는 않으니 상수인 것 같다고는 생각하지만, 일반적으로 primitive나 String이 아니고서야 다 소문자로 쓰는 것 같네요 ..ㅎㅎ
그냥 이건 정해진 방법이 없는 것 같아서 제가 보기 편한대로 사용한답니다..

study/src/main/resources/application.yml Show resolved Hide resolved
@LJW25
Copy link
Author

LJW25 commented Oct 1, 2023

안녕하세요 말랑!
리뷰 감사드립니다 :)
너무 기본적인걸 놓친 부분이 있어 부끄럽네요. 😂
벌써 반이 지나갔지만 즐거운 연휴 되세요~~

Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

1단계 미션 고생하셨습니다 이오~
코드 너무 잘 작성해 주셔서 많이 수정할 부분이 없었네요!
이만 merge하겠습니다!
2단계도 잘 부탁드려요! :)

Comment on lines 17 to 18
);
private final JdbcTemplate jdbcTemplate;

Choose a reason for hiding this comment

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

static이랑 공백 한 칸 부탁드려요!

@shin-mallang shin-mallang merged commit b76329c into woowacourse:ljw25 Oct 1, 2023
1 check failed
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.

3 participants