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 라이브러리 구현하기 - 4단계] 모디(전제희) 미션 제출합니다. #597

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

jaehee329
Copy link

안녕하세요 콩하나~

4단계 내용 구현하여 제출합니다!
저는 TxUserService의 InnerClass로 Transaction을 관리하도록 구현했어요!
말씀 주셨던 부분들도 반영해서 리팩토링했습니다

항상 꼼꼼한 리뷰 감사합니다!

Copy link

@kong-hana01 kong-hana01 left a comment

Choose a reason for hiding this comment

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

안녕하세요 모디! 콩하나입니다.
4단계 미션 구현하신 내용을 보니 정말 잘 구현하셨더라고요.
코드 자체가 너무 깔끔해서 제가 추가로 리뷰를 드릴 내용이 많이 없네요.
아주 사소하고 간단한 질문들만 몇 개 남겨드렸어요!!

궁금한 점이 있어서 일단 머지는 바로 하지 않겠습니다 ㅋㅋㅋㅋ
여유되실 때 답변 달아주시면 감사드리겠습니다 ㅎㅎ

고생하셨어요~

Comment on lines +10 to +32
private final UserDao userDao;
private final UserHistoryDao userHistoryDao;

public AppUserService(final UserDao userDao, final UserHistoryDao userHistoryDao) {
this.userDao = userDao;
this.userHistoryDao = userHistoryDao;
}

public User findById(final long id) {
return userDao.findById(id);
}

public void insert(final User user) {
userDao.insert(user);
}

public void changePassword(final long id, final String newPassword, final String createBy) {
User user = findById(id);
user.changePassword(newPassword);
userDao.update(user);
userHistoryDao.log(new UserHistory(user, createBy));
}
}

Choose a reason for hiding this comment

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

오태식이 돌아왔구나~
다시 돌아온 비즈니스 로직이 좋아요

Comment on lines +62 to +70
private static class TransactionExecutor implements AutoCloseable {

private final DataSource dataSource;
private final Connection connection;

private TransactionExecutor(DataSource dataSource, Connection connection) {
this.dataSource = dataSource;
this.connection = connection;
}

Choose a reason for hiding this comment

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

오... AutoCloseable에 이너클래스까지 ㄷㄷ
덕분에 코드가 깔끔하네요!

Comment on lines 101 to 104
public void close() {
DataSourceUtils.releaseConnection(connection, dataSource);
TransactionSynchronizationManager.unbindResource(dataSource);
}

Choose a reason for hiding this comment

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

모디는 커넥션을 닫아줄 때 autoCommit을 설정하지 않는 것을 선택하셨군요!
jpa에서는 기본적으로 connection을 닫을 때 setAutocommit(true)를 호출해서 저도 그렇게 했었는데,
재밌는 글이 있어서 공유해드립니다 ㅎㅎ

https://stackoverflow.com/questions/53922370/when-shall-use-setautocommittrue-in-java

Copy link
Author

Choose a reason for hiding this comment

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

JPA가 그런다니 신기하네요.. autoCommit 설정을 변경하는 것은 네트워크 비용은 전혀 들지 않나요?
콩하나는 JPA가 그렇게 하는 이유는 무엇이라고 생각하시는지 궁금하네요 ㅎㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

조오금 찾아보니 하이버네이트는 DBCP connection의 autoCommit 상태를 신뢰하지 않고 쿼리 실행 시 비용을 들여 getAutoCommit, setAutoCommit등을 실행시킨다는 말이 있네요. 끝날 때의 autoCommit 설정은 그럴 만한 가치가 있는 것일까요?

Choose a reason for hiding this comment

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

끝날 때의 autoCommit 설정은 그럴 만한 가치가 있는 것일까요?

딱히 그럴만한 가치는 없을 것 같아요~~
스택 오버 플로우도 그런 의견으로 수렴된 것 같더라고요 ㅎㅎ
아까 재미나게 이야기 나눴네요~~

Comment on lines 37 to 40
private void execute(Runnable runnable) {
try (TransactionExecutor executor = TransactionExecutor.initTransaction(datasource)) {
try {
runnable.run();

Choose a reason for hiding this comment

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

Runnable 인터페이스에 대한 공식문서에서는 다음과 같이 설명하고 있네요!

The Runnable interface should be implemented by any class whose instances are intended to be executed by a thread.

쓰레드를 구현할 때 해당 인터페이스를 활용하는데, 재활용한다는 측면에서는 좋지만
처음 보는 사람들은 새로운 쓰레드를 만들어서 처리하나?하는 의문이 들 수 있을 것 같아요.
connection을 활용할 때에는 동일한 쓰레드에서 동작해야하는데 말이죠!

그래서 파라미터와 리턴값이 같더라도 다른 걸로 함수형 인터페이스를 정의해주는 게 더 좋을 것 같다고 생각하는데,
모디는 어떻게 생각하시나요?

Choose a reason for hiding this comment

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

그래도 해당 메소드 내부에서 비즈니스 로직의 동작과 트랜잭션 동작이 명확하게 구분되어서 좋네요 👍

Copy link
Author

Choose a reason for hiding this comment

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

오.. 덕분에 Runnable 인터페이스의 javadoc을 자세히 읽어봤어요!
시그니처랑 별개로 인터페이스 자체가 새로운 스레드에 작업을 위임할 때의 콜백 함수를 정의하는 용도로 존재하는 것이었군요!

좋은 리뷰 감사합니다!!

Comment on lines +7 to +9
User findById(final long id);
void insert(final User user);
void changePassword(final long id, final String newPassword, final String createBy);

Choose a reason for hiding this comment

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

사소한 부분인데요.
final이 있는 게 있고, 없는 게 있네요.

여태 모디가 직접 작성한 코드에서는 final이 없었던 것으로 기억하는데,
final을 사용하시는 기준이 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 주어진 뼈대 코드의 final인데 뼈대 코드를 완전히 수정하지 않아서 남아있네요 ㅎㅎ..
뼈대 코드에 final이 항상 붙어있어 다 없애는 방향으로 통일해야 하나 항상 고민이 되었어요. 다 지우다가 일부 놓친 적도 있고..

불변성의 장점은 물론 알고 있어 예전에는 IDE의 도움으로 final을 붙여주었었는데,
쓰다 보니 final이 붙음으로써 메서드의 파라미터들이나 각종 변수들이 너무 길어지는 점,
때로는 까먹고 final을 붙이지 않기도 하고 이후에 코드를 읽을 때 의도적으로 뺐었는지 불필요하게 다시 고민하게 되는 점 등..

자바 외의 언어를 썼다면 당연히 따랐겠지만 자바에서는 언제나 final 키워드를 붙이면서 오는 잔실수나 가독성 저하가 불변 키워드에서 오는 장점보다 더 크게 다가오더라구요. 또 캡슐화를 통한 불변 객체 구현이 아닌 메서드 내부에서의 파라미터와 지역 변수들의 final 키워드가 과연 유용한게 맞냐는 생각도 많이 했고요...ㅎㅎㅎ

Choose a reason for hiding this comment

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

인정합니다 ㅎㅎ
저도 사실 final 키워드 신경쓰는게 번거롭더라고요...
그래서 기존 코드가 아예 텅텅 빈 상태다! 하면 final 키워드를 붙이지 않고 작업하고,
final이 포함되어있으면 이를 써는 정도로만 유지하고 있어요.
아무튼 모디 의견에 백배 공감합니다.

Copy link

@kong-hana01 kong-hana01 left a comment

Choose a reason for hiding this comment

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

모디 안녕하세요~~
jdbcTemplate 미션 잘 구현하셨군요 ㅎㅎ
리뷰 반영하신거 확인했습니다!

추가로 아까 만나서 이야기나눴던 부분 링크 남겨드릴게요.
ThreadLocal을 초기화할 때 static 블럭으로 초기화하면 안되더라고요~

#486 (comment)

고생하셨습니다.
레거시 리팩토링 미션도 화이팅!!!!!!!

@kong-hana01 kong-hana01 merged commit 707d5c9 into woowacourse:jaehee329 Oct 12, 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