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단계] 마코(이규성) 미션 제출합니다. #310

Merged
merged 3 commits into from
Oct 1, 2023

Conversation

aak2075
Copy link

@aak2075 aak2075 commented Sep 28, 2023

안녕하세요 스플릿! 마코입니다~😄

스플릿을 마지막으로 모든 온보딩 조원과 페어, 미션을 진행했네요ㅎㅎㅎ

일단은 요구 사항만 충족시키도록 구현했습니다.

무언가 더 해보고 싶지만 어렵기도하고 2단계가 리팩터링이라 다음 단계로 미루겠습니다.

즐거운 추석 보내세요~~🌕

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

@splitCoding splitCoding left a comment

Choose a reason for hiding this comment

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

마코!!😁

추석 잘 보내고 계신가요ㅎㅎ🙇

캠퍼스 갈린후로 거의 못봤는데 리뷰로 만나네요ㅎㅎ👍

개인적으로 고민을 많이 한 미션인데 비슷한 고민을 한거 같은 부분도 있고 훨씬 간단하게 풀어낸 부분도 있어서 저한테도 도움이 많이 되는 리뷰였어요!!💪

1단계 미션 요구사항은 충분히 반영되어 있다고 생각합니다!!

의견을 묻는 코멘트 2개정도 있는데 이부분에 대해서 답변해주시고 리뷰 요청주시면 바로 머지하도록 하겠습니다!!

수고많았어요 🎁

Comment on lines +35 to +36
try (Connection conn = dataSource.getConnection();
PreparedStatement pstmt = conn.prepareStatement(sql)) {

Choose a reason for hiding this comment

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

try-with-resources 👍
finally 코드들 없애보니 가독성이 진짜 많이 올라가네요오!!

@Override
public void setValues(PreparedStatement pstmt) throws SQLException {
for (int i = 0; i < args.length; i++) {
pstmt.setObject(i + 1, args[i]);

Choose a reason for hiding this comment

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

setObject 👍

import java.sql.PreparedStatement;
import java.sql.SQLException;

public class ArgumentPreparedStatementSetter implements PreparedStatementSetter{

Choose a reason for hiding this comment

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

책임 분리 좋네요!!👍

PreparedStatement 의 코드에 설명에서 Parameter 라는 단어로 해당 값들을 표현하더라구요 Parameter 를 이용한 네이밍을 사용해보면 더 좋을 것 같아요~😁

사소하지만 제가 이해한 PreparedStatement의 Argument를 setting을 하는 책임을 가진 객체가 맞다면 PreparedStatmentArgumentSetter 라는 네이밍이 저는 좀 더 와닿는데 클래스명만으로 PreparedStatementSetter 인터페이스를 구현한 클래스임을 충분히 알수있는 현재 네이밍에도 장점이 있어서 마코의 생각이 궁금합니다!🫡

Copy link
Author

Choose a reason for hiding this comment

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

저는 argument 기반의 PreparedStatementSetter라고 생각을 해서 ArgumentPreparedStatementSetter라고 지었습니다.
확실히 스플릿이 말씀해주신 PreparedStatementArgumentSetter가 의미 전달이 잘 되는 것 같습니다👍
하지만 나중에 PreparedStatementSetter 뿐만 아니라 PreparedStatementCreator와 같이 비슷한 이름의 여러 인터페이스가 생기는 경우 헷갈릴 것 같아 가능하면 인터페이스의 이름을 사용하는 것이 좋다고 생각합니다~

import java.sql.SQLException;

@FunctionalInterface
public interface PreparedStatementSetter {

Choose a reason for hiding this comment

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

확장성을 위한 추상화 👍

Comment on lines 44 to 47
int rowNum = 0;
while (resultSet.next()) {
results.add(rowMapper.mapRow(resultSet, rowNum++));
}

Choose a reason for hiding this comment

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

마코가 현 1단계 미션에서 rowNum 이 포함된 코드를 작성해야겠다고 결정한 이유가 궁금해요😊

Copy link
Author

Choose a reason for hiding this comment

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

페이징 쿼리와 같은 확장성을 생각했었는데 사실 지금 생각해보니 구현도 안됐고, 할지 안할지도 모르는 상태에서 짧은 생각으로 넣었던 것 같네요! 복잡도만 높이는 요인인 것 같아 일단 제거하고 나중에 필요할 때 추가하겠습니다~

@splitCoding splitCoding merged commit d518d4f into woowacourse:aak2075 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.

2 participants