[3단계 - Transaction 적용하기] 벨로(방예혁) 미션 제출합니다. #1137
Conversation
- UserService에 수동 트랜잭션 로직 적용 - 트랜잭션 전파를 위해 JdbcTemplate 및 DAO에 Connection을 받는 메서드 추가 - UserHistoryDao를 JdbcTemplate 기반으로 리팩토링하여 일관성 개선
Walkthrough
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
study/src/test/java/connectionpool/stage0/Stage0Test.java (1)
44-47: 용어/문구 정정 좋습니다설명 가독성이 좋아졌습니다. 추가로 “왜 DriverManager 대신 DataSource인지”를 한 문장으로 요약해 머리말에 배치하면 학습자 이해가 더 좋아질 것 같아요.
app/src/test/java/com/techcourse/service/UserServiceTest.java (4)
21-29: 테스트 데이터 초기화의 일관성 검증 필요매 테스트마다 DatabasePopulatorUtils.execute 이후에 동일 계정을 insert 하고 있습니다. 스키마 초기화가 아이디 시퀀스/데이터까지 항상 깨끗이 리셋되는지 확인해보셨나요? 만약 리셋이 보장되지 않으면 중복 계정, 예측 불가능한 id가 생길 수 있어요.
- 힌트: 스키마 초기화가 완전 초기화인지 확인하거나, 각 테스트가 고유 데이터를 사용하도록 분리해보세요.
As per coding guidelines
34-35: 동일 DataSource 사용 보장 관점 점검Service에 주입하는 DataSource와 JdbcTemplate에 주입된 DataSource가 동일 인스턴스여야 트랜잭션 경계가 일치합니다. 현재는 모두 DataSourceConfig.getInstance()를 사용하지만, 혹시라도 다른 구성으로 변경되면 어떤 문제가 생길지 고려해보셨나요?
- 힌트: “동일 DataSource 주입”을 명시적으로 검증하는 테스트/어설션을 추가해보면 어떨까요?
As per coding guidelines
Also applies to: 49-50
40-43: 상수 ID 의존도 낮추기findById(1L)에 의존하면 테스트 취약성이 커집니다. 초기화/삽입 순서가 바뀌면 깨질 수 있어요.
- 힌트: 삽입한 계정으로 조회(findByAccount) 후 id를 사용하거나, 삽입 시 반환된 식별자를 사용해보면 어떨까요?
As per coding guidelines
Also applies to: 57-60
46-56: 롤백 검증 범위 확장 제안비밀번호 롤백은 잘 검증하셨습니다. 추가로 user_history 테이블에 기록이 남지 않았는지도 확인해보면 트랜잭션 회귀를 더 잘 막을 수 있습니다.
- 힌트: 변경 이력 건수/최신 레코드의 존재 여부를 점검해보세요.
As per coding guidelines
jdbc/src/test/java/com/interface21/jdbc/core/JdbcTemplateTest.java (1)
231-243: Fail-fast 검증 테스트 추가, 매우 좋습니다두 케이스(인자 부족/과다)로 명확히 커버되었습니다. 몇 가지 보완점 제안드려요:
- 에러 메시지는 exact match 대신 contains를 사용하면 리팩터 시 덜 깨집니다.
- 새로 추가된 update(Connection, ...) 오버로드 경로도 동일 검증이 동작하는지 별도 테스트를 추가해보면 어떨까요?
As per coding guidelines
Also applies to: 245-258
app/src/main/java/com/techcourse/dao/UserDao.java (2)
39-50: update 중복 구현 제거동일 SQL/로깅이 두 메서드에 중복됩니다. 중복은 유지보수 비용과 버그 가능성을 키웁니다.
- 힌트: SQL을 상수로 추출하거나, 하나의 경로로 위임하는 형태로 DRY를 지켜보세요.
As per coding guidelines
53-64: 컬럼 인덱스 기반 매핑 일관성/안정성findAll/findById는 인덱스(1,2,3,4)로 매핑하고, findByAccount는 컬럼명으로 매핑합니다. 인덱스는 SELECT 컬럼 순서 변경에 취약합니다.
- 왜 중요할까요? 스키마/쿼리 리팩터 시 런타임 오류 없이 잘못된 데이터가 매핑될 수 있습니다.
- 힌트: 컬럼명을 사용하거나 RowMapper를 재사용 가능한 형태로 추출해 일관성을 유지해보세요.
As per coding guidelines
Also applies to: 66-79
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (1)
16-21: 테스트 더블의 실패 지점 강화 제안connection 기반 log만 오버라이드했습니다. 만약 서비스 구현이 실수로 비-connection 메서드를 호출하게 바뀌면 롤백 테스트가 “통과”할 수 있어요.
- 힌트: 비-connection log도 동일하게 실패하도록 만들어 회귀를 더 빨리 잡아내는 것을 고려해보세요.
As per coding guidelines
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
39-50: Connection 전달형 update 추가 적절트랜잭션 경계 외부 제어를 위한 오버로드 설계가 명확합니다. 몇 가지 점검 포인트:
- 이 메서드는 커넥션 수명/트랜잭션을 호출자가 책임집니다. 이를 Javadoc(또는 클래스 레벨 문서)로 명시해두면 오용을 줄일 수 있어요.
- 커넥션을 닫지 않는 설계가 맞는지(서비스 계층에서 try-with-resources/rollback 관리) 테스트로도 보호해보면 좋습니다.
As per coding guidelines
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
19-52: 중복 코드가 발견됩니다.두 개의
log메서드가 동일한 SQL 쿼리와 파라미터 매핑 로직을 포함하고 있습니다. 이는 DRY(Don't Repeat Yourself) 원칙을 위반하며, 향후 SQL 변경 시 두 곳을 모두 수정해야 하는 유지보수 부담이 발생합니다.다음을 고려해보세요:
- 한 메서드가 다른 메서드를 호출하도록 구조화할 수 있을까요?
- 또는 공통 로직을 추출하여 재사용할 수 있는 방법은 없을까요?
예를 들어, Connection이 없는 메서드에서 Connection을 획득한 후 Connection을 받는 메서드를 호출하는 방식도 가능합니다. 어떤 방식이 더 나은 설계일지 생각해보세요.
app/src/main/java/com/techcourse/service/UserService.java (2)
43-55: 중첩된 try-catch 구조를 개선할 수 있을지 고려해보세요.현재 구조는 다음과 같습니다:
- 외부 try-with-resources: Connection 관리
- 내부 try-catch: 트랜잭션 로직과 롤백 처리
이 구조가 복잡도를 높이고 있는데, 다음 질문들을 생각해보세요:
connection.rollback()도 SQLException을 던질 수 있는데, 이 경우는 어떻게 처리되나요?- 내부 Exception과 외부 SQLException을 구분하여 처리하는 것이 꼭 필요한가요?
- 두 예외를 통합하여 처리하면 코드가 더 간결해질 수 있지 않을까요?
힌트: 트랜잭션 관련 모든 SQLException을 하나의 try-catch에서 처리하고, 비즈니스 예외는 별도로 전파하는 방식도 고려해볼 수 있습니다.
49-51: 일반적인 Exception을 catch하고 있습니다.Line 49에서
catch (final Exception e)를 사용하고 있는데, 이는 다음을 의미합니다:
- RuntimeException을 포함한 모든 예외를 잡습니다
- 예상하지 못한 예외까지 모두 잡아서 롤백합니다
질문:
- 정말 모든 예외에 대해 롤백이 필요한가요?
- 특정 예외만 잡아서 처리하는 것이 더 명확하지 않을까요?
- DataAccessException과 같은 구체적인 예외 타입을 catch하면 어떨까요?
참고: 일반적으로 체크 예외(checked exception)나 특정 도메인 예외만 catch하여 처리하고, 예상치 못한 RuntimeException은 상위로 전파시키는 것이 더 명확한 설계일 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/com/techcourse/dao/UserDao.java(4 hunks)app/src/main/java/com/techcourse/dao/UserHistoryDao.java(1 hunks)app/src/main/java/com/techcourse/service/UserService.java(2 hunks)app/src/test/java/com/techcourse/service/MockUserHistoryDao.java(2 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(3 hunks)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(3 hunks)jdbc/src/test/java/com/interface21/jdbc/core/JdbcTemplateTest.java(1 hunks)study/src/test/java/connectionpool/stage0/Stage0Test.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 🎯 코드 품질 중심 리뷰 가이드라인이 리뷰는 코드 품질과 객체지향 원칙에 집중합니다.
미션 달성 여부가 아닌, 코드 설계와 품질 개선에 대한 피드백을 한글로 제공해주세요.📚 학습 원칙
- 직접 코드를 제공하지 마세요 (학습자가 명시적으로 요청하는 경우 제외)
- 문제 해결 과정을 안내하되, 정답을 바로 알려주지 마세요
- 작은 단계로 문제를 분해하여 접근하도록 도와주세요
💡 피드백 방법
- 유도 질문 활용: "만약 ~라면 어떻게 될까요?", "~를 고려해보셨나요?"
- 힌트 제공: 방향은 제시하되, 구체적인 구현은 학습자가 하도록
- 다양한 접근법 제시: 한 가지 해결책이 아닌 여러 가능성을 제안
- 왜?에 집중: 단순히 무엇이 잘못되었는지보다 왜 그런지 이해하도록
⚡ 객체지향 생활 체조 원칙 검토
다음은 객체지향 생활 체조(Object Calisthenics) 9가지 원칙입니다.
위반 시 학습 효과를 위해 반드시 피드백을 제공하되, 왜 이 원칙이 중요한지 설명해주세요:규칙 1: 한 메서드에 오직 한 단계의 들여쓰기만
- 들여쓰기 depth 최대 1 (중첩 제어구조 금지)
- 📖 이유: 메서드 복잡도 감소, 단일 책임 원칙 강화
- 💡 힌트: "이 부분을 별도 메서드로 추출하면 어떨까요?"
규칙 2: else 예약어 금지
- else, switch/case 사용 금지
- 📖 이유: 복잡한 분기 제거, 명확한 코드 흐름
- 💡 힌트: "early return이나 가드 클로즈 패턴을 고려해보세요"
규칙 3: 모든 원시값과 문자열 포장
- 원시 타입과 String을 객체로 포장
- 📖 이유: 도메인 개념 명확화, 비즈니스 로직 응집
- 💡 예시:
int port→Port port,String name→Name name규칙 4: 한 줄에 점 하나만 (디미터 법칙)
- 메서드 체이닝 제한
- 📖 이유: 결합도 감소, 캡슐화 향상
- 💡 나쁜 예:
request.getUri().getPath().substring()- 💡 좋은 예:
request.extractPath()규칙 5: 축약 금지
- 명확한 이름 사용 (축약어 금지)
- 📖 이유: 코드 가독성, 의도 명확화
- 💡 예시:
req→request,calcAmt→calculateAmount규칙 6: 모든 엔티티를 작게 유지
- 클래스 50줄, 메서드 10줄 이하
- 📖 이유: 단일 책임, 이해와 테스트 용이성
- 💡 힌트: "이 클래스가 너무 많은 일을 하고 있지 않나요?"
규칙 7: 인스턴스 변수 3개 이하
- 클래스당 최대 3개의 인스턴스 변수
- 📖 이유: 높은 응집도, 단일 책임 유지
- 💡 힌트: "관련 필드들을 별도 객체로 묶을 수 있을까요?"
규칙 8: 일급 컬렉션 사용
- 컬렉션을 감싸는 클래스 사용
- 📖 이유: 컬렉션 로직 캡슐화, 불변성 보장
- 💡 예시:
List<HttpHeader>→HttpHeaders클래스규칙 9: 게터/세터/프로퍼티 금지
- Tell, Don't Ask 원칙 준수
- 📖 이유: 진정한 캡슐화, 객체 자율성
- 💡 나쁜 예:
if (user.getPassword().equals(input))- 💡 좋은 예:
if (user.authenticate(input))🎯 추가 프로그래밍 요구사항
코드 스타일
- Java Style Guide 준수
- 3항 연산자 사용 금지
테스트 및 구조
- **모든 기능 T...
Files:
app/src/main/java/com/techcourse/dao/UserDao.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/main/java/com/techcourse/service/UserService.javaapp/src/main/java/com/techcourse/dao/UserHistoryDao.javaapp/src/test/java/com/techcourse/service/MockUserHistoryDao.javastudy/src/test/java/connectionpool/stage0/Stage0Test.javaapp/src/test/java/com/techcourse/service/UserServiceTest.javajdbc/src/test/java/com/interface21/jdbc/core/JdbcTemplateTest.java
🧬 Code graph analysis (4)
app/src/main/java/com/techcourse/service/UserService.java (3)
app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(10-95)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(9-53)app/src/main/java/com/techcourse/domain/UserHistory.java (1)
UserHistory(5-59)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
app/src/main/java/com/techcourse/domain/UserHistory.java (1)
UserHistory(5-59)
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (2)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(9-53)app/src/main/java/com/techcourse/domain/UserHistory.java (1)
UserHistory(5-59)
app/src/test/java/com/techcourse/service/UserServiceTest.java (3)
jdbc/src/main/java/com/interface21/dao/DataAccessException.java (1)
DataAccessException(3-26)jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
JdbcTemplate(15-140)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
🔇 Additional comments (2)
app/src/main/java/com/techcourse/service/UserService.java (2)
12-12: 클래스를 final로 선언한 것은 좋은 선택입니다.불필요한 상속을 방지하고 클래스의 의도를 명확하게 표현하고 있습니다.
46-47: DAO 메서드 호출 순서를 한번 점검해보세요.현재 구조는:
userDao.update()- users 테이블 업데이트userHistoryDao.log()- user_history 테이블 삽입만약
userDao.update()는 성공했지만userHistoryDao.log()가 실패한다면, 롤백으로 인해 사용자 비밀번호가 변경되지 않습니다.다음을 고민해보세요:
- 이력 로그 실패가 비밀번호 변경 자체를 막아야 하는 요구사항인가요?
- 아니면 이력 기록은 부가적인 작업으로, 실패해도 비밀번호 변경은 성공해야 하나요?
- 현재 설계가 비즈니스 요구사항을 정확히 반영하고 있나요?
트랜잭션의 범위와 롤백 정책은 비즈니스 요구사항에 따라 달라집니다. 요구사항을 다시 한번 확인해보시기 바랍니다.
3 participants
안녕하세요 새로이 😊
요구사항을 충족하면서 기존 구조를 최대한 유지하는 방향으로 구현했습니다.
변경 내역
항상 꼼꼼한 리뷰 감사해요 👍
변경 사항