오늘 받은 코드 리뷰에 대해서 정리해 보려고 합니다.
문제가 되는 부분이 있다면 댓글로 자유롭게 남겨주세요.
❓record를 활용하자
public class CustomerResponse { // record
private final String email;
private final LocalDateTime createdAt;
private final String name;
private final LocalDateTime lastLoginAt;
public CustomerResponse(Customer customer) {
this.email = customer.getEmail();
this.createdAt = customer.getCreatedAt();
this.name = customer.getName();
this.lastLoginAt = customer.getLastLoginAt();
}
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append(name).append(" ")
.append(email).append(" ")
.append(createdAt).append(" ")
.append(lastLoginAt).append(" ");
return sb.toString();
}
public String getName() {
return name;
}
}디컴파일
❓ 개행 😂
before
after
//befor
}
}
// after
}
}//before
@Mock
private KeyGenerator keyGenerator;
@Mock
private VoucherCreator voucherCreator;
//after
@Mock
private KeyGenerator keyGenerator;
@Mock
private VoucherCreator voucherCreator;❓CustomerResponse인 dto에 출력과 관련된 부분이 있어도 될까
지금 당장은(지금 프로젝트 수준에서는 + 콘솔환경) 문제 없다 왜냐하면 NPE가 발생할 일이 없기 때문에
하지만 DTO에는 비즈니스 로직이 있어서는 안된다.
❓ name의 검증을 Customer에서 알 필요가 있을까
private void validateName(String name) {
if (name == null || name.isBlank()) {
throw new IllegalArgumentException(ErrorMessage.NULL_ARGUMENT.getMessage());
}
}
public void changeName(String name) {
validateName(name);
this.name = name;
}❓ DtoConverter 역할을 하는 클래스에서 dto를 도메인으로 변경하는 로직은 조심하자
@Component
public class WalletConverter {
private final KeyGenerator keyGenerator;
public WalletConverter(KeyGenerator keyGenerator) {
this.keyGenerator = keyGenerator;
}
public Wallet convertWallet(WalletRequest walletRequest) {
int id = keyGenerator.make();
int customerId = walletRequest.getCustomerId();
int voucherId = walletRequest.getVoucherId();
return new Wallet(id, customerId, voucherId);
}
}❓삭제에 관해서 생각해 볼 것 (소프트 삭제와 로그파일)
public class Wallet {
private final int customerId;
private final int voucherId;
private final int walletId;
private boolean deleted = false;
public Wallet(int walletId, int customerId, int voucherId) {
this.walletId = walletId;
this.customerId = customerId;
this.voucherId = voucherId;
}저의 경우 사용자가 데이터를 마음대로 삭제하는 건 위험하다고 생각하여 deleted라는 boolean 변수를 사용하여
삭제 여부를 표시했고
이로써 관리자가 삭제 여부를 판단하고 삭제할 수 있도록 관리자의 역할을 나두게 되었습니다.
하지만 멘토님께서
- 소프트 delete
- 로그 파일을 이용해서 사용자가 삭제 시에 DB에서는 삭제하는 로그 파일을 이용해서 관리자가 확인하는 방법이 있다고
소개해주셨습니다.
❓로그에 에러 메세지 넘겨주기
before
@Override
public void run(String... args) {
Power power = Power.ON;
while (power.isOn()) {
try {
Menu menu = guideStartVoucher();
Command command = commandCreator.createCommand(menu);
power = command.execute();
} catch (IllegalArgumentException e) {
logger.error("사용자의 잘못,된 입력이 발생하였습니다.");
output.write(e.getMessage());
}
}
}after
@Override
public void run(String... args) {
Power power = Power.ON;
while (power.isOn()) {
try {
Menu menu = guideStartVoucher();
Command command = commandCreator.createCommand(menu);
power = command.execute();
} catch (IllegalArgumentException e) {
logger.error("사용자의 잘못,된 입력이 발생하였습니다. {0}", e);
output.write(e.getMessage());
}
}
}❓예상치 못한 프로그램 종료에 대해 방어하기
before
@Override
public void run(String... args) {
Power power = Power.ON;
while (power.isOn()) {
try {
Menu menu = guideStartVoucher();
Command command = commandCreator.createCommand(menu);
power = command.execute();
} catch (IllegalArgumentException e) {
logger.error("사용자의 잘못,된 입력이 발생하였습니다. {0}", e);
output.write(e.getMessage());
}
}
}after
@Override
public void run(String... args) {
Power power = Power.ON;
while (power.isOn()) {
try {
Menu menu = guideStartVoucher();
Command command = commandCreator.createCommand(menu);
power = command.execute();
} catch (IllegalArgumentException e) {
logger.error("사용자의 잘못,된 입력이 발생하였습니다. {0}", e);
output.write(e.getMessage());
} catch (Exception e) {
logger.error("알 수 없는 error 발생");
}
}
}IllegalArgumentException인 언체크드 인셉션 외의 예외가 발생할 경우를 대비
❓ErrorMessage에 대해서 신경쓰기
NULL_ARGUMENT("null이 발생하였습니다."),null이어서는 안됩니다와 같은 진짜 에러를 나타내는 메세지
❓정말로 Bean으로 등록해야 하는가 생각해보기
Dispatcher Servlet
❓ 정말 Mock이 필요한가
외부인프라, 메세지큐의 경우 Mocking만으로 테스트가 가능하다
그러나 서비스처럼 핵심 비즈니스 로직은 목킹으로 테스트 하는게 의미가 있을까
@SpringBootTest를 이용해서 진짜로 되는지 확인해야 하는데 목킹이 의미가 있을까
생각해봐야 한다.
class WalletServiceTest {
private static final int CUSTOMER_ID = 10;
private static final int VOUCHER_ID = 10;
private static final int WALLET_ID = 10;
private static final String TEST_USER_NAME = "test-user";
private static final String TEST_USER_EMAIL = "test1-user@gmail.com";
private static final LocalDateTime CREATE_AT = LocalDateTime.now();
@Mock
private WalletRepository walletRepository;
@Mock
private VoucherConverter voucherConverter;
@Mock
private WalletConverter walletConverter;
@Mock
private CustomerConverter customerConverter;
@InjectMocks
private WalletService walletService;
private WalletRequest walletRequest;
private Wallet wallet;
@BeforeEach
void setup() {
MockitoAnnotations.openMocks(this);
walletRequest = new WalletRequest(CUSTOMER_ID, VOUCHER_ID);
wallet = new Wallet(WALLET_ID, CUSTOMER_ID, VOUCHER_ID);
}
@Test
@DisplayName("사용자가 바우처를 등록하는 지갑 정보와 지갑 정보를 등록해주는 서비스로 등록된 지갑정보는 같다.")
void giveVoucher_InsertWallet_Equals() {
//given
when(walletConverter.convertWallet(walletRequest)).thenReturn(wallet);
when(walletRepository.insert(wallet)).thenReturn(wallet);
BDDMockito.given().willReturn()
//when
Wallet result = walletService.giveVoucher(walletRequest);
//then
assertThat(result).isEqualTo(wallet);
verify(walletRepository, times(1)).insert(wallet);
}
@Test
@DisplayName("사용자가 삭제하고자 하는 지갑 정보와 바우처를 삭제헤주는 서비스를 통해 삭제된 지갑 정보는 같다.")
void takeVoucher_DeleteWallet_Equals() {
//given
when(walletRepository.deleteWithVoucherIdAndCustomerId(VOUCHER_ID, CUSTOMER_ID)).thenReturn(wallet);
//when
Wallet result = walletService.takeVoucher(walletRequest);
//then
assertThat(result).isEqualTo(result);
verify(walletRepository, times(1)).deleteWithVoucherIdAndCustomerId(VOUCHER_ID, CUSTOMER_ID);
}
@Test
@DisplayName("임의로 고객의 정보를 넣었을 때, 그 고객의 정보가 바우처 아이디를 통해 찾은 고객 정보 리스트에 포함되어 있다.")
void customerList_CustomerResponseList_Contains() {
//given
List<Customer> customers = new ArrayList<>();
Customer customer = new Customer(CUSTOMER_ID, TEST_USER_NAME, TEST_USER_EMAIL, CREATE_AT);
customers.add(customer);
List<CustomerResponse> customerResponses = List.of(new CustomerResponse(customer));
when(walletRepository.findAllCustomersByVoucher(VOUCHER_ID)).thenReturn(customers);
when(customerConverter.convertCustomerResponse(customers)).thenReturn(customerResponses);
//when
List<CustomerResponse> result = walletService.customerList(VOUCHER_ID);
//then
assertThat(result).containsExactlyElementsOf(customerResponses);
verify(walletRepository, times(1)).findAllCustomersByVoucher(VOUCHER_ID);
verify(customerConverter, times(1)).convertCustomerResponse(customers);
}
@Test
@DisplayName("임의로 바우처의 정보를 넣었을 때, 그 바우처의 정보가 고객 아이디를 통해 찾은 바우처 정보 리스트에 포함되어 있다.")
void voucherList_VoucherResponseList_Contains() {
//given
Voucher voucher = new FixedAmountVoucher(VOUCHER_ID, new FixedDiscount(20), VoucherType.FIXED_AMOUNT_VOUCHER);
List<Voucher> voucherList = List.of(voucher);
Vouchers vouchers = new Vouchers(voucherList);
List<VoucherResponse> voucherResponses = List.of(new VoucherResponse(voucher));
when(walletRepository.findAllVouchersByCustomer(CUSTOMER_ID)).thenReturn(vouchers);
when(voucherConverter.convertVoucherResponse(vouchers)).thenReturn(voucherResponses);
//when
List<VoucherResponse> result = walletService.voucherList(CUSTOMER_ID);
//then
assertThat(result).containsExactlyElementsOf(voucherResponses);
verify(walletRepository, times(1)).findAllVouchersByCustomer(CUSTOMER_ID);
verify(voucherConverter, times(1)).convertVoucherResponse(vouchers);
}
}
문제의 코드 죄다 목킹인데
껍데기에 리턴값 이거다 알려주고 확인하는 용도에서만 그친다
정말 좋은 테스트일까?
핵심 비즈니스로직이 있는 코드인데
❓ BDDMokito.given()이 when()보다 의미가 더 잘 전달된다.
❓ Assertion.assertThatIllegalArgumentException()도 있다.
❓ 사용자에게 개인 정보를 노출할 수도 있으니 return 값에 주의하자
public Voucher createVoucher(VoucherType voucherType, double discountAmount) {
int id = keyGenerator.make();
Discount discount = discountCreator.createDiscount(voucherType, discountAmount);
Voucher voucher = voucherCreator.createVoucher(id, voucherType, discount);
return voucherRepository.insert(voucher);
}현재 Voucher가 있는데 사용자가에게 Voucher 안에 있는 중요 정보가 그대로 노출될 수 있는 환경이다.
❓상수표기법와 카멜케이스
final 카멜케이스
static final 상수펴기법
❓노란색 줄로 추천하는 거 적용하자
❓다양한 랜덤 클래스에 대해서 알아보자
🔥🔥🔥 내 코드의 가장 큰 문제 생성된 빈을 다시 생성한다🔥🔥🔥
@Component
public class CommandCreator {
private final Map<Menu, Command> strategies;
private final WalletService walletService;
private final VoucherService voucherService;
private final Input input;
private final Output output;
public CommandCreator(WalletService walletService, VoucherService voucherService, Input input, Output output) {
this.input = input;
this.output = output;
strategies = new HashMap<>();
this.voucherService = voucherService;
this.walletService = walletService;
strategies.put(Menu.EXIT, new ExitCommand(this.output));
strategies.put(Menu.LIST, new ListCommand(this.output, voucherService));
strategies.put(Menu.CREATE, new CreateCommand(this.input, this.output, voucherService));
strategies.put(Menu.GIVE_VOUCHER, new GiveVoucherCommand(this.output, this.input, walletService));
strategies.put(Menu.TAKE_VOUCHER, new TakeVoucherCommand(this.output, this.input, walletService));
strategies.put(Menu.CUSTOMER_LIST, new CustomerListCommand(this.output, this.input, walletService));
strategies.put(Menu.VOUCHER_LIST, new VoucherListCommand(this.input, this.output, walletService));
}
public Command createCommand(Menu menu) {
return strategies.get(menu);
}
} 전략패턴을 사용했는데
이미 있는 빈을 생성한다.
사용자 요청마다 만드는 것...!!
Price에 빼먹은 비즈니스 로직이 하나 있는데 그거 테스트에 추가하자
꾸준하게 Ready, Set, Go!