[3,4 단계 - 체스] 제나(위예나) 미션 제출합니다.#579
Conversation
jnsorn
left a comment
There was a problem hiding this comment.
안녕하세요 제나! 3,4단계도 잘 구현해주셨네요. 😎
같이 얘기해보면 좋을 부분에 코멘트 남겨두었는데, 확인해보시고 언제든 DM 주세요!
| - [x] Delete | ||
| - [x] King 이 죽으면 DB 초기화 해준다 | ||
|
|
||
| - Query |
There was a problem hiding this comment.
- readme도 같이 잘 업데이트 해주셨네요 👍
- sql은 init.sql 파일을 통해 관리하고 있으니, readme에서는 제거하는게 어떨까요? sql이 변경이 필요하면 두 곳의 수정이 필요해져서 점점 관리에 소홀해질 것 같아요 ㅎㅎ
- 추가로 로컬 환경 세팅이 필요한 경우에는 readme를 통해 가이드를 적어두면, 프로그램을 더 편하게 돌려볼 수 있겠네요.
| public Connection getConnection() { | ||
| // 드라이버 연결 | ||
| try { | ||
| return DriverManager.getConnection("jdbc:mysql://" + SERVER + "/" + DATABASE + OPTION, USERNAME, PASSWORD); | ||
| } catch (final SQLException e) { | ||
| System.err.println("DB 연결 오류:" + e.getMessage()); | ||
| e.printStackTrace(); | ||
| return null; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
- 커넥션 객체 분리 👍
- SQLException 발생시 Null을 리턴하고 있는데, 문제는 없을까요?
- e.printStackTrace()를 수행하는 이유는 무엇인가요?
- 예외 발생시 connection 리소스가 정상적으로 반환될까요?
There was a problem hiding this comment.
학습 시에 배운 예제 코드를 그대로 사용해서 문제가 많았습니다 !! 😥
- null을 리턴하면 connection 을 사용하는쪽에서 NPE 발생가능성이 있을 거 같습니다
- printStackTrace() 를 공부해보니 사용하지 말아야 할 이유가 여럿 있었고(java reflection 사용, 출력을 관리하기 힘듬, 서버에서 스택을 추적하는데 부하 발생 가능) 예외 발생을 바로 화면에 출력해버리면 사용자에게 그대로 예외발생이 노출된다고 생각하여 DBconnectionException 을 커스텀해서 던져주는 것으로 변경했습니다. 해당 예외를 캐치 해줘야 할지 혹시 캐치 한다면 어느 상위 객체에서 해줘야 할지 고민이 됩니다 (지금은 상위로 올라가면 바로 컨트롤러가 있고 그 위에는 메인문이 있습니다)
- Dao 에서는 try-with-resources 로 처리해서 실패 시 바로 자원을 반납해줄 거 같은데 지금 DriverManager.getConnection 이 실패하면 예외 발생 -> 프로그램 종료가 될테니 자원은 반납이 되지 않을까요 ?? (사실 잘 모르겠습니다 !!)
There was a problem hiding this comment.
- 맞습니다. 뿐만 아니라 예외상황임에도 호출하는 쪽에서는 예외상황임을 인지할 수 없게 됩니다. EffectiveJava 아이템 77. 예외를 무시하지 말라
- 좋습니다 👍 복구할 수 있는 오류라면, 복구 할 수 있는 위치에서 catch하는 것도 좋을 것 같습니다. 다만 해당 예외는 DB 연결 오류니 그냥 예외만 발생시키고 끝내도 될 것 같아요.
- 강제로 예외가 발생하는 상황을 만들어 디버깅 해봐도 좋겠네요. 😃
| public static ChessGame load(ChessGameDao chessGameDao) { | ||
| // 조회 | ||
| ChessGame chessGame = chessGameDao.select(); | ||
| if (chessGame == null) { | ||
| chessGame = new ChessGame(); | ||
| // 생성 후 저장 | ||
| chessGameDao.save(chessGame); | ||
| } | ||
| return chessGame; | ||
| } | ||
| } |
| } | ||
|
|
||
| @Override | ||
| public ChessGame select() { |
| public boolean isUpsideDiagonalPosition(Position target) { | ||
| int fileGap = target.file - this.file; | ||
| int rankGap = target.rank - this.rank; | ||
| return Math.abs(fileGap) == 1 && rankGap == 1; | ||
| Direction direction = Direction.findDirection(target.file - this.file, target.rank - this.rank); | ||
| return direction.equals(Direction.UP_LEFT) || direction.equals(Direction.UP_RIGHT); | ||
| } | ||
|
|
||
| public boolean isDownSideDiagonalPosition(Position target) { | ||
| int fileGap = target.file - this.file; | ||
| int rankGap = target.rank - this.rank; | ||
| return Math.abs(fileGap) == 1 && rankGap == -1; | ||
|
|
||
| Direction direction = Direction.findDirection(target.file - this.file, target.rank - this.rank); | ||
| return direction.equals(Direction.DOWN_LEFT) || direction.equals(Direction.DOWN_RIGHT); | ||
| } |
There was a problem hiding this comment.
direction을 만들어 비교하지말고, 아예 direction에게 isUpsideDiagonal인지 물어볼수도 있을 것 같아요.
There was a problem hiding this comment.
direction 에게 물어보는 방식으로 다시 수정해봤습니다!! 이 방식이 또링이 의도하신게 맞을까욥 ??
|
|
||
| public final class ChessBoard { | ||
| public static final int INITIAL_KING_COUNT = 2; | ||
| private static final Map<ChessGame, ChessBoard> CACHE = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
개인적으로 지금 구조에서 ChessBoard를 캐싱해서 얻는 장점이 크게 없을 것 같은데요, 어떤 경우를 생각해서 캐싱하신건지 궁금해요! 😃
There was a problem hiding this comment.
현재는 chessGame 을 하나만 가지고 있지만 유저가 여러명이라 chessGame 이 여러개 만들어질 경우를 대비해서 게임당 보드 하나만 두려고 캐싱을 해두었습니다.... 사실 불필요해보이긴 하는데 혹시나.. 해서 지금은 놔두고 싶습니닷 ..
| } | ||
|
|
||
| private static Double sumScorePawn(TeamColor color, Map<Position, Piece> board) { | ||
| return IntStream.range(0, 8) |
| private State move(final Command command) { | ||
| final List<String> commands = command.getCommands(); | ||
| final Position source = PositionConverter.convert(commands.get(SOURCE_INDEX)); | ||
| final Position target = PositionConverter.convert(commands.get(TARGET_INDEX)); | ||
| chessGame.setUp(source, target, teamColor); | ||
| if (chessGame.isEnd()) { | ||
| return new End().run(chessGame); | ||
| } | ||
| return new Move(chessGame, teamColor.changeTurn()); | ||
| } |
There was a problem hiding this comment.
status 상태를 추가하는 과정에서 Move 와 Status 에서 command 를 확인해 상태를 전환하는 메소드에 if 분기문이 하나 더 추가되어 총 11줄이 되어버렸습니다...
프로그램이 종료될 때 chess game 이 종료되었단 정보를 controller 에 넘겨줘야 하는데 현재 구조로는 ChessController 에 해당 코드를 넘길 수가 없어서 Move 객체의 move 메소드에서 확인해서 게임이 종료된 상태면 End 상태를 반환하게 하는데 설계 변경이 필요해 보입니다.
command 의 입력 상태를 처리하는 로직과 chess game 의 완료 상태를 확인 후 처리하는 로직을 분리하고 싶은데 상태 패턴을 걷어내야 할까요 ??
좋은 고민이네요! 지금 구조에서는 Move에서 분기문을 처리하지 않고, Status에서 처리해도 될 것 같아요.
@Override
public State checkCommand(Command command) {
if (command.isEnd() || chessGame.isEnd()) {
return new End().run(chessGame);
}
if (command.isMove()) {
Move move = new Move(chessGame, teamColor);
return move.checkCommand(command);
}
if (command.isStatus()) {
return run();
}
throw new UnsupportedOperationException("end, move 명령어만 입력 가능합니다.");
}그리고 State에 available(Command)와 같은 메서드를 만들어 각 구현체의 조건을 넣어주면 위 분기문도 없앨 수 있을 것 같은데요, 혹시 이해가 안가시거나 제가 질문을 잘못 이해한거라면 DM 주세요 😃
There was a problem hiding this comment.
지금 체스 게임 플레이가 Move 상태의 move 메소드에서 chessgame.setup() 으로 시작해서 Status 에서 처리하기는 힘들거같아보입니다..!! (Status 에서 처리가 된다면 status 를 입력해야만 체스게임이 종료되었는지를 확인하게 됨)
말씀해주신대로 available(Command) 의 메소드를 만들어서 분기문을 없애는 방식이 최선일 거 같은데 혹시 어떻게 조건을 넣어줘야 할지 힌트를 좀만 더 주실수있을까요 ?? 😿
There was a problem hiding this comment.
앗 그렇군요. 제가 구조를 잘못 이해한 것 같네요 😭😭 그렇다면 현재 �로직도 괜찮을 것 같아요!
| Map<Position, Piece> board = chessGame.getChessBoard(); | ||
| for (Map.Entry<Position, Piece> boardEntry : board.entrySet()) { | ||
| final var query = "INSERT INTO chess_game(piece_type, piece_rank, piece_file, team, turn) VALUES (?, ?, ?, ?, ?)"; | ||
| try (final var connection = dbConnection.getConnection(); |
There was a problem hiding this comment.
save, select, delete 등 DB 연결을 하는 메소드를 실행할 때마다
connection = dbConnection.getConnection()
를 실행하고 있는데 너무 비효율적인 거 같아서 개선할 수 있는 방법이 있을까요 ?? DB 에 연결하는 것이 비용이 크다고 배웠는데 연결할 때마다 connection 을 만들고 close 해주는게 옳은 방법인지 궁금합니다 !
좋은 고민이네요! 상황에 따라 달라지겠지만, 현재 구조도 충분히 괜찮아 보입니다. 다만 연결 비용이 걱정된다면 커넥션 풀이라는 키워드에 대해 찾아보면 좋을 것 같아요. DB 연결이 필요할때마다 커넥션을 새로 만들지 않고, 미리 만들어둔 커넥션을 이용하여 사용하고 반환하는 방식입니다. (참고로 커넥션 풀의 크기가 너무 크거나 작아도 문제가 발생하니 실제 서비스 환경에선 적절한 테스트를 통해 사이즈를 조절해야합니다.)
추가로 제나에게 궁금한게 있어요~ 하나의 요청에 대해 connection 하나로 처리하는 방법도 있었을 것 같은데, 위와 같이 구현하신 이유를 알 수 있을까요~? 어떤 단점이 느껴져서 이 방식을 선택하신건지 궁금해요 ㅎㅎ (어떤 의도를 가지고 한 질문은 아니고, 커넥션에 대해 고민이 많아보이셔서 궁금해서 질문드렸습니다.)
There was a problem hiding this comment.
- 커넥션 풀! 키워드 확인했습니다
- 사실 DB 관련 코드는 코드를 복사해서 사용해서 for 문 안에서 연결을 계속 생성하고 있는지 몰랐습니다,,,,😂 Connection 하나만 생성해준 후 반복문 도는 방향으로 수정했습니다 !!
| public void update(ChessGame chessGame) { | ||
| delete(chessGame); | ||
| save(chessGame); | ||
| } | ||
|
|
There was a problem hiding this comment.
체스 말이 움직일 때 마다 update 를 해야 하는데 UPDATE 쿼리를 어떻게 작성해야 할지 감이 안와서 피드백 강의 때 학습했던 방식으로 chess game DB 를 delete 를 해주고 다시 chess game 을 save 하는 방식으로 구현했습니다.
포지션을 찾아 team과 piece를 바꿔주는 방법이 생각나는데, 혹시 이미 시도해보셨다면 어디서 막히셨는지 DM주세요!
There was a problem hiding this comment.
포지션을 기반으로 바꿔주는 방법을 시도하다가 막혔던 부분입니다!
현재 Dao 가 update 를 호출하는 부분이 컨트롤러인데 컨트롤러는 chessGame 밖에 알지 못하는 상태입니다.
그래서 매개변수로 chess game만을 넣어줄 수 밖에 없는데 chessgame 에서 불러올수 있는 정보는 move 가 된 이후의 Map<Position, Piece> board 뿐이라서
a2 -> a4 이동 시 pawn 을 지우고 a4 를 추가해야 한다 의 상황 발생 시
지금 chessgame 에서 사용할 수 있는 chessboard 에는 a4 저장된 pawn 의 정보밖에 담고있지 않기때문에 a2 에서 이동했다는걸 알수가 없어서 정보를 업데이트할 수 가 없다는 문제점이 있습니다 ㅠㅠ
There was a problem hiding this comment.
아하.. source도 업데이트를 해주어야 하는데, 알 수 없어서 문제였네요! 말씀해주신대로, 구조를 그대로 가져가면서 변경된 부분만 저장하기는 어려울 것 같아요.😭 변경점을 알아야 하는데 현재 command는 알 수 없는 상황이니까요. 기존 board를 findAll해와서 현재와 비교하여 바뀐 위치를 알아내는 방법도 있긴 할 것 같은데, 효율적인 방법은 아닌 것 같아 망설여지네요... 즉 정리해보자면 아래와 같을 것 같아요.
- 기존 구조를 그대로 유지한다면 기존에 저장된 보드와 비교하여 변경점을 찾아낸다.
- 변경점(command)을 dao가 이용할 수 있는 구조로 이용해, 해당 정보를 통해 변경된 부분만 업데이트한다.
지금 구조에서는 제나가 구현해준 방식도 합리적일 것 같아서, 굳이 수정은 안하셔도 될 것 같네요.
|
안녕하세요 또링! 리뷰반영이 늦었습니다.. |
jnsorn
left a comment
There was a problem hiding this comment.
안녕하세요 제나~ 방학임에도 피드백 반영 하시느라 고생 많으셨어요.
체스 미션은 여기서 머지하도록 할게요.
코멘트 남겨주신 건들은 따로 확인해보고 DM 드릴게요.
안녕하세요 또링!
체스 3,4 단계를 구현해서 리뷰 요청드립니다!
지금까지 미션 중에서 제일 어려워서 구현에 집중하다 보니 코드도 깔끔하지 못한 부분이 많고 요구사항도 지키지 못한 부분들이 있습니다 😥
3, 4 단계를 구현하면서 변경한 주요 사항과 문제점들을 정리해봤습니다.
commandType 에 status 추가
문제점
DB 적용
문제점
save, select, delete 등 DB 연결을 하는 메소드를 실행할 때마다
connection = dbConnection.getConnection()를 실행하고 있는데 너무 비효율적인 거 같아서 개선할 수 있는 방법이 있을까요 ?? DB 에 연결하는 것이 비용이 크다고 배웠는데 연결할 때마다 connection 을 만들고 close 해주는게 옳은 방법인지 궁금합니다 !
체스 말이 움직일 때 마다 update 를 해야 하는데 UPDATE 쿼리를 어떻게 작성해야 할지 감이 안와서 피드백 강의 때 학습했던 방식으로 chess game DB 를 delete 를 해주고 다시 chess game 을 save 하는 방식으로 구현했습니다.
+) 이번 미션때 깃 크라켄을 처음 사용해봤는데 commit 이름을 변경할 수 있는 기능이 있어서 한번 실행해봤다가 똑같은 내용의 커밋이 한번씩 더 생겼습니다 리뷰하시는데 불편하실거같아요 죄송합니다 ㅜㅜ