#18 송금 요청 기능 #25
#18 송금 요청 기능 #25
Conversation
- MyBatis 를 통해 insert 쿼리 시 반환 값이 저장된 행의 수임을 확인 - 할당된 ID를 반환하기 위해 로직 변경
- 레코드를 저장, 업데이트하는 MyBatis 로직 추가
| if (amount <= 0) { | ||
| throw new InvalidWithdrawalException(amount); | ||
| } | ||
|
|
||
| if (this.balance < amount) { | ||
| throw new InvalidWithdrawalException(); | ||
| } | ||
|
|
||
| if (this.balance < this.pendingAmount + amount) { | ||
| throw new InvalidWithdrawalException(); | ||
| } |
There was a problem hiding this comment.
amount <= 0 / this.balance < amount / this.balance < this.pendingAmount + amount은 어떤 조건을 검증할것인지에 대한 명시성이 부족합니다. 단순히 어떤 값이 어떤 형태여야한다는 조건이 아닌, validation이라는 비즈니스 검증 로직이 들어갔으므로 어떤 형태의 검증 요구사항을 수행하는 것인지 명확하게 표현해야 검증에 대한 코드를 읽기 수월해집니다.
예를 들어, 이런식으로 표현할 수 있습니다.
if (!isAmountPositive()) {
throw new InvalidWithdrawalException(amount);
}
if (!isEnsureBalanceAvailable()) {
throw new InvalidWithdrawalException();
}
There was a problem hiding this comment.
출금 검증 로직을 메서드로 추출해서 의미를 명확하게 표현하도록 변경했습니다!
| * @return 생성된 계좌의 ID | ||
| */ | ||
| Long create(Account account); | ||
| Long create(final Account account); |
| @Transactional(readOnly = true) | ||
| public Account findByIdForUpdate(final Long memberId) { | ||
| return accountRepository.findByIdForUpdate(memberId) | ||
| .orElseThrow(() -> new AccountNotFoundException(memberId)); |
There was a problem hiding this comment.
반드시 대상을 조회해야 하는 경우에는 get~~
대상이 있는지 없는지 모르지만 찾아보는 경우(null 허용)에는 findBy~~로 컨벤션을 통일해주면 이 메서드의 의도를 명확하게 드러낼 수 있습니다.
메서드 명칭은 find이나, 실제로는 데이터가 없을 경우 예외를 발생시키고 있으니 메서드 이름에서 의도를 명확하게 드러낼 수 있는 방법을 고민해보시는건 어떨까요?
또한, 불필요하게 메서드 파라미터로 Wrapper Type을 받고 있습니다.
null이 허용되지 않는 파라미터라면 Wrapper Type의 사용을 지양해주세요~
There was a problem hiding this comment.
데이터가 조회되지 않을 경우 예외를 던지는 현재 흐름에서는 말씀 주신 것처럼 get~~ 형태로 네이밍하는 것이 메서드의 의도를 더 명확하게 드러내는 방향이라고 생각합니다. 사용하는 입장에서도 해당 리소스가 반드시 존재해야 하는 상황이라 get이 더 어울리는 표현인 것 같습니다.
평소에 관성적으로 findBy~~를 사용하는 경우가 있는 것 같은데, 앞으로 이런 맥락에 따라 네이밍을 좀 더 신중하게 고려해보겠습니다!
그리고 현재 메서드에서 파라미터에 Wrapper 타입을 사용할 필요는 없는데, 이런 부분도 수정하고 이후에는 기본형을 우선적으로 고려하는 습관을 들이도록 하겠습니다!
| /** | ||
| * 송금자 ID로 송금 내역을 조회합니다. | ||
| * @param senderId 송금자 ID | ||
| * @return 조회된 송금 내역 | ||
| */ | ||
| RemittanceHistory findBySenderId(final Long senderId); |
There was a problem hiding this comment.
송금자로 히스토리를 조회한다면 송금내역이 여러개가 있을 수 있지 않을까요? 리턴 타입이 단건조회형태인데, 여러건이 될 가능성은 없을까요?
| /** | ||
| * 송금자 ID로 송금 요청 정보를 조회합니다. | ||
| * @param senderId 조회할 송금자 ID | ||
| * @return 조회된 송금 요청 정보 | ||
| */ | ||
| Optional<RemittanceRequest> findBySenderId(final Long senderId); |
There was a problem hiding this comment.
이 부분도 마찬가지로 송금자가 여러건의 송금 정보를 가지고 있지 않을까요?
| /** | ||
| * 송금자 ID로 송금 요청 상태의 변경 이력을 조회합니다. | ||
| * @param senderId 송금자 ID | ||
| * @return 송금 요청 상태 변경 이력 | ||
| */ | ||
| Optional<RemittanceStatusHistory> findBySenderId(final Long senderId); |
There was a problem hiding this comment.
단순히 송금자의 id로는 특정 송금 건을 조회하기는 어렵습니다.
sender Id는 member의 id일텐데요, 한명의 유저는 여러건의 송금을 할 수 있으므로 정확한 송금내역의 seq로 조회가 필요합니다.
There was a problem hiding this comment.
송금자 ID만으로는 특정 송금 건을 조회하기 어려운 점을 고려하여 requestId를 기준으로 조회하도록 수정했습니다!
- 송금 내역, 송금 요청, 요청 상태 이력을 requestId로 조회하도록 수정
|
리뷰해주신 사항 반영하여 다시 리뷰 요청드립니다 🙇♂️ @f-lab-maverick
|
f-lab-maverick
left a comment
f-lab-maverick
left a comment
There was a problem hiding this comment.
아직 관습적으로 Wrapper Type을 사용하는 부분이 많이 보입니다.
nullable한 값은 최대한 지양하여 명확한 타입을 사용할 수 있도록 신경써주세요.
고생하셨습니다. 머지해주셔도 좋습니다. 테스트 꼼꼼히 진행해주세요~
@f-lab-maverick
안녕하세요, 송금 요청 기능 개발 완료되어 리뷰 요청 드립니다. 😄
이번 PR은 송금 요청 생성에 집중되어있고, 다음 기능은 별도의 PR에서 다룰 예정입니다.
추가된 주요 로직은 다음과 같습니다.
기타 기술 이슈로 MyBatis 반환값 이슈가 있었습니다. 🤣
또한 송금 요청 시 송금 내역(
RemittanceHistory)은 트랜잭션 실패와 관계없이 저장되어야 하지만, 현재는 트랜잭션 내에 포함되어 있어, 이 부분은 이후 PR에서 반영하겠습니다.감사합니다.