-
Notifications
You must be signed in to change notification settings - Fork 300
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
[4단계 - JDBC 라이브러리 구현하기] 카피(김상혁) 미션 제출합니다. #885
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 카피!
데모데이 준비랑 인프라 미션으로 리뷰가 많이 늦어졌네요. 죄송합니다.
간단한 코멘트 몇 개 작성해보았습니다. 확인 부탁드려요!
public DefaultAdvisorAutoProxyCreator defaultAdvisorAutoProxyCreator() { | ||
return new DefaultAdvisorAutoProxyCreator(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 객체는 어떤 역할을 하는 객체일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AOP 프록시를 자동으로 생성하는 역할을 합니다!
@Transactional
같은 어노테이션은 프록시가 필요한데, 등록된 빈에 대해 프록시를 생성해줍니다.
다만 제가 적용한 것 처럼 수동으로 빈 등록을 안해도 자동으로 적용되다 하여 코드는 제거했습니다!
@Override | ||
public void changePassword(long id, String newPassword, String createBy) { | ||
Connection connection = DataSourceUtils.getConnection(DataSourceConfig.getInstance()); | ||
try { | ||
connection.setAutoCommit(false); | ||
|
||
userService.changePassword(id, newPassword, createBy); | ||
|
||
connection.commit(); | ||
} catch (Exception e) { | ||
handleException(connection); | ||
if (e instanceof SQLException) { | ||
throw new DataQueryException(e.getMessage(), e); | ||
} | ||
throw new DataAccessException(e.getMessage(), e); | ||
} | ||
} | ||
|
||
private void handleException(Connection connection) { | ||
if (connection != null) { | ||
handleRollBack(connection); | ||
} | ||
} | ||
|
||
private void handleRollBack(Connection connection) { | ||
try { | ||
connection.rollback(); | ||
} catch (SQLException rollbackEx) { | ||
throw new DataQueryException(rollbackEx.getMessage(), rollbackEx); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TxUserService로 잘 분리하셨네요 👍
Service에 트랜잭션 로직 전체가 들어있는건 조금 어색한 것 같아요.
이 부분을 다른 클래스로 분리해보는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
트랜잭션 관련 로직은 TransactionExcutor에서 수행하도록 변경했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 Runnable로 구현했는데 FunctionalInterface로도 할 수 있군요~ 👍
throw new DataQueryException(rollbackEx.getMessage(), rollbackEx); | ||
} | ||
} | ||
void changePassword(final long id, final String newPassword, final String createdBy); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserService 인터페이스에 save 메서드가 없는 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
save도 추가했습니다!
final UserService userService = (UserService) Proxy.newProxyInstance( | ||
appUserService.getClass().getClassLoader(), | ||
appUserService.getClass().getInterfaces(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 테스트가 추가되면 중복되는 코드가 많이 생길 것 같아요. setUp 안으로 넣는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주입받는 userHistoryDao가 서로 달라서 하나의 setUp으로 처리할 수 없었습니다.
그래서 해당 로직을 메서드 분리하는 정도로 개선했습니다!
if (targetMethod.isAnnotationPresent(Transactional.class)) { | ||
final var transactionStatus = transactionManager.getTransaction(new DefaultTransactionDefinition()); | ||
|
||
try { | ||
Object result = method.invoke(target, args); | ||
transactionManager.commit(transactionStatus); | ||
return result; | ||
} catch (Exception e) { | ||
transactionManager.rollback(transactionStatus); | ||
throw new DataAccessException(e); | ||
} | ||
} | ||
return method.invoke(target, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
써서 depth 1 줄이기 vs. !
안 쓰고 depth 1 늘리기
카피의 취향이 궁금합니다! ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
를 써서 depth를 1줄이는 방식으로 바꿨더니 이 방식이 더 가독성이 좋은 것 같네요!
반영했습니다~
import java.util.Map; | ||
|
||
public abstract class TransactionSynchronizationManager { | ||
|
||
private static final ThreadLocal<Map<DataSource, Connection>> resources = new ThreadLocal<>(); | ||
private static final ThreadLocal<Map<DataSource, Connection>> resources = ThreadLocal.withInitial(HashMap::new); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ThreadLocal은 무엇일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각 스레드마다 독립적인 값을 저장할 수 있게 하는 클래스입니다.
여러 스레드가 동시에 값을 접근해도 스레드가 각자의 값을 반환받을 수 있습니다.
여기서는 하나의 스레드가 하나의 커넥션을 사용하여 트랜잭션을 유지시키기 위해 ThreadLocal을 사용했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 카피!
피드백 사항 잘 반영해주셨네요.
이번 미션은 여기서 마치겠습니다.
남은 인프라 미션도 화이팅!!
안녕하세요 메이슨!
드디어 마지막 미션까지 왔네요!
끝까지 잘 부탁드립니다~~