[4, 5단계 - 체스] 연로그(권시연) 미션 제출합니다. #366

yeon-06 · 2022-04-05T17:32:01Z

안녕하세요 스티치! 좋은 밤입니다!
여러가지 고민하면서 구현을 했는데 이에 대해 함께 이야기 나눠주시면 너무 감사할 것 같아요😄

추가 기능 구현

웹 UI 연동
- Spark 이용 (Javascript는 프론트 부분의 복잡도만 높아진다 생각해서 사용하지 않았습니다..😂)
DB 연동
방 번호를 통해 게임방에 입장 가능

고민해본 문제들

상태 패턴의 제거

상태와 뷰 분리하기 위해 불필요한 메서드 계속 늘어남
파라미터로 BiConsumer 등을 넘길 수는 있지만 이를 통해 View 메서드를 호출하는 경우 이것 역시 View와 완전히 분리된 것이 아니라고 생각
현재 특정 조건이 나타날 때까지 계속 request를 받기 위해 생성하였음
Web을 적용하게 된다면 한번의 request를 보내면 한번의 response를 무조건 받아야한다고 생각함

위와 같은 사유로 득보다 실이 많다고 판단하여 상태 패턴을 제거하게 되었습니다 T.T
다만 ChessGame은 View만 분리하고 코드를 유지했습니다.
기존의 Status, Move 등의 상태가 갖던 로직을 ChessGame 내부에 status(), move() 같은 메서드로 존재하게 만들었습니다.

Controller vs Service vs DAO

Controller: url과 실제 돌아가는 로직을 매핑시켜주는 역할
Service: 로직이 존재하는 위치. (DAO 실행 포함)
DAO: DB와 연동해 데이터를 받아옴

제 주관적 기준으로는 각자의 역할이 위와 같다는 생각을 하고 로직을 구현했습니다
혹시 잘못 이해하고 있는 부분이 있다면 정정 부탁드려요 🙂

DAO에 대해

지금은 한 쿼리문을 실행하는 것에 대해서만 처리가 되어있는데요
move의 경우에는 move 실행이 성공한다는 가정 하에 아래 쿼리가 실행됩니다.

to에 있는 piece 삭제하기
from에 있는 piece를 찾아 위치를 to로 바꾸기
현재 board의 turn 변경하기

위의 세 쿼리문에 대한 것을 한 DAO에 넣어야 할까요?
전 DAO의 원자성, 재사용성을 위해 분리해야한다고 생각했는데 셋 중 하나라도 쿼리문이 실패하면 모두 다 롤백되어야 하는건 아닌가...
롤백이 되어야한다면 어떤 식으로 할 수 있을지 궁금합니다.

DB 환경을 테스트 하기

웹 UI를 적용하는건 이미 1~3단계의 테스트코드를 조합하는 정도라 별도의 테스트코드를 작성해가며 진행할 필요가 없다고 생각했습니다.
다만 DB를 연동하는 부분에 대해서는 테스트 코드가 필요하다고 느꼈는데요
(원하는 데이터가 잘 select 되었는지, update 쿼리문을 통해 데이터 변경이 잘 일어났는지 등)
요 부분에 대해 테스트 코드를 짜기가 너무 어려워 막무가내로 짰습니다 😂

현재는 DB에 원하는 데이터가 잘 존재한다는 가정 하에 짜다보니 항상 true가 나올거란 기대를 하기 힘든데요
select를 해서 전후를 비교하자니 테스트 코드를 위해 select 관련 메서드를 만드는게 과연 좋은 방법인지 의문이 드네요ㅠㅠ

- controller: url과 로직 매핑 - service: 로직 실행 (이후에는 DB 연동하는 역할까지 함)

lxxjn0

연로그, 미션 코드 잘 봤습니다!!

웹 미션이 처음인데도 dto 사용, dao 인터페이스 활용 등 다양하게 좋은 구조를 만들어 주신 것 같아요!! 질문한 부분에 대해서는 중간 중간 코멘트에서 함께 언급해두었습니다!! 상태 패턴의 경우에는 제거된 현재의 코드가 더 직관적이고 이해가 잘 가서 더 좋았습니다!! 👍🏼

그리고 DB 테스트에 대해서는 코멘트에도 남겨두긴 했지만 외부 라이브러리를 사용하는 것과 동일하기 때문에 DB 동작에 대한 테스트를 굳이 작성하실 필요는 없습니다!! DB에서는 정상적인 결과를 반환한다고 가정하고 그에 따른 다른 테스트들을 보완하는 것이 더 좋아보여요!! :)

src/main/java/chess/controller/ChessController.java

lxxjn0 · 2022-04-06T15:47:46Z

src/main/java/chess/controller/ChessController.java

+
+    public void run() {
+        ChessGame chessGame = new ChessGame();
+        AtomicInteger boardId = new AtomicInteger();


boardId를 AtomicInteger로 관리하는 이유가 있을까요?? 해당 변수가 사용되는 곳을 보면 사실상 생성된 id를 반환하거나 또는 request로 받은 id를 전달하는 역할을 하고 있어요!! 동시성을 관리할 만한 부분들이 아닌 것으로 보이는데 어떤 이유로 AtomicInteger를 사용하기로 결정하신걸까요??

원래 int로 선언했었는데 int로 하니까 IntelliJ가 에러(동시성 이슈)를 띄우면서 AtomicInteger를 추천해주더라고요💦
동시성을 관리하려는 의도는 아니었으나 인텔리제이가 컴파일 에러였나 노란전구였나... 를 띄워서 추천대로 바꿨습니다!

AtomicInteger가 어떤 상황에서 주로 사용하는지 한번 살펴보면 좋을 것 같아요!! 현재 상황에서는 굳이 atomicInteger까지 사용할 필요는 없을 것 같아서 int로 처리해도 충분할 것 같습니다! :)

Variable used in lambda expression should be final or effectively final
라는 컴파일 오류가 떴네요! 람다에서 값을 변경하려고 발생한 문제였어요😂

지금처럼 람다로 구성되어있는게 편리하고 가독성이 좋다고 생각해서 그냥 두었습니다!
(int를 사용하기 위해 이 구조를 깰 필요는 없다고 생각했어요!)

그래도 적용 시도는 해봤는데 궁금증이 하나 더 발생했습니다 ^-T
Spark의 get, post의 두번째 인자는 Route라는 함수형 인터페이스를 받고 있는데요
함수형 인터페이스를 구현하기 위해서는 익명클래스나 람다 등을 이용하면 될 것 같아요
근데 해당 방법을 통해 final이 아닌 값을 int 값을 변경하는 방법이 있을까요!?

src/main/java/chess/controller/ChessController.java

src/main/java/chess/dto/MoveRequestDto.java

lxxjn0 · 2022-04-06T16:15:54Z

src/main/java/chess/dto/PieceDto.java

+    public PieceDto(Piece piece, Position position) {
+        this(piece.getTeam().name().toLowerCase(),
+                piece.getInfo().getType(),
+                position.getName());
+    }


this 생성자를 사용하기 위해서 this 내부에 로직들이 담긴 걸까요?? 아니면 객체의 필드가 아닌 값을 받아서 생성하는 경우에 정적 팩토리 메서드를 사용해보는 것은 어떨까요?? 코드가 좀 더 가독성 좋게 개선될 수 있을 것 같아요!!

값들을 받는 생성자가 원래 있었고 나중에 Piece랑 Position을 받는 생성자가 생겨났는데요
외부에서 Piece 값을 꺼내서 dto에 넣는 것보다는 dto에서 꺼내 쓰는게 더 깔끔해보일 것 같아서 선택했습니다!

다양한 방식으로 생성할 수 있게 다형성을 제공한다고 생각을 했습니다
이 경우 정적 팩토리 메서드를 사용해본다면 그 장점이 뭔지 여쭤볼 수 있을까요?
여태까지는 뭔가 캐싱(?)을 위해..? 라고 해야할까요,..
같은 인스턴스를 반환할 때나 사용했는데 dto에서 적용하는 경우 어떤 점이 좋은지 궁금해요!

정적 팩토리 메서드를 캐싱된 값에 대한 반환으로만 사용되는 것 이외에도 다양한 목적으로 사용하고 있어요!! (https://bit.ly/3NSxQUV)

저 같은 경우는 생성자를 사용할 때는 말 그대로 해당 객체가 가지는 필드들을 통해서 생성할 때만 생성자를 사용하고 그 외로 데이터 가공을 통해 생성하는 경우에는 정적 팩토리 메서드를 사용하곤 합니다!!
이건 순전히 저의 코딩 스타일이기 때문에 참고 정도만 해주셔도 될 것 같아요!!

그런데 해당 코드에서 정적 팩토리 메서드를 추천드린 이유는 this 생성자를 사용하기 위해서 코드가 작성되다 보니 너무 가독성이 떨어져 보여서 그랬습니다!! this 생성자를 사용할 경우 생성자에 넘겨줄 인자를 로컬 변수로 뽑지 못하기 때문에 이러한 코드가 된 것으로 보여요!! 순전히 가독성 개선을 위한 포인트로 정적 팩토리 메서드를 제안드리기도 했고 추가로 저의 개발 스타일에 대한 추천이기도 해서 말씀드려 보았습니다 :)

Suggested change

public PieceDto(Piece piece, Position position) {

this(piece.getTeam().name().toLowerCase(),

piece.getInfo().getType(),

position.getName());

}

public PieceDto(Piece piece, Position position) {

String team = piece.getTeam().name().toLowerCase();

String type = piece.getInfo().getType();

String position = position.getName();

return new PieceDto(team, type, position);

}

src/main/java/chess/service/ChessService.java

src/test/java/chess/dao/BoardDaoImplTest.java

lxxjn0 · 2022-04-06T16:24:15Z

src/main/java/chess/service/ChessService.java

+                .collect(Collectors.toUnmodifiableList());
+    }
+
+    public Map<String, Object> move(ChessGame chessGame, PieceDao pieceDao, BoardDao boardDao, int boardId,


move 명령어에서 실행되는 쿼리에 대해서 원자성을 유지하고 싶다고 하셨는데 JDBC Transaction이라는 키워드로 검색해보면 다양한 방법들을 확인해볼 수 있을 것 같아요!! 해당 방법을 참고해서 한번 개선해보는 것은 어떨까요??

적용하다가 한가지 고민이 생겨 일단 커밋에서 제외했습니다ㅜ.ㅠ

현재 BoardDao와 PieceDao가 나뉘어져있는데요
한 가지 기능에서 양쪽의 Dao를 다 호출해야 하고, 이 역시 함께 commit / rollback되도록 하고 싶다면 어떤 식으로 조합하는게 좋을까요?

DAO에서 SQLException을 throw하고 Service에서 처리해주기

BoardDao와 PieceDao를 합치기

위 두 가지로 생각해보았는데 둘 다 썩 마음에 들지는 않네요 ㅠㅠ

DAO를 나눴을 때 해당 부분에 대해서 따로 고민해보진 못했는데 저도 한번 방법을 고민해볼게요!! 어떤 좋은 방법이 없을까 생각해보겠습니다 😄

lxxjn0

연로그, 코드 리뷰 반영해주신 부분 잘 봤습니다!!

추가로 더 코멘트를 남겨보려고 했는데 제가 요청드린 부분에 대해서 충분히 코드가 잘 반영된 것 같아서 추가로 코멘트를 드릴 부분이 없네요.. ㅠㅠ
그래서 마지막 코멘트로 service 테스트를 요청드렸습니다!! service에서 dao에 대한 의존성이 기존에 비해서 개선된 코드가 어떤 장점을 가지고 있는지 한번 경험해보기 좋을 것 같아요!! :)

lxxjn0 · 2022-04-09T15:55:27Z

src/main/java/chess/service/ChessService.java

+    private final PieceDao pieceDao;
+    private final BoardDao boardDao;
+
+    public ChessService(PieceDao pieceDao, BoardDao boardDao) {
+        this.pieceDao = pieceDao;
+        this.boardDao = boardDao;
+    }


dao를 외부에서 받아옴으로써 훨씬 코드를 유연하게 사용할 수 있을 것 같아요!!! 이번에 service에 대한 테스트 코드를 한번 작성해보면 어떨까요?? 그러면 의존성을 외부로부터 주입받은 것에 대한 장점을 좀 더 확실하게 느낄 수 있을 것 같아요!! 👍🏼

lxxjn0

연로그, 리뷰 반영하느라 수고 많으셨습니다!! :)

서비스 테스트에서 사용할 dao 구현체를 따로 두어서 실제 dao에 의존하지 않고 테스트가 가능하게 되었네요!! 추가로 직접 구현한 테스트용 dao 구현체들도 내부적으로 동일하게 동작할 수 있도록 구체적으로 구현해 둔 부분이 인상적이었네요 :)

추가적으로 학습해볼만한 내용 피드백 남겨두었으니 한번 공부해보시면 좋을 것 같아요!! :)
레벨 1 진행하느라 수고 많으셨고 방학 푹 쉬시면서 다음 레벨 2에서 다시 만나뵙길 기대할게요!! 👍🏼 수고하셨어요

lxxjn0 · 2022-04-11T15:18:11Z

src/main/java/chess/service/BoardDaoTestImpl.java

+import chess.dao.BoardDao;
+import chess.domain.piece.Team;
+
+public class BoardDaoTestImpl implements BoardDao {


테스트에서 사용하기 위한 mock 객체 구현 good!!

lxxjn0 · 2022-04-11T15:26:37Z

src/main/java/chess/controller/ChessController.java

+    public void run(ChessService chessService) {
        ChessGame chessGame = new ChessGame();


제가 해당 부분은 체크를 하질 못했었네요 ㅠㅠ 현재 서비스에서는 ChessGame을 가지고 있진 않지만 모든 메서드에서 chessGame을 전달받고 있습니다!!

spark에서는 run 메서드를 실행시켜둔 상태로 Service가 계속 유지되기 때문에 사실상 Service에서 ChessGame을 상태로 가지고 있는 것과 동일하다고 생각합니다!! 그래서 만약 DB 업데이트와 ChessGame 변수의 상태가 달라지게 된다면 로직상에서 문제가 발생할 것 같아요!!

Service는 주로 무상태 객체로 사용합니다. 즉 상태를 가지면서 상태를 변경하는 것이 아니라 DB에서 상태를 조회해오고 service에서는 비즈니스 로직을 관리하며 가져온 데이터를 처리하는 역할을 주로 합니다!! 해당 부분은 한번 controller와 service, 그리고 무상태 객체라는 키워드로 한번 학습해보면 좋을 것 같아요!! :)

yeon-06 added 30 commits April 1, 2022 23:00

refactor: 상태 패턴 적용하지 않고 enum 활용

33d025b

feat: index 페이지 생성

d988d3f

refactor: 콘솔에서 View와 도메인 로직 분리

2bb21b2

feat: 초기화 DB 생성

bb68d2e

refactor: index 페이지 수정

9a7fe35

docs: 체스 기물 svg 파일 추가

182ed6d

feat: 체스판 화면 보일 html 생성

46efa81

refactor: Board를 ChessGame이 갖게함

e7d2e2c

refactor: 게임 시작 시 post -> get 요청

18ff091

refactor: 변수명 변경 board->value

d95c760

refactor: 웹 페이지 구성 요소 변경

b35e0bb

feat: /new로 접근 시 board 초기화

e2922b9

feat: move 구현

62f83d4

feat: end 구현

827b18a

fix: 새 게임 시작 시 이전 게임의 기록 남아있는 현상 수정

d6cf402

refactor: 잘못된 이동하는 경우 경고 메시지 출력

2332a00

feat: status 구현

b16937c

refactor: 메서드 분리

0d4c385

refactor: ChessGame에서 View 완전히 분리 (에러메시지 출력 포함)

884ee3b

feat: Board 테이블 DB 연동

6d7cf27

feat: Piece 테이블 DB 연동

98b91cb

refactor: controller, service 분리

a042ef8

- controller: url과 로직 매핑 - service: 로직 실행 (이후에는 DB 연동하는 역할까지 함)

refactor: Request에 관한 처리 controller에서 하도록

9c1492f

refactor: SQLException을 unchecked exception으로 변경

d3aaf69

feat: 체스 게임 id로 입장하는 기능 구현

b41f5f9

fix: 잘못된 외래키 관계 수정

10c71a5

refactor: 새 게임 생성하는 기능 DB 연동

a5c1a55

refactor: move 기능 DB 연동

6b7834a

fix: 체스판 로드 시 turn이 항상 white인 현상 수정

30779d6

refactor: 공통 검증 메서드 분리

6f3db6f

yeon-06 added 4 commits April 6, 2022 01:39

refactor: 불필요한 메서드 제거

f59f887

refactor: 하드 코딩 제거

5ab1492

refactor: factory 형식 변경

8c5e86a

refactor: 삭제할 piece 변경

30ed73c

yeon-06 changed the base branch from main to yeon-06 April 5, 2022 17:32

lxxjn0 requested changes Apr 6, 2022

View reviewed changes

yeon-06 added 7 commits April 8, 2022 21:50

refactor: Dao와 Service 분리

3bee942

refactor: 접근제어자 최소화

6b1569a

refactor: Controller에서 불필요한 필드 제거

b2d5187

refactor: request를 받는 생성자 생성

6071796

refactor: Map을 필드로 갖지 않도록 수정

3f3b37e

refactor: PieceDto에서 Piece로 변환하는 메서드 생성

f7877e9

style: MoveRequestDto -> MoveRequest 이름 변경

35b2b4d

lxxjn0 requested changes Apr 9, 2022

View reviewed changes

yeon-06 added 3 commits April 11, 2022 12:06

refactor: PieceDto에서 정적 팩토리 메서드 적용

3af6c48

test: ChessService에 대한 테스트 코드 추가

17c42a9

test: DB 테스트 삭제

3be86f1

lxxjn0 approved these changes Apr 11, 2022

View reviewed changes

lxxjn0 merged commit 94bf38f into woowacourse:yeon-06 Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4, 5단계 - 체스] 연로그(권시연) 미션 제출합니다. #366

[4, 5단계 - 체스] 연로그(권시연) 미션 제출합니다. #366

yeon-06 commented Apr 5, 2022

lxxjn0 left a comment

lxxjn0 Apr 6, 2022

yeon-06 Apr 7, 2022 •

edited

lxxjn0 Apr 9, 2022

yeon-06 Apr 11, 2022

lxxjn0 Apr 6, 2022

yeon-06 Apr 8, 2022 •

edited

lxxjn0 Apr 9, 2022

lxxjn0 Apr 6, 2022

yeon-06 Apr 8, 2022

lxxjn0 Apr 9, 2022

lxxjn0 left a comment

lxxjn0 Apr 9, 2022

lxxjn0 left a comment

lxxjn0 Apr 11, 2022

lxxjn0 Apr 11, 2022

		public void run(ChessService chessService) {
		ChessGame chessGame = new ChessGame();

[4, 5단계 - 체스] 연로그(권시연) 미션 제출합니다. #366

[4, 5단계 - 체스] 연로그(권시연) 미션 제출합니다. #366

Conversation

yeon-06 commented Apr 5, 2022

추가 기능 구현

고민해본 문제들

상태 패턴의 제거

Controller vs Service vs DAO

DAO에 대해

DB 환경을 테스트 하기

lxxjn0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeon-06 Apr 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeon-06 Apr 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxxjn0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxxjn0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeon-06 Apr 7, 2022 •

edited

yeon-06 Apr 8, 2022 •

edited