[1단계 - 웹 자동차 경주] 이리내(성채연) 미션 제출합니다. #1
Conversation
hectick
changed the title
| private long insertGame(int trialCount) { | ||
| String sql = "INSERT INTO game (trialCount, date) VALUES (?, ?)"; | ||
| KeyHolder keyHolder = new GeneratedKeyHolder(); | ||
| jdbcTemplate.update(connection -> { | ||
| PreparedStatement pst = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS); | ||
| pst.setInt(1, trialCount); | ||
| pst.setDate(2, Date.valueOf(LocalDate.now())); | ||
| return pst; | ||
| }, keyHolder); | ||
| long gameId = keyHolder.getKey().longValue(); |
There was a problem hiding this comment.
여기서 마지막 행의 keyHolder.getKey().longValue(); 에서 NPE가 생길 수 있다는 경고가 나옵니다. 하지만 NPE 가 나오는 상황이 어떤 상황인지 잘 모르겠습니다. 쿼리가 잘못되었다면 예외가 발생해서 null값이 들어갈 수 없을 것 같습니다. 이 부분이 NPE 처리가 꼭 필요한 부분인가요? 어떤 이유에서 NPE 처리를 해줘야 하는 건가요?
There was a problem hiding this comment.
keyHolder.getKey() 메서드를 들어가보면 @Nullable 어노테이션이 붙어있습니다~ null일 수 있음을 명시해두었기 때문에 npe 경고를 알려주고 있는건데요! 이리내와 같은 경우라면 말씀해주신 것처럼 NPE케이스가 발생하진 않을 것 같은데요!
아래처럼 getKey를 jdbcTemplate.update 이전에 부른다면 NPE가 발생하겠죠? (어떤 케이스에 null을 반환하는지 알고 싶다면 GeneratedKeyHolder 클래스의 getKey 메서드를 찾아보면 어떨까요.ㅎㅎㅎ)
null일 수 있음을 어노테이션으로 경고해주었으나 이리내가 설정한 상황에서는 null이 아님을 보장한다면 Objects.requiredNonNull로 null이 아닌 상황을 확실히 해주면 어떨까 싶어요~!
KeyHolder keyHolder = new GeneratedKeyHolder();
long gameId = keyHolder.getKey().longValue();
jdbcTemplate.update(connection -> {
PreparedStatement pst = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS);
pst.setInt(1, trialCount);
pst.setDate(2, Date.valueOf(LocalDate.now()));
return pst;
}, keyHolder);| jdbcTemplate.batchUpdate(sql2, new BatchPreparedStatementSetter() { | ||
| @Override | ||
| public void setValues(PreparedStatement ps, int i) throws SQLException { | ||
| ps.setLong(1, gameId); | ||
| ps.setString(2, racingCars.get(i).getName()); | ||
| ps.setInt(3, racingCars.get(i).getPosition()); | ||
| } | ||
|
|
||
| @Override | ||
| public int getBatchSize() { | ||
| return racingCars.size(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
여러개의 데이터를 삽입하다가 중간에 어느 한 데이터에서 오류가 날 수 있는 경우를 고려해서 그냥 update를 하는 대신에 batchUpdate를 사용했습니다만, 코드가 장황해진 느낌이 듭니다. 이게 최선일까요?
There was a problem hiding this comment.
- 저장시 하나가 문제가 생기면 모든걸 저장하지 않는게 더 좋다고 생각하신 걸까요?
- batchUpdate를 사용하는 이유는 무엇일까요~?
There was a problem hiding this comment.
저장시 하나가 문제가 생기면 모든걸 저장하지 않는게 더 좋다고 생각하신 걸까요?
네! 문제가 생긴 데이터를 저장하하게 된다면, 잘 종료된 데이터와 중간에 갑자기 종료된 데이터가 섞여버려서 혼란스러울 것 같다고 생각했습니다!
batchUpdate를 사용하는 이유는 무엇일까요~?
여러개의 SQL을 하나의 트랜젝션으로 묶어서 실행하기 위함이 아닐까요?
There was a problem hiding this comment.
- 넵! 트랜잭션의 단위는 서비스의 특성에 따라 판단하여 설정하시면 될 것 같습니다 :)
- batchUpdate를 사용하는 이유는 connection 단위를 줄이기 위함에 더 의미가 있다고 생각해서 남긴 코멘트 였습니다!ㅎㅎ
| public void saveGame(int trialCount, ResultDto resultDto) { | ||
| long gameId = insertGame(trialCount); | ||
| insertCar(resultDto, gameId); | ||
| insertWinner(resultDto, gameId); | ||
| } |
There was a problem hiding this comment.
데이터베이스에 테이블은 game, car, winner 세개가 존재합니다. 하지만 insertGame을 성공적으로 실행 하고, 다음으로insertCar를 실행하다가 오류가 나는 경우, 데이터베이스에 game 테이블만 온전히 생성되게 될텐데요, 이런 경우에 game 테이블가지 싹 다 롤백해버릴 수 있는 좋은 방법이 있을까요?
| import racingcar.model.manager.CarMoveManager; | ||
| import racingcar.util.RandomNumberGenerator; | ||
|
|
||
| @Component |
There was a problem hiding this comment.
서비스에 필드로 cars가 있어서 이를 자동으로 주입받기 위해 달아놨는데요,
리팩토링하면서 서비스가 필드로 dao만을 갖게 수정하고, 어노테이션을 @Service, @Controller, @Repository 만 남겨두면서 제거하였습니다!
코멘트는 서비스, dao, 컨트롤러만 빈으로 등록해서 관리하고, 비즈니스 로직은 서비스 안에서 순수하게 처리하는 것이 좋을까요?
| @Autowired | ||
| private Cars cars; | ||
| @Autowired | ||
| private final CarMoveManager carMoveManager; | ||
| @Autowired | ||
| private final GameDao gameDao; |
There was a problem hiding this comment.
각각을 왜 빈으로 관리했고, Autowired를 어노테이션을 사용하였는지 궁금합니다~
There was a problem hiding this comment.
별 의도는 없던 것 같아요. 프로그램이 돌아가게 하기 위해 이런저런 어노테이션을 달아보다가 이런 혼란한 코드가 된것 같네요 ㅜ.ㅜ
| public void saveGame(int trialCount, ResultDto resultDto) { | ||
| long gameId = insertGame(trialCount); | ||
| insertCar(resultDto, gameId); | ||
| insertWinner(resultDto, gameId); | ||
| } |
| private long insertGame(int trialCount) { | ||
| String sql = "INSERT INTO game (trialCount, date) VALUES (?, ?)"; | ||
| KeyHolder keyHolder = new GeneratedKeyHolder(); | ||
| jdbcTemplate.update(connection -> { | ||
| PreparedStatement pst = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS); | ||
| pst.setInt(1, trialCount); | ||
| pst.setDate(2, Date.valueOf(LocalDate.now())); | ||
| return pst; | ||
| }, keyHolder); | ||
| long gameId = keyHolder.getKey().longValue(); |
There was a problem hiding this comment.
keyHolder.getKey() 메서드를 들어가보면 @Nullable 어노테이션이 붙어있습니다~ null일 수 있음을 명시해두었기 때문에 npe 경고를 알려주고 있는건데요! 이리내와 같은 경우라면 말씀해주신 것처럼 NPE케이스가 발생하진 않을 것 같은데요!
아래처럼 getKey를 jdbcTemplate.update 이전에 부른다면 NPE가 발생하겠죠? (어떤 케이스에 null을 반환하는지 알고 싶다면 GeneratedKeyHolder 클래스의 getKey 메서드를 찾아보면 어떨까요.ㅎㅎㅎ)
null일 수 있음을 어노테이션으로 경고해주었으나 이리내가 설정한 상황에서는 null이 아님을 보장한다면 Objects.requiredNonNull로 null이 아닌 상황을 확실히 해주면 어떨까 싶어요~!
KeyHolder keyHolder = new GeneratedKeyHolder();
long gameId = keyHolder.getKey().longValue();
jdbcTemplate.update(connection -> {
PreparedStatement pst = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS);
pst.setInt(1, trialCount);
pst.setDate(2, Date.valueOf(LocalDate.now()));
return pst;
}, keyHolder);| jdbcTemplate.batchUpdate(sql2, new BatchPreparedStatementSetter() { | ||
| @Override | ||
| public void setValues(PreparedStatement ps, int i) throws SQLException { | ||
| ps.setLong(1, gameId); | ||
| ps.setString(2, racingCars.get(i).getName()); | ||
| ps.setInt(3, racingCars.get(i).getPosition()); | ||
| } | ||
|
|
||
| @Override | ||
| public int getBatchSize() { | ||
| return racingCars.size(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
- 저장시 하나가 문제가 생기면 모든걸 저장하지 않는게 더 좋다고 생각하신 걸까요?
- batchUpdate를 사용하는 이유는 무엇일까요~?
안녕하세요 배럴! 리뷰이 이리내입니다~
따끔한 리뷰 부탁드립니다.
페어와 코드를 작성하다가 궁금한 점이 몇개 생겼는데, 그것은 코멘트로 달아두겠습니다!