[3단계 - Transaction 적용하기] 우가(민보욱) 미션 제출합니다. #1159
Conversation
WalkthroughDAO 메서드들(UserDao.update, UserHistoryDao.log 등)이 명시적 java.sql.Connection 파라미터를 받도록 시그니처가 변경되었습니다. UserService는 DataSource를 주입받아 changePassword에서 직접 Connection을 획득하고 auto-commit을 끈 뒤 사용자 업데이트와 이력 로그를 같은 트랜잭션 내에서 수행하며 성공 시 커밋, 실패 시 롤백합니다. JdbcTemplate에는 Connection을 인수로 받는 update 오버로드들이 추가되었습니다. 테스트 코드와 목(Mock) 클래스들도 이에 맞게 수정되었습니다. 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: 3
🧹 Nitpick comments (13)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (2)
18-26: Connection 수명 관리와 setUp 단순화 제안
- @beforeeach에서 Connection을 열고 닫지 않아 누수 위험이 있어요. @AfterEach에서 닫거나, 테스트 메서드 스코프로 한정하는 방식을 고려해보셨나요?
- setUp 안에 스키마 초기화·DAO 생성·데이터 삽입까지 모두 포함되어 10줄을 넘습니다. 역할을 분리(예: 스키마 초기화 메서드, 테스트 데이터 준비 메서드)하면 규칙 6(작은 메서드)과 SRP 충족에 도움이 됩니다.
- DatabasePopulatorUtils.execute가 스키마 초기화를 담당한다면 수동 DROP 호출은 중복일 수 있어요. 초기화 책임을 한 곳으로 일원화하는 편이 안전합니다.
코딩 가이드라인 기준.
78-78: DAO 호출 방식의 일관성update는 Connection을 주입받고, 다른 조회/삽입은 내부에서 커넥션을 획득합니다. 테스트/서비스 전반에서 “트랜잭션 경계 내 호출은 모두 Connection 기반”처럼 일관된 규칙을 정하면 이해·유지보수성이 좋아집니다. 지금 단계에서는 OK지만, 다음 단계에서 정책을 정리해보시는 건 어떨까요?
코딩 가이드라인 기준.
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (2)
16-18: 롤백 경로를 검증하기 위한 예외 처리 방향 좋아요테스트 더블이 의도적으로 예외를 던져 트랜잭션 롤백을 유도하는 전략은 명확합니다. 다만, 단언 시 메시지나 cause까지 확인하면(예: 원인 예외가 DataAccessException인지) 회귀 시그널이 더 선명해집니다. 필요하시면 보조 단언 전략 제안해드릴게요.
코딩 가이드라인 기준.
9-13: 상속 기반 테스트 더블의 결합도구현 클래스(UserHistoryDao)를 상속하면 테스트가 구현 변경에 취약해집니다. 인터페이스 도입 또는 합성(필요 의존만 주입)으로 결합도를 낮추는 방식을 검토해보셨나요?
코딩 가이드라인 기준.
app/src/main/java/com/techcourse/dao/UserDao.java (2)
27-31: 타입 명시와 원시값 포장
- password에 setObject를 사용하면 타입 의도가 흐려집니다. 드라이버 의존 없이 setString처럼 명시하는 편이 안전합니다.
- account/password/email은 도메인 개념이 강한 값이에요. 규칙 3(모든 원시값과 문자열 포장)에 따라 Value Object로 감싸면 검증·중복 제거·의미 전달이 좋아집니다. 어디에서 유효성(빈 문자열, 포맷 등)을 보장하실 계획인가요?
코딩 가이드라인 기준.
36-39: DAO 공개 API에 Connection 노출Connection이 퍼블릭 메서드 시그니처에 노출되면 인프라 세부사항이 도메인/응용 계층으로 번집니다. 지금 단계에서는 수작업 트랜잭션을 위해 불가피하지만, 이후 단계에서 트랜잭션 경계를 추상화(TransactionManager/Template 등)해 DAO를 인프라로 캡슐화하는 방향을 고려해보세요. 또한 where account = ? 업데이트는 account의 유니크 제약을 전제로 합니다. 스키마에서 UNIQUE가 보장되나요?
코딩 가이드라인 기준.
app/src/test/java/com/techcourse/service/UserServiceTest.java (2)
27-34: setUp 중복 제거와 작은 메서드 유지
- dataSource를 이미 필드로 보유하므로 DatabasePopulatorUtils.execute 호출도 동일 인스턴스를 재사용하면 의도 전달이 선명합니다(중복 호출/획득 지양).
- setUp가 10줄을 넘습니다. 테스트 픽스처 구성(DAO/Service 생성)과 데이터 초기화를 분리하면 규칙 6 충족과 가독성에 도움이 됩니다.
코딩 가이드라인 기준.
62-64: 예외 단언의 구체성현재 RuntimeException으로만 단언하고 있어 롤백 경로가 깨져도 통과할 여지가 있습니다. 메시지/원인 예외까지 함께 단언하면(예: cause가 DataAccessException) 실패 원인 파악이 쉬워집니다. 어떻게 검증하고 싶으신가요?
코딩 가이드라인 기준.
app/src/main/java/com/techcourse/service/UserService.java (4)
20-26: 서비스 계층의 인프라 결합도Service가 DataSource에 직접 의존하면 응용/도메인 로직이 인프라와 섞입니다. 이후 단계에서 트랜잭션 경계를 추상화(예: TransactionTemplate)해 서비스는 “무슨 일을 할지”만, 인프라 계층은 “어떻게 할지”만 책임지도록 분리하는 걸 권합니다.
코딩 가이드라인 기준.
37-59: 들여쓰기 1단계·작은 메서드 원칙 위반(트랜잭션 처리 분리 필요)중첩 try-catch로 들여쓰기 depth가 2단계이고, 메서드도 10줄을 넘습니다. 규칙 1·6 위반이에요. 트랜잭션 경계를 별도 메서드/컴포넌트로 추출하고, 비즈니스 로직(비밀번호 변경·이력 기록)을 작은 메서드로 분리하면 의도와 책임이 선명해집니다. 가드 클로즈/템플릿 메서드/함수형 콜백 등도 고려해보셨나요?
코딩 가이드라인 기준.
41-45: 트랜잭션 경계 내 일관된 커넥션 사용findById는 현재 JdbcTemplate이 내부에서 다른 커넥션을 열어 실행됩니다. 동일 트랜잭션 커넥션으로 조회까지 수행(또는 필요 시 SELECT … FOR UPDATE)하면 일관성과 동시성 안전성이 올라갑니다. 조회 API에도 Connection을 받을지, 트랜잭션 템플릿에서 커넥션을 전파할지 결정해보세요.
코딩 가이드라인 기준.
50-55: 로깅 레벨·메시지와 예외 계층
- “롤백 성공” 메시지에 스택트레이스를 함께 남기면 잡음이 될 수 있어요. 원인 예외는 error로, 롤백 성공은 요약 로그로 분리하는 편이 가독성에 좋습니다.
- 최종적으로 RuntimeException을 던지는데, 애플리케이션 계층 예외로 표준화하면 핸들링이 수월합니다. 메시지·원인 유지 전략도 함께 정리해보세요.
코딩 가이드라인 기준.
Also applies to: 57-59
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
81-98: 코드 중복이 발견되었습니다두
executeUpdate메서드의 예외 처리 로직이 동일합니다 (lines 85-88과 lines 94-97).🤔 생각해볼 질문들:
- SQLException을 DataAccessException으로 변환하는 로직을 별도로 분리하면 어떨까요?
- 만약 예외 처리 방식이나 로그 메시지가 변경되어야 한다면, 현재는 몇 군데를 수정해야 할까요?
- 중복된 예외 처리를 하나의 메서드로 추출한다면 어떤 이점이 있을까요?
💡 힌트:
DRY(Don't Repeat Yourself) 원칙을 고려해보세요. SQLException을 처리하는 공통 로직을 별도 메서드로 추출하거나, 함수형 인터페이스를 활용하여 중복을 제거할 수 있습니다.📖 이 원칙이 중요한 이유:
- 유지보수성: 한 곳만 수정하면 모든 곳에 적용됨
- 일관성: 동일한 상황에 대한 처리가 항상 같음
- 가독성: 핵심 로직이 더 명확하게 드러남
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/com/techcourse/dao/UserDao.java(2 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/dao/UserDaoTest.java(3 hunks)app/src/test/java/com/techcourse/service/MockUserHistoryDao.java(2 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(2 hunks)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:
app/src/main/java/com/techcourse/dao/UserHistoryDao.javaapp/src/main/java/com/techcourse/service/UserService.javaapp/src/test/java/com/techcourse/dao/UserDaoTest.javajdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.javaapp/src/main/java/com/techcourse/dao/UserDao.javaapp/src/test/java/com/techcourse/service/UserServiceTest.javaapp/src/test/java/com/techcourse/service/MockUserHistoryDao.java
🧬 Code graph analysis (4)
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/dao/UserDaoTest.java (1)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
app/src/test/java/com/techcourse/service/UserServiceTest.java (4)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
JdbcTemplate(15-155)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(11-65)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(10-33)
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (2)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(10-33)app/src/main/java/com/techcourse/domain/UserHistory.java (1)
UserHistory(5-59)
🔇 Additional comments (2)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
16-18: 생성자 구조가 깔끔합니다!JdbcTemplate을 주입받는 구조가 명확하고 의존성 관리가 잘 되어 있습니다.
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
35-41: 트랜잭션 지원을 위한 API 확장이 적절합니다!외부에서 Connection을 제공받는 오버로드 메서드들이 추가되어 트랜잭션 관리가 가능해졌습니다. 메서드 책임도 명확하게 분리되어 있습니다.
| PreparedStatementSetter pss = ps -> { | ||
| ps.setLong(1, userHistory.getUserId()); | ||
| ps.setString(2, userHistory.getAccount()); | ||
| ps.setString(3, userHistory.getPassword()); | ||
| ps.setString(4, userHistory.getEmail()); | ||
| ps.setObject(5, userHistory.getCreatedAt()); | ||
| ps.setString(6, userHistory.getCreateBy()); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
객체지향 생활 체조 원칙 9번 위반: Tell, Don't Ask
현재 코드는 UserHistory 객체에게 데이터를 요청(Ask)하여 직접 처리하고 있습니다. 이는 캡슐화를 약화시키고 UserHistory를 단순한 데이터 홀더로 만듭니다.
🤔 생각해볼 질문들:
UserHistory객체 스스로가 PreparedStatement에 자신의 데이터를 설정하도록 할 수는 없을까요?- 만약 UserHistory가 "PreparedStatement에 내 정보를 바인딩해줘"라고 요청받는다면 어떻게 설계할 수 있을까요?
- 데이터를 꺼내서(get) 사용하는 것과, 객체에게 행동을 요청하는(tell) 것의 차이는 무엇일까요?
💡 힌트:
"Tell, Don't Ask" 원칙에 따르면, 객체에게 데이터를 달라고 하지 말고, 객체에게 무언가를 하라고 말해야 합니다. UserHistory가 스스로 PreparedStatement 바인딩 책임을 가질 수 있는지 고민해보세요.
📖 이 원칙이 중요한 이유:
- 진정한 캡슐화: 객체가 자신의 데이터를 스스로 관리
- 객체의 자율성: 데이터와 행동이 함께 있어야 함
- 변경에 강한 코드: UserHistory의 내부 구조가 변경되어도 이를 사용하는 코드는 영향받지 않음
As per coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
25-25: SQL 문법 수정 확인이전 리뷰에서 지적되었던 H2 DROP TABLE 문법 문제가 올바르게 수정되었습니다.
"DROP TABLE IF EXISTS users"는 H2의 정확한 문법입니다. 👍app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
43-47: 하드코딩된 1L 제거로 테스트 안정성 강화여러 곳에서 사용자 ID를 1L로 가정합니다. 데이터 초기화나 실행 순서에 따라 깨질 수 있는 취약한 전제입니다. 계정으로 조회해 ID를 얻거나, 삽입 시 반환/조회한 ID를 사용하는 방식으로 바꿔볼까요?
Also applies to: 61-66
🧹 Nitpick comments (4)
app/src/test/java/com/techcourse/service/UserServiceTest.java (4)
26-36: 테스트 격리와 DataSource 사용 일관성 개선
- setUp에서 만든 dataSource를 두고, DB 초기화에 다시 DataSourceConfig.getInstance()를 호출하고 있습니다. 싱글턴을 재참조하면 테스트 간 상태가 은연중 공유될 수 있어 격리가 약해집니다.
- DatabasePopulatorUtils가 시퀀스/ID를 매번 초기화하는지도 확인이 필요합니다. 초기화가 충분하지 않다면 이후 테스트에서 ID 가정(예: 1L)이 깨질 수 있습니다.
동일 필드 dataSource로 초기화 유틸을 호출하고, 초기화가 ID까지 리셋하는지 확인해보는 건 어떨까요? 필요하다면 테스트마다 독립 DB(URL 분리)도 고려해보세요.
54-69: 롤백 테스트를 더 “의미 있게” 강화
- 현재는 예외 발생과 비밀번호 불변만 확인합니다. 트랜잭션 경계의 의도(업데이트/히스토리 모두 원자적)를 더 확실히 검증하려면, 히스토리가 기록되지 않았음을 함께 확인해보는 게 좋습니다. Mock/Spy로 log 호출 여부를 검증하거나, DB 스냅샷/카운트를 확인하는 방식 등 여러 선택지가 있습니다.
- 또한 예외 타입을 RuntimeException 대신 도메인 특화 예외(또는 DataAccessException 계열)로 좁히면 의도 전달과 테스트 표현력이 좋아집니다. 의도적으로 RuntimeException을 사용하셨다면 그 이유(계층 간 전파/랩핑 전략)를 주석이나 테스트 명세로 남겨볼까요?
이 두 가지를 반영하면 “롤백이 정말로 모든 부작용을 되돌렸다”는 신뢰도가 높아집니다.
40-50: 전반 평가: 트랜잭션 커밋/롤백 흐름 검증은 명료합니다given/when/then 구조로 의도가 잘 드러나고, Mock DAO로 실패 시나리오를 유도한 점이 좋습니다. 위 제안들(식별자 하드코딩 제거, 행위 기반 검증, 부작용 검증 강화)을 적용하면 테스트 품질이 한 단계 더 올라갈 것 같습니다.
Also applies to: 54-69
42-43: 이름 명확성: createBy → createdBy 검토변수명이 과거분사 형태(createdBy)보다 축약형(createBy)에 가까워 보입니다. 규칙 5(축약 금지) 관점에서, 도메인 전반 네이밍과 일관되게 맞춰보는 것을 권합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/test/java/com/techcourse/dao/UserDaoTest.java(3 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.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:
app/src/test/java/com/techcourse/dao/UserDaoTest.javaapp/src/test/java/com/techcourse/service/UserServiceTest.java
🧬 Code graph analysis (2)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
app/src/test/java/com/techcourse/service/UserServiceTest.java (4)
jdbc/src/main/java/com/interface21/jdbc/core/JdbcTemplate.java (1)
JdbcTemplate(15-155)app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(11-65)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(10-33)
🔇 Additional comments (2)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (2)
73-85: 테스트 설계에 대한 질문
update()테스트에서 직접 Connection을 DAO에 전달하고 있습니다. PR 설명에 따르면 트랜잭션은 Service 계층에서 관리되는 것으로 보이는데요.몇 가지 생각해볼 점들입니다:
- 이 테스트는 DAO의 단위 테스트인가요, 아니면 통합 테스트인가요?
- 실제 프로덕션 코드에서 DAO는 어떻게 호출되나요? (누가 Connection을 전달하나요?)
- DAO 테스트에서 Connection을 직접 관리해야 한다면, 이것이 DAO의 책임 범위를 벗어나는 것은 아닐까요?
- UserService의
changePassword같은 트랜잭션이 필요한 비즈니스 로직을 테스트하는 것과, DAO의 CRUD 기능을 테스트하는 것의 차이는 무엇일까요?💡 힌트: 테스트의 목적과 범위를 명확히 하면, 어떤 수준에서 테스트해야 할지 결정하는 데 도움이 됩니다.
18-18: 리뷰 의견이 @beforeeach의 동작 방식 이해 부족에 기인함검증 결과, 원래 리뷰 의견의 "Connection 공유로 인한 테스트 격리 문제"는 타당하지 않습니다.
@BeforeEach어노테이션은 각 테스트 메서드 실행 전마다 실행되므로, 모든 테스트는 독립적인 새로운 Connection 객체를 받습니다. 따라서:
- 테스트 간 Connection 상태 공유 없음
- 각 테스트마다 fresh database state 보장 (매번 DROP TABLE 실행)
- 테스트 격리 원칙 준수됨
다만, 리소스 정리라는 실제 문제가 있습니다. Connection이 절대
close()되지 않으므로@AfterEach메서드를 추가하여 정리해야 합니다.Likely an incorrect or invalid review comment.
|
|
||
| private UserDao userDao; | ||
| private JdbcTemplate jdbcTemplate; | ||
| private Connection connection; |
There was a problem hiding this comment.
Connection 리소스 누수 문제
setup() 메서드에서 dataSource.getConnection()으로 Connection을 획득하고 있지만, 이를 닫는 코드가 보이지 않습니다. Connection은 데이터베이스와의 연결을 나타내는 중요한 리소스로, 사용 후 반드시 해제해야 합니다.
몇 가지 질문을 드려보겠습니다:
- 테스트가 완료된 후 Connection을 어떻게 정리하고 계신가요?
- 여러 테스트가 순차적으로 실행될 때 어떤 일이 발생할까요?
- JDBC 리소스 관리에 대해 어떤 패턴들이 있는지 알아보셨나요?
💡 힌트: JUnit의 라이프사이클 애노테이션(@AfterEach 등)과 try-with-resources 구문을 고려해보세요.
Also applies to: 24-24
🤖 Prompt for AI Agents
In app/src/test/java/com/techcourse/dao/UserDaoTest.java around lines 18 and 24,
the test acquires a Connection via dataSource.getConnection() but never closes
it, causing resource leaks; add proper cleanup by either (a) wrapping per-test
connection usage in try-with-resources so the Connection is closed
automatically, or (b) add an @AfterEach teardown method that checks if
connection is non-null and not closed and then calls connection.close(); ensure
any exceptions are handled or propagated as test failures and reset the
connection field to null after close.
| assertThat(actual.getPassword()).isEqualTo(newPassword); | ||
| } |
There was a problem hiding this comment.
비밀번호 getter 의존은 보안·설계 상 취약합니다
- 테스트가 비밀번호를 직접 읽어 비교합니다. 비밀번호 노출은 캡슐화를 약화시키고, 실수로 로깅/출력을 유도할 수 있습니다.
- 객체지향 생활 체조 규칙 9(게터/세터 금지), 규칙 3(원시값 포장) 관점에서도 도메인 행위로 검증하는 편이 좋습니다.
행위 기반 검증(예: 비밀번호 일치 여부를 도메인/서비스가 판단)으로 전환하고, Password 같은 값 객체 도입을 고민해보셨나요?
Also applies to: 67-69
🤖 Prompt for AI Agents
In app/src/test/java/com/techcourse/service/UserServiceTest.java around lines
49-50 (and similarly lines 67-69), the test reads the user's password via a
getter which exposes sensitive state; change the assertions to behavior-based
verification instead: call the service/domain method that checks credentials
(e.g., authenticate(email, rawPassword) or user.matchesPassword(rawPassword)) or
introduce a Password value object with a match/verify method and assert that it
returns true for the expected password and false for old/invalid ones; remove
direct getPassword() usages and update tests accordingly to use the new
behavior-based API.
2 participants
안녕하세요 대니!
이번 미션에서는 3단계의 요구사항만 최소한으로 반영을 했어요.
4단계때 동기화 작업을 하면서 분리를 하는 것 같더라구요!@!
이번 미션도 잘 부탁드립니다!