-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
박자스동터차 경주 2단계 #8
base: main
Are you sure you want to change the base?
Conversation
* feat: 자동차 경주 코드 가져오기 * feat: 웹 요청,응답 구현 * feat: service 분리 * feat: 데이터베이스 연결 * feat: table 추가 * feat: 데이터베이스 저장하는 기능 구현 * test: car dao 테스트 추가 * test: racing game dao 테스트 추가 * test: racing game service 테스트 추가 * test: transactional 어노테이션 추가 * test: service test 리팩터링 * test: controller test 추가
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
화이팅 뭘 저렇게 많이 적었냐
private void validateLength(String name) { | ||
if (name.length() > MAX_NAME_LENGTH) { | ||
throw new IllegalArgumentException("이름이 " + name.length() + "자 입니다. " + "이름은 5자 이하로 가능합니다."); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 name 쪽으로 가도 좋을지도?
private final Name name;
} | ||
|
||
private void validateDuplicateName(List<Car> cars) { | ||
Set<String> distinctNames = cars.stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set 보다는 distinct 쪽이 더 좋을 것 같아보이기도 하고 아닌것 같기도 하고 어떻게 생각해?
|
||
private final NumberGenerator numberGenerator; | ||
private final Cars cars; | ||
private int tryCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것도 객체로 빼도 괜찮아보이는데?
private final TryCount tryCount;
원시값 포장 느낌으로
|
||
@Override | ||
public int generate() { | ||
return (int) (Math.random() * BOUND); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Math.random() 을 사용한 이유는?
@@ -0,0 +1,20 @@ | |||
package racingcar.dto; | |||
|
|||
public class CarDto { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍이 CarResponse 가 아닌 이유가 뭐야?
응답에서만 쓰이는 것 같은데
void findByGameId() { | ||
List<CarEntity> carEntities = carDao.findByRacingGameId(gameId); | ||
|
||
assertThat(carEntities).hasSize(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 조금 더 자세하게 내부 데이터 검증해도 괜찮을거 같은데?
import racingcar.dto.RacingGameResponse; | ||
import racingcar.service.RacingGameService; | ||
|
||
@WebMvcTest(RacingGameController.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
webmvc test 굿
List.of("현구막"), | ||
List.of(new CarDto("현구막", 10), new CarDto("박스터", 7)) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현구막이 이렇게 많이 나오다니 부럽네요
static class InnerRacingGameDao implements RacingGameDao { | ||
private final Map<Long, RacingGameEntity> database = new HashMap<>(); | ||
private Long id = 1L; | ||
|
||
@Override | ||
public Long save(RacingGameEntity racingGameEntity) { | ||
database.put(id, racingGameEntity); | ||
id++; | ||
return id - 1; | ||
} | ||
|
||
@Override | ||
public List<RacingGameEntity> findAllByCreatedTimeAsc() { | ||
return database.values() | ||
.stream().sorted(Comparator.comparing(RacingGameEntity::getCreatedTime)) | ||
.collect(Collectors.toList()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
웨 static class?
} | ||
|
||
public RacingGameResponse play(RacingGameRequest racingGameRequest) { | ||
Cars cars = toCars(racingGameRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서비스 제일 앞에서 dto 뽀개는거 굿
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 박스터 🫡
많이 배워갑니다 🔥
this.racingGameAddService = racingGameService; | ||
this.racingGameFindService = racingGameFindService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command
와 Query
를 분리하셨네요 👍
@@ -0,0 +1,32 @@ | |||
package racingcar.dao.entity; | |||
|
|||
public class CarEntity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entity
를 식별할 수 있는 식별자가 없는 것 같은데 의도하신걸까요? 🤔
|
||
public class RacingGameRequest { | ||
|
||
@Size(min = 2, message = "참가자는 최소 2명이여야 합니다") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 메시지까지 설정한 거 좋은데요 👍
RacingGame racingGame = new RacingGame(racingGameRequest.getCount(), cars); | ||
racingGame.run(); | ||
|
||
Long racingGameId = racingGameDao.save(new racingcar.dao.entity.RacingGameEntity(racingGameRequest.getCount())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RacingGameEntity
를 이렇게 import
하는 이유가 궁금합니다!
import racingcar.dto.RacingGameResponse; | ||
|
||
@Service | ||
public class RacingGameAddService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조회에 @Transactional
어노테이션을 사용한만큼 통일성 있게 여기도 사용하는건 어떨까요?
List.of(new CarDto("현구막", 10), new CarDto("박스터", 7)) | ||
); | ||
String requestString = objectMapper.writeValueAsString(request); | ||
when(racingGameService.play(any())).thenReturn(expectedResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mockito
와 BDDMockito
를 선택하는 기준이 궁금합니다~
.andExpect(status().isCreated()) | ||
.andExpect(jsonPath("$.winners[0]", is("현구막"))) | ||
.andExpect(jsonPath("$.racingCars", hasSize(2))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
꼼꼼한 응답 결과 검증 굳
String sql = "SELECT trial_count FROM racing_game WHERE id = " + gameId; | ||
Integer savedTrialCount = jdbcTemplate.queryForObject(sql, Integer.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
쿼리를 직접 날려확인하기보다는 findAllByCreatedTimeAsc
메서드를 활용해 테스트하는건 어떨까요?
No description provided.