Skip to content
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

[Spring 체스 - 1단계] 디우(김동규) 미션 제출합니다. #380

Merged
merged 94 commits into from Apr 25, 2022

Conversation

tco0427
Copy link

@tco0427 tco0427 commented Apr 24, 2022

안녕하세요 앨런!! 디우라고 합니다🙂

이번 "웹 체스" 미션 Spring 적용하기가 수월하게 진행될거라는 예상과는 달리 페어와 여러가지 문제를 만나고, 고민을 하다보니 시간이 꽤나 걸렸네요...ㅠ.ㅠ


PRG 패턴

우선 기존과 달리 "Post-Redirect-Get" 패턴을 지킬 수 있도록 하였습니다.
HTTP Post 요청은 새로고침시 이전요청과 동일한 요청을 중복하게 되어 의도치 않은 결과를 낳을 수 있다는 것을 알게 되었고,
체스 말 이동(/move) 후에 redirect를 함으로써 새로운 요청으로 옮겨줄 수 있도록 개선해보았습니다.

또 이렇게 되다보니 Redirect시에 한글 파라미터가 깨지는 문제가 발생하더라구요...그래서 다음의 블로그글을 참고하여 파라미터에 대해서 인코딩한 값을 주도록 하였습니다.
Redirect 시 한글 파라미터가 깨지는 문제


favicon.ico

두번째로는 "favicon.ico" 를 읽어들이는 문제를 겪었습니다.
request에 대한 매핑을 단순히 "/{gameNaame}"과 같이 루트 바로 밑으로 설정해두니 "favicon.ico"를 읽어 제대로 된 요청에 대한 처리를 못하는 이슈를 마주하게 되었습니다. 이는 간단하게 루트 바로 밑에 두는 것이 아니라 uri를 "/game/"을 모든 매핑에서 기본으로 가지도록 함으로써 해결해볼 수 있었습니다.

RedirectAttributes

"/game/{gameName}/move" 에서 에러 발생시(ex. 잘못된 이동, 다른 팀의 말 이동) 에러 메시지를 출력해줘야하지만 "redirect:/game/{gameName}"을 수행하기 때문에 에러 메시지를 전달하지 못하는 문제가 있었고, RedirectAttributes와 함께 @RequestParam(value = "error", required = false) 를 사용하여 에러 메시지를 출력해줄 수 있도록 하였습니다.


질문

public ChessGame findByName(String gameName) {
        String sql = "select CHESSGAME.turn, CHESSGAME.game_name, PIECE.type, PIECE.team, PIECE.`rank`, PIECE.file from CHESSGAME, PIECE\n"
                + "where CHESSGAME.game_name = PIECE.game_name AND CHESSGAME.game_name = ?;";

        List<ChessGame> result = jdbcTemplate.query(connection -> {
            PreparedStatement preparedStatement = connection.prepareStatement(sql, ResultSet.TYPE_SCROLL_SENSITIVE,
                    ResultSet.CONCUR_UPDATABLE);
            preparedStatement.setString(1, gameName);
            return preparedStatement;
        }, chessGameRowMapper);

        if(result.isEmpty()) {
            return null;
        }

        return result.get(0);
    }

    private final RowMapper<ChessGame> chessGameRowMapper = (resultSet, rowNum) -> new ChessGame(
        getTurn(resultSet),
        resultSet.getString("game_name"),
        makeCells(resultSet)
    );

    private String getTurn(ResultSet resultSet) throws SQLException {
        resultSet.beforeFirst();
        resultSet.next();
        return resultSet.getString("turn");
    }

    private Map<Position, Piece> makeCells(ResultSet resultSet) throws SQLException {
        resultSet.beforeFirst();

        Map<Position, Piece> cells = new HashMap<>();

        while (resultSet.next()) {
            Position position = makePosition(resultSet);
            Piece piece = makePiece(resultSet);
            cells.put(position, piece);
        }

        return cells;
    }

ChessGameDao 에서 위와 같이 두 테이블을 함께 조회하고 있습니다. 그러다 보니 "game_name"과 "turn"은 하나의 데이터만 필요한데, 피스들의 데이터는 여러개이므로 ResultSet의 커서를 움직여서 각각 필요한 데이터를 조회해오고 있습니다.
그리고 ResultSet의 커서를 움직이기 위해서 PreparedStatement를 사용해야했고, 따라서 우리가 필요한 데이터는 ChessGame 하나인데, "jdbcTemplate.query()"의 결과로 List를 가져와야하는 불필요한 작업이 필요해졌습니다.

그래서 페어와 고민한 내용은 지금 현재 코드를 유지하는게 좋을지 혹은 CHESSGAME에 대한 조회, PIECE들에 대한 조회 총 2번의 쿼리를 이용하여 ChessGame을 만드는 것이 나은지를 고민하였습니다.

저는 하나의 데이터(ChessGame)을 위해서 분리된 2개의 쿼리를 작성해야한다 라는 점이 어색하게 느껴졌습니다.
turn, game_name 그리고 piece들의 정보 모두 ChessGame이라는 하나의 도메인을 위한 데이터들인데 이를 서로 독립적인 각각의 쿼리를 통해서 가져오고 이를 합친다라는 과정에서 여러가지 문제가 발생할 수 있지 않을까하는 생각도 들었습니다.
(아직 쿼리를 보내고 결과를 받는 등의 과정에서 여러가지 문제들을 만나보지 못해서..구체적으로 어떤 문제다라고 이야기는 못하지만..두 개의 쿼리 중 하나의 쿼리만 성공하는 경우에 발생하는 문제도 있을 수 있을 것 같습니다.)

혹시 이 내용에 대한 앨런의 의견을 들어볼 수 있을까요???😅


이외에도 "2단계"로 가기 위해서(체스 게임을 진행할 수 있는 방을 만들어서 동시에 여러 게임이 가능하도록 하기) 현재 Service단에서 가지고 있는 ChessGame(메모리)을 제거하여야겠다는 생각을 하였는데요! 우선 1단계의 목표가 "Spring 적용하기"인 만큼 리뷰어님의 리뷰를 받고 난 후 개선해보려고 합니다!!

아! 현재 제 컴퓨터의 Docker가 "8080" 포트를 쓰고 있어서...tomcat의 포트번호는 80포트를 사용하였습니다!
(8080포트가 자주 쓰이는 포트번호라 해결해보려고 했는데, 해결방법을 아직 찾지못하여서..😢 양해부탁드립니다..)

감사합니다!!🙏

체리픽으로 이전 미션의 체스판을 불러왔습니다.
남아 있는 매직넘버에 대해서 상수로 치환해주었습니다.
isPosition은 `!isPosition()` 형태로만 사용되고 있어 이를 validatePosition이라는 메소드로 변경하였습니다.
move, start 등의 문자열에 대해서 상수로 관리하도록 개선해보았습니다.
불필요한 team 필드를 제거하고, 상수에 대해서 private 선언으로 변경해주었습니다.
두 Position을 equals()로 비교하도록 조건문을 개선하였습니다.
chessGame의 메소드들을 하나씩 호출해 수행하는 것이 아닌 하나의 메시지를 보내는 것으로 수정하였습니다.
`source` Position 과 `target` Position 을 Map 으로 가지는 일급컬렉션 적용
게임 시작 전에 status 입력시 제한하도록 수정하였습니다.
두 Position을 equals()로 비교하도록 조건문을 개선하였습니다.
기물 움직임에 대해서 Direction의 방향 열거형 상수를 사용하도록 수정해보았습니다.
symbol 만드는 책임을 ChessGame으로 이동시켰습니다.
결과에 대한 책임을 가지는 Result 열거형 도입
getCells() 메소드에서 Map 반환시 unmodifiableMap을 추가해주었습니다.
불필요한 매개변수를 제거하였습니다.
각 기물의 기호를 나타내는 Symbol 열거형을 도입하였습니다.
웹 체스판 뷰 구현하였습니다.
체스판의 움직임을 구현하였습니다.
예외를 표시해줄 수 있도록 기능을 추가하였습니다.
드래그 앤 드롭으로 체스말을 이동시키도록 구현하였습니다.
이 과정을 통해서 DB에서 데이터를 가져오더라도 백팀과 흑팀의 위치가 뒤바뀌는 문제 해결
경로 구하는 방법을 변경하고, onclick() 이벤트시 폼을 생성해 넘기도록 수정하였습니다.
- ModelAndView -> Model을 사용
- post요청시 redirect하도록 수정
- error메세지를 RedirectAttributes에 담아 전달하도록 수정
체스말을 움직이면서 자동으로 저장되기 때문에 별도의 저장 버튼이 필요없어져 제거하였습니다.
'게임 종료' 버튼을 눌렀을 때 DB에 저장하도록 수정하였습니다.
update시 batchUpdate 이용하여 '피스'들을 저장
요구사항 목록을 점검하며 제대로 요구사항을 반영하였는지 확인하였습니다.
@hongbin-dev
Copy link

hongbin-dev commented Apr 25, 2022

ChessGameDao 에서 위와 같이 두 테이블을 함께 조회하고 있습니다. 그러다 보니 "game_name"과 "turn"은 하나의 데이터만 필요한데, 피스들의 데이터는 여러개이므로 ResultSet의 커서를 움직여서 각각 필요한 데이터를 조회해오고 있습니다.
그리고 ResultSet의 커서를 움직이기 위해서 PreparedStatement를 사용해야했고, 따라서 우리가 필요한 데이터는 ChessGame 하나인데, "jdbcTemplate.query()"의 결과로 List를 가져와야하는 불필요한 작업이 필요해졌습니다.

정리해보니 'chessgame와 피스들을 한번에 조회하기, 나눠서 조회하기 중에 고민중이다.' 로 이해하면될까요?

우선 전 비즈니스적으로 chessgame과 피스들이 한번에 조회되어도 어색하지 않는지를 확인하는편입니다.
피스들과 체스게임은 라이프사이클이 같다고 생각하는데요. 한번에 �조회되고 좋겠네요.

또 각각의 장/단점을 비교해보면 좋을 것 같은데요. 그러면 선택할 수 있는 폭이 좁혀질 것 같습니다.


이외에도 "2단계"로 가기 위해서(체스 게임을 진행할 수 있는 방을 만들어서 동시에 여러 게임이 가능하도록 하기) 현재 Service단에서 가지고 있는 ChessGame(메모리)을 제거하여야겠다는 생각을 하였는데요! 우선 1단계의 목표가 "Spring 적용하기"인 만큼 리뷰어님의 리뷰를 받고 난 후 개선해보려고 합니다!!

좋습니다! 리뷰에 적은 것처럼 Service내에 상태공유를 안하는방법으로 가면 좋을 것 같네요.

Copy link

@hongbin-dev hongbin-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 디우~
스프링 미션 잘 진행해주셨네요 💯
마이너한 코멘트 남겼으니 확인부탁드려요~

이만 머지해도 좋을 것 같네요! 다음단계에서 만나요~

public String move(@PathVariable String gameName,
@RequestParam("from") String from, @RequestParam("to") String to,
Model model, RedirectAttributes redirectAttributes) throws UnsupportedEncodingException {
String encodedParam = URLEncoder.encode(gameName, "UTF-8");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프레임워크에 만들어져 있는 상수가 있군요?
StandardCharsets.UTF_8 이 값으로 사용하면 좋을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java에서 StandardCharsets 클래스를 통해 "표준 문자 집합"에 대한 상수를 정의해주고 있는지 몰랐네요!!
개선해보겠습니다~_~


private final ChessGameDao chessGameDao;
private final PieceDao pieceDao;
private ChessGame chessGame;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코멘트 남겨주신 것 처럼, 여기 상태가 있네요..!
이 부분으로 여러명이 접속 했을 때 문제가 발생할 수 있어요. (상태를 공유해서)

다음 단계에서 이 부분을 제거하고 비즈니스를 풀어보면 좋겠습니다~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChessGame이라는 인스턴스 변수를 제거하고, 그 때 그때마다 DB에 쿼리를 날려서 데이터를 가져오고 처리를 하게끔 수정해보았습니다!

Comment on lines +26 to +28
private void rollback() {
chessGameDao.remove("test");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private으로 작성하면 동작 안할 것 같은데요. 한번 확인해보시겠어요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AfterEach (JUnit 5.0.1 API)
위 문서를 참고해보니 "AfterEach의 경우 must have a void return type, must not be private, and must not be static. 즉, 반환타입이 void이고, private이 반드시 아니어야하며 static 또한 불가능 하다는 내용이 있네요!! 제대로된 사용법을 모르고 사용했던 것 같습니다...🥲
개선해보겠습니다!

Comment on lines +25 to +27
public ChessGameDao(JdbcTemplate jdbcTemplate) {
this.jdbcTemplate = jdbcTemplate;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jdbcTemplate 빈 주입 좋네요 👍

@Repository
public class ChessGameDao {

private JdbcTemplate jdbcTemplate;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private JdbcTemplate jdbcTemplate;
private final JdbcTemplate jdbcTemplate;

런타임에 jdbctemplate이 변경되지 않도록 final을 붙이면 좋을 것 같습니다~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의존성 주입에 대해서 저랑 페어 둘 다 잘 모르는 상태였어서..필드 주입을 시도하다보니 final이 누락되었던 것 같아요..!!

필드 주입 대신 생성자 주입을 사용한 이유는 다음 블로그 글을 참고하였습니다!!
생성자 주입을 @Autowired를 사용하는 필드 주입보다 권장하는 하는 이유

@hongbin-dev hongbin-dev merged commit 8c244df into woowacourse:tco0427 Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants