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

[스티치] 체스 스프링 실습 1단계 미션 제출입니다 #37

Merged
merged 9 commits into from
Apr 27, 2020
Merged

Conversation

lxxjn0
Copy link

@lxxjn0 lxxjn0 commented Apr 23, 2020

웹이 이번에 처음이라 부족한 부분이 많을 것 같습니다 ㅜㅡㅜ
잘 부탁드리겠습니다 :)

Copy link

@Ramen6315 Ramen6315 left a comment

Choose a reason for hiding this comment

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

안녕하세요 스티치!

오래 기다리게 해서 죄송합니다 ㅜㅜ !

정말 좋은 코드를 작성 해주셔서 리뷰 하는 동안 제가 오히려 배우는 느낌의 코드였어요!

리뷰 내용중에서 혹시나 이해가 안되거나 제 생각이 궁금하시다면 24시간 언제든 DM 보내주셔도 됩니다!.

추가적으로 말씀드리면 AND, OR에 대해서 말씀드리자면 제가 드린 의견처럼 하면 parameter가 4개가 나오는 경우도 있더라구요 ! 참고 부탁드리겠습니다!.

좋은 코드 감사합니다 ! Approve 할게요!


private String endChessGame(final Request request, final Response response) {
return renderEnd(chessService.endChessGame());
}

Choose a reason for hiding this comment

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

이 부분에서 run()메서드 안에 playChessGame, newChessGame, endChessGame 이렇게 호출이 되는데요!
저같은 경우에 는 run() 메서드 안에 호출된 메서드 순서대로 아래쪽에 위치 하게 합니다!
제방식으로 메서드 명으로만 정리 하자면

run()
playChessGame()
newChessGame()
endChessGame()
render()
renderEnd()
의 순서대로 정리 해줍니다!


public void validate(final Position position) {
Objects.requireNonNull(position, "체스 위치가 null입니다.");
}

Choose a reason for hiding this comment

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

validate 가 Class 안에서만 사용되는듯 합니다!
접근제어자 확인 부탁드릴께요!

public boolean isLeapableChessPieceOn(final Position position) {
validate(position);
return chessBoard.get(position).canLeap();
}

Choose a reason for hiding this comment

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

메서드 명에 대한 의견인데요!
return 에서 canLeap()과 통일하게 isLeapableChessPieceOn -> canLeapChessPieceOn 이런식으로 명명 해주시는건 어떤가요??

int chessRankGap = Math.abs(sourcePosition.calculateChessRankGapTo(targetPosition));

return (chessFileGap == 0) && (chessRankGap <= pawnMovableRange);
}

Choose a reason for hiding this comment

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

하드 코딩이 보이는데요 !

chessFileGap의 경우 말그대로 File의 거리 차이 값을 말하기 떄문에 직관적인 값도 괜찮을거같다는 의견을 comment 이전에 주셨는데 충분히 공감합니다!

제가 이부분에 대해 피드백 드리는 기준은 내가 아닌 다른 개발자가 이코드를 이어 받았고 체스의 규칙을 변경 할 수 있다고 했을때

0 -> private static final int NONE_FILE_GAP = 0

이런식으로 수정해줄수 있지 않을까? 하는 생각입니다!

int chessRankGap = Math.abs(sourcePosition.calculateChessRankGapTo(targetPosition));

return (chessFileGap == 1) && (chessRankGap == 1);
}

Choose a reason for hiding this comment

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

하드 코딩
내용은 먼저 드린 comment 와 같습니다.

다음 내용은 제가 생각한 변수명입니다.

CAN_CATCH_PAWN_FILE_GAP_VALUE, 
CAN_CATCH_PAWN_RANK_GAP_VALUE

final int chessRankGap = Math.abs(sourcePosition.calculateChessRankGapTo(targetPosition));

return chessFileGap <= 1 && chessRankGap <= 1;
}

Choose a reason for hiding this comment

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

하드 코딩
내용은 위 comment 와 같습니다.

다음은 제가 생각한 변수명입니다.

KING_MOVE_MAX_FILE_VALUE
KING_MOVE_MAX_RANK_VALUE

if (Objects.isNull(commandArguments) || commandArguments.isEmpty()) {
throw new IllegalArgumentException("유효한 체스 명령어가 아닙니다.");
}
}

Choose a reason for hiding this comment

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

if (Objects.isNull(commandArguments) || commandArguments.isEmpty()) {
        throw new IllegalArgumentException("유효한 체스 명령어가 아닙니다.");
}

이코드의 경우

if ('적절한 메서드명'(commandArguments)) {
	throw new IllegalArgumentException("유효한 체스 명령어가 아닙니다.");
}
private static boolean '적절한 메서드명'(List<String> commandArguments) {
		return Objects.isNull(commandArguments) || commandArguments.isEmpty();
}

이런식의 변경은 어떠신가요?


private boolean isNotExistOnAxis(final int chessFileGap, final int chessRankGap) {
return chessFileGap != 0 && chessRankGap != 0;
}

Choose a reason for hiding this comment

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

하드 코딩

내용은 위와 같고 제가 생각하는 변수명은

NONE_FILE_GAP
NONE_RANK_GAP
입니다.

final int chessRankGap = Math.abs(sourcePosition.calculateChessRankGapTo(targetPosition));

return (chessFileGap == 1) && (chessRankGap == 1);
}

Choose a reason for hiding this comment

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

하드 코딩

내용은 위와 같고 제가 생각하는 변수명은

CAN_CATCH_PAWN_FILE_GAP_VALUE
CAN_CATCH_PAWN_RANK_GAP_VALUE
입니다.

final int chessRankGap = Math.abs(sourcePosition.calculateChessRankGapTo(targetPosition));

return (chessFileGap == 0) && (chessRankGap <= pawnMovableRange);
}

Choose a reason for hiding this comment

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

하드 코딩

내용은 위와 같습니다.

제가 생각하는 변수명은
NONE_FILE_GAP
입니다.


private static String render(Map<String, Object> model, String templatePath) {
return new HandlebarsTemplateEngine().render(new ModelAndView(model, templatePath));
}

Choose a reason for hiding this comment

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

이 부분은 제가 리뷰 받은 내용인데요!`

HandlebarsTemplateEngine 객체를 반복해서 생성하는 것은 굉장히 비용이 큰 작업입니다.
재사용할 수 있도록 어떠한 장점이 있을지 고민해보아요.

충분한 고민 후 이펙티브 자바 아이템 6. 불필요한 객체 생성을 피하라 참고해주세요!

그래서 제 경우는

private static final HandlebarsTemplateEngine handlebarsTemplateEngine = new HandlebarsTemplateEngine();

이렇게 field생성후

    private static String render(Map<String, Object> model, String templatePath) {
        return handlebarsTemplateEngine.render(new ModelAndView(model, templatePath));
    }

이런식으로 수정했는데요 한번 재고 해주시기 바랍니다.

lxxjn0 added 4 commits April 26, 2020 14:18
- ConnectionManager를 DataSource로 명명 수정.
- CustomJdbcTemplate을 JdbcTemplate으로 명명 수정, 별칭 지정.
- MySqlConnectionManager를 MySqlDataSource로 명명 수정, 별칭 지정.
@woowahanCU woowahanCU merged commit 42d3810 into woowacourse:lxxjn0 Apr 27, 2020
gracefulBrown pushed a commit that referenced this pull request May 4, 2020
* [스티치] 체스 스프링 실습 1단계 미션 제출입니다 (#37)

* feat : 기존 Chess 코드 추가

* refactor : 기존 Chess 코드의 Application과 Controller 명명 수정

* refactor : JdbcTemplate 명명 수정 및 Spring Bean 등록

- 기존 JdbcTemplate 라이브러리와 충돌로 인한 명명 수정
- Dao, Service, ConnectionManager를 Spring Bean 등록

* feat : SpringChessController 추가

* feat : SpringChessController 구현

* refactor : controller 피드백 리팩토링

* refactor : database package 리팩토링

- ConnectionManager를 DataSource로 명명 수정.
- CustomJdbcTemplate을 JdbcTemplate으로 명명 수정, 별칭 지정.
- MySqlConnectionManager를 MySqlDataSource로 명명 수정, 별칭 지정.

* refactor : 접근 지정자 수정 및 상수 추출

* refactor : BlackPawnStrategy와 WhitePawnStrategy를 PawnStrategy로 추상화

* [스티치] 체스 스프링 실습 2단계 미션 제출입니다 (#87)

* feat : Spring Data JDBC 적용

- Spring Data JDBC 적용을 위한 GameRoomRepository 구현
- schema.sql 구현

* feat : GameHistory, GameRoom Entity 구현

* refactor : Spring Application에 불필요한 클래스 및 테스트 삭제

* refactor : 불필요한 util 패키지 및 클래스 삭제, GameHistory 클래스 내 CreatedTime 필드 삭제

Co-authored-by: Junyoung Lee <[email protected]>
gracefulBrown pushed a commit that referenced this pull request May 8, 2020
* [스티치] 체스 스프링 실습 1단계 미션 제출입니다 (#37)

* feat : 기존 Chess 코드 추가

* refactor : 기존 Chess 코드의 Application과 Controller 명명 수정

* refactor : JdbcTemplate 명명 수정 및 Spring Bean 등록

- 기존 JdbcTemplate 라이브러리와 충돌로 인한 명명 수정
- Dao, Service, ConnectionManager를 Spring Bean 등록

* feat : SpringChessController 추가

* feat : SpringChessController 구현

* refactor : controller 피드백 리팩토링

* refactor : database package 리팩토링

- ConnectionManager를 DataSource로 명명 수정.
- CustomJdbcTemplate을 JdbcTemplate으로 명명 수정, 별칭 지정.
- MySqlConnectionManager를 MySqlDataSource로 명명 수정, 별칭 지정.

* refactor : 접근 지정자 수정 및 상수 추출

* refactor : BlackPawnStrategy와 WhitePawnStrategy를 PawnStrategy로 추상화

* [스티치] 체스 스프링 실습 2단계 미션 제출입니다 (#87)

* feat : Spring Data JDBC 적용

- Spring Data JDBC 적용을 위한 GameRoomRepository 구현
- schema.sql 구현

* feat : GameHistory, GameRoom Entity 구현

* refactor : 게임방 생성과 새로운 방을 생성할 수 있도록 index.hbs 수정

* refactor : 종료된 게임 표시 및 왕이 잡히면 게임이 종료 되도록 수정

* refactor : ConsoleApplication 관련 패키지 및 클래스 삭제

* refactor : Service 흐름에 따라 메서드 순서 변경

* feat : DB에 GameRoom이 존재하지 않는 경우 예외 발생 기능 구현

* feat: MockMvc를 활용한 컨트롤러 테스트

* chore : 불필요한 todo 주석 제거

Co-authored-by: Junyoung Lee <[email protected]>
@lxxjn0 lxxjn0 deleted the step1 branch May 17, 2020 08:51
@lxxjn0 lxxjn0 restored the step1 branch June 27, 2020 19:35
@lxxjn0 lxxjn0 deleted the step1 branch June 29, 2020 08:07
@lxxjn0 lxxjn0 restored the step1 branch June 29, 2020 08:07
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.

3 participants