[3단계 - Transaction 적용하기] 메이(김시원) 미션 제출합니다. #1131
Conversation
Walkthrough트랜잭션 기반 갱신을 도입했다. Jdbc에 Connection을 직접 주입하는 경로를 추가하며 다음을 변경했다: UserDao에 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 (6)
app/src/main/java/com/techcourse/dao/UserDao.java (1)
21-24: SQL 중복 구조 재검토 제안
update(Connection, User)와 기존update(User)가 동일한 SQL과 파라미터 구성을 각각 유지하고 있어 변화 시 두 곳을 모두 수정해야 하는 구조입니다. 규칙 6(모든 엔티티를 작게 유지) 관점에서도 책임이 흩어질 위험이 보이는데요. 혹시 SQL 생성과 파라미터 매핑을 한 곳으로 모아두면 변경에 더 안전해지지 않을까요?app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
64-90: 트랜잭션용 로그 메서드의 중복 최소화 고민새로 도입된
log(Connection, UserHistory)가 기존log(UserHistory)와 SQL 구성·파라미터 세팅 로직을 그대로 복제하고 있어 수정 포인트가 두 배로 늘어난 상태입니다. 이런 형태는 규칙 6과 응집도 측면에서 부담이 커질 수 있는데요. 공통 동작을 별도의 추출 지점으로 모아 두면 향후 변경이나 확장 시 안정성을 더 높일 수 있을 것 같습니다. 어떤 방식으로 분리하면 좋을지 한번 고민해보시면 어떨까요?jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
67-77: 코드 중복을 제거하는 방법을 고려해보셨나요?
execute(Connection, ...)메서드(67-77줄)와 기존execute(String, ...)메서드(55-65줄)를 비교해보면, PreparedStatement 생성 이후의 로직(파라미터 설정, 로깅, 실행, 예외 처리)이 동일합니다.Connection 관리 방식만 다른 상황에서 이러한 중복이 발생했을 때, 어떤 방법으로 공통 로직을 추출할 수 있을까요?
힌트: Connection을 얻는 부분과 PreparedStatement를 사용하는 부분을 분리하면 어떨까요?
jdbc/src/main/java/com/interface21/transaction/TransactionManager.java (3)
16-16: 불필요한 세미콜론이 있습니다.
getConnection()뒤에 세미콜론이 중복되어 있습니다. 코드 동작에는 영향을 주지 않지만 제거하는 것이 좋습니다.
22-24: 예외 처리의 구체성을 높여보면 어떨까요?22줄에서
Exception을 catch하고 있는데, 이는 너무 광범위합니다.질문:
- 어떤 예외들이 발생할 수 있을까요?
SQLException과 비즈니스 로직에서 발생하는 예외를 구분해서 처리해야 할까요?- 모든 예외를 동일한 메시지로 감싸는 것이 적절할까요?
힌트:
action.execute()에서 발생할 수 있는 예외와connection.rollback()에서 발생할 수 있는 예외를 생각해보세요.
27-28: 더 구체적인 예외 타입을 사용하면 어떨까요?Connection 획득 시 발생하는
SQLException을 잡아서RuntimeException으로 감싸고 있습니다.이미
DataAccessException(24줄)을 사용하고 계신데, Connection 관련 오류도 동일한 예외 타입으로 처리하는 것이 일관성 측면에서 어떨까요? 또는 별도의 구체적인 예외 타입이 필요할까요?
📜 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(2 hunks)app/src/main/java/com/techcourse/dao/UserHistoryDao.java(2 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(2 hunks)jdbc/src/main/java/com/interface21/transaction/TransactionManager.java(1 hunks)jdbc/src/main/java/com/interface21/transaction/TransactionalAction.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/test/java/com/techcourse/service/MockUserHistoryDao.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javajdbc/src/main/java/com/interface21/transaction/TransactionalAction.javaapp/src/main/java/com/techcourse/dao/UserDao.javajdbc/src/main/java/com/interface21/transaction/TransactionManager.javaapp/src/main/java/com/techcourse/dao/UserHistoryDao.javaapp/src/main/java/com/techcourse/service/UserService.javaapp/src/test/java/com/techcourse/service/UserServiceTest.java
🧬 Code graph analysis (4)
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (2)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(12-92)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/main/java/com/techcourse/service/UserService.java (4)
jdbc/src/main/java/com/interface21/transaction/TransactionManager.java (1)
TransactionManager(7-31)app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(8-45)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(12-92)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(14-84)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
🔇 Additional comments (1)
app/src/main/java/com/techcourse/service/UserService.java (1)
34-37: 트랜잭션 경계 설정이 잘 되었습니다!
userDao.update와userHistoryDao.log를 하나의 트랜잭션으로 묶어 일관성을 보장하고 있습니다. 두 작업이 모두 성공하거나 모두 실패하도록 처리된 점이 좋습니다.
| public UserService(final UserDao userDao, final UserHistoryDao userHistoryDao, final DataSource dataSource) { | ||
| this.userDao = userDao; | ||
| this.userHistoryDao = userHistoryDao; | ||
| this.transactionManager = new TransactionManager(dataSource); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
의존성 주입 패턴에 대해 다시 생각해볼까요?
현재 UserService는 TransactionManager를 직접 생성하고 있습니다(19줄). 하지만 UserDao와 UserHistoryDao는 생성자를 통해 주입받고 있습니다.
이런 차이가 발생한 이유는 무엇일까요? 만약 TransactionManager도 외부에서 주입받는다면 어떤 장점이 있을까요?
힌트: 테스트 작성 시나 다른 트랜잭션 구현체로 교체해야 할 때를 생각해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/service/UserService.java around lines 16 to
20, UserService currently constructs a new
TransactionManager(this.transactionManager = new TransactionManager(dataSource))
instead of receiving it like the DAOs; change the constructor to accept a
TransactionManager parameter (inject it) and assign that field, remove direct
new TransactionManager(dataSource) construction, and update all call sites/DI
configuration and tests to provide a TransactionManager (or a mock) so swapping
implementations and unit testing becomes possible.
| public void executeTransaction(final TransactionalAction action) { | ||
| try (final var connection = dataSource.getConnection();) { | ||
| connection.setAutoCommit(false); | ||
|
|
||
| try { | ||
| action.execute(connection); | ||
| connection.commit(); | ||
| } catch (Exception e) { | ||
| connection.rollback(); | ||
| throw new DataAccessException("트랜잭션 내부 작업 수행 중 오류가 발생하여 롤백이 수행되었습니다."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
메서드의 들여쓰기 깊이를 줄여볼 수 있을까요?
현재 executeTransaction 메서드는 try-with-resources(16줄) 안에 또 다른 try-catch 블록(19-25줄)이 중첩되어 있어 들여쓰기 깊이가 2입니다.
객체지향 생활 체조 규칙 1번은 "한 메서드에 오직 한 단계의 들여쓰기만"을 권장합니다. 이는 메서드의 복잡도를 낮추고 각 메서드가 하나의 책임만 갖도록 하기 위함입니다.
중첩된 트랜잭션 로직을 별도 메서드로 분리하면 어떨까요?
힌트: Connection을 받아서 트랜잭션을 실행하는 부분을 별도 메서드로 추출해보세요.
🤖 Prompt for AI Agents
jdbc/src/main/java/com/interface21/transaction/TransactionManager.java around
lines 15-25: the executeTransaction method currently nests a try-with-resources
and an inner try-catch, increasing indentation depth; refactor by extracting the
inner transactional logic into a new private helper method that accepts a
Connection (e.g., private void executeWithinTransaction(Connection connection,
TransactionalAction action)), move the action.execute/commit/rollback and
exception-to-DataAccessException mapping into that helper, and have
executeTransaction simply obtain the connection in a try-with-resources and call
the new helper; ensure the helper performs rollback on exception and rethrows
the DataAccessException (preferably wrapping the original exception).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (2)
67-77:execute(Connection, …)리소스 처리 OK. 로그/예외 메시지 개선을 제안드립니다.
- “query :” 라벨은 update에도 쓰여 혼동될 수 있어요. 중립적인 “sql :” 등으로 통일하면 가독성이 좋아집니다.
- 장애 분석 시, error 로그/예외 메시지에 SQL(또는 식별자)과 파라미터 “개수/타입” 정도를 포함하면 진단성이 크게 올라갑니다. 값 자체는 PII 노출 우려로 지양.
- 디버깅 품질을 위해 SQL 내 ‘?’ 개수와 전달된 파라미터 개수 불일치를 사전 검증하는 방어 로직을 작은 헬퍼로 분리해보는 건 어떨까요?
executor.execute에서 발생하는 비-SQLException 런타임 예외를 어떻게 래핑/전파할지 정책을 정해 일관화하면 호출 측 예외 처리 흐름이 명확해집니다.
80-82:parameters == null즉시 예외는 호환성 리스크가 있어요. 관대한 처리 또는 사전 검증을 고려해보세요.
- varargs를 생략하면 빈 배열이라 문제없지만, 호출부가
(Object[]) null을 명시적으로 넘겨왔던 관행이 있었다면 런타임에서 바로 깨질 수 있습니다. 또한 바인딩할 플레이스홀더가 없는 SQL에서는null을 “파라미터 없음”으로 간주해도 자연스러워요.- 선택지
- null을 빈 파라미터로 해석(관대한 API)하거나,
- SQL의 ‘?’ 개수와 파라미터 개수 불일치를 검출하는 쪽으로 검증의 초점을 옮기는 방법을 검토해보세요.
- 기존 코드베이스에
(… , null)호출이 있는지 한 번 확인 부탁드립니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java(2 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:
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java
🔇 Additional comments (1)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
28-30: 트랜잭션 경계 내 단일 커넥션 사용을 위한update(Connection, …)추가, 방향 좋습니다.
- 호출자(서비스/트랜잭션 매니저)가 커밋/롤백과 커넥션 생명주기를 책임진다는 계약을 Javadoc/주석으로 명확히 하면 오용을 줄일 수 있어요. autoCommit 전제도 함께 명시해보실래요?
- 같은 커넥션에서 “읽기-자신(read-your-writes)”이 필요하다면
query(Connection, …)계열도 일관성 차원에서 고려할 여지가 있어요. SELECT … FOR UPDATE 시나리오도요.- 이 오버로드를 DAO 내부에서만 쓰도록 경계를 정할지(응집도), 서비스에서 직접 쓰지 않도록 가이드를 둘지(결합도)도 점검해보세요.
1 participant
안녕하세요 띠용~
트랜잭션 관련 코드를 애플리케이션단에서 분리하고 싶었는데, step4까지 다루는 내용인 것 같아서 보류했어요!
이번에도 리뷰 잘 부탁드립니다 🙇♀️