Skip to content

[1,2,3단계 - 체스] 코다(김윤정) 미션 제출합니다.#196

Merged
jaeyeonling merged 40 commits intowoowacourse:yjkswfrom
yjksw:step1
Mar 28, 2021
Merged

[1,2,3단계 - 체스] 코다(김윤정) 미션 제출합니다.#196
jaeyeonling merged 40 commits intowoowacourse:yjkswfrom
yjksw:step1

Conversation

@yjksw
Copy link
Copy Markdown

@yjksw yjksw commented Mar 22, 2021

안녕하세요:) 리뷰 요청 합니다!!

이번 체스 미션이 중복되는 기능도 많고, 도메인 모델이 굉장히 복잡하게 얽혀 있어서 구현하기가 굉장히 까다로웠어요.
각 기물을 움직이는 부분을 구현하는 것이 가장 까다로웠는데, 본래 기물이 움직일 때 검증해야 하는 로직들을 모두 MoveStrategy 인터페이스를 상속하는 구현체에서 다루었었는데, 그러다보니 넘겨지는 인자도 너무 많아져서 몇몇 로직을 보드에서 담당하고 각 기물들의 특수한 로직들만 MoveStrategy를 상속하는 구현체에 포함했습니다 ㅎㅎ

그렇게 하다 보니 보드의 코드가 다소 길어진 것 같아요.
현재 보드에서는 기물을 움직이는 기능, 왕이 죽었는지 확인하는 기능, 각 칸에 존재하는 기물 확인 기능을 가지고 있어요. 그런데 코드가 다소 길어서 어떻게 분리를 해야 할지 고민스러운데 어떻게 생각하시나요?
움직이는 기능을 따로 분리하더라도, 제대로 된 객체가 아니라, utility 와 같은 기능을 하는 객체가 될 것 같아서 분리하기 적절하지 않다는 생각이 들기도 했어요! 혹시 이 생각이 어떠한지 조언해 주시면 감사합니다 😊

두번째는 사용자 입력으로 들어오는 키워드를 기반으로 다음 실행 명령이 달라 지는 부분에 고민이 많았습니다!
현재 저희 코드는 컨트롤러에서 인풋으로 들어온 문자열을 Command enum 으로 보내서 분석하여 enum의 값으로 있는 함수형 인터페이스 함수를 호출합니다! 이 부분이 모두 컨트롤러에서 이루어지는데, Command 클래스가 domain에 포함 되어야 할 것 같기도 하고,, 그러자니 domain에서 다시 컨트롤러의 함수를 호출하는 격이 되니 어떻게 구현해야할지 고민이 되었어요! 이 부분에 대해서는 어떻게 생각하시나요?

긴 코드 보시느라 힘드실텐데 잘 부탁드립니다! 😆
감사합니다 ㅎㅎ

yjksw added 30 commits March 16, 2021 14:34
- 색깔에 따라 대문자, 소문자 출력 메소드 구현
- 말 이름 가져오는 메소드
- 말 색깔 비교 메소드
- Controller 와 View 연결
- 보드판 출력
- 거리 제한 없는 moveStrategy 들을 위한 추상 클래스 분리
Copy link
Copy Markdown

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

안녕하세요 코다!
체스 미션을 함께할 재연링입니다 :)
어려운 미션이라 힘드셨을텐데 잘 구현해주셨네요 👍
질문의 답변과 피드백 남겨두었으니 확인 부탁드릴게요!!

# java-chess
체스 게임 구현을 위한 저장소

## 1 단계 - 체스판 초기화
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

요구사항 작성 👍

public class ChessController {

public void run() {
OutputView.printInitMessage();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

게임 시작 시 플레이에 대한 설명이 부족하네요!
요구사항을 확인해주세요 :)

image

List<String> processedInput = Arrays.asList(command.split(" "));

game.move(processedInput.get(1), processedInput.get(2));
OutputView.printBoard(game);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

a-h, 1-8 위치에 대한 정보도 같이 출력해줘야 할 것 같아요!

image


public class InitializedBoard {

private static final Map<Position, Piece> initializedBoard = new HashMap<>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

지금은 Console로만 사용이 가능하여 하나의 게임만 진행할 수 있지만, 해당 서비스를 웹으로 공개하여 여러 유저가 플레이 한다면 지금의 구조에서 어떠한 문제가 발생할 수 있을지 고민해보아요

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

static 으로 선언되어 있는 필드를 여러 유저가 공유하게 되면 치명적인 문제가 발생하겠군요...!!
좋은 부분 알려주셔서 넘 감사합니다!
static이 아니라 InitializedBoard 객체를 생성한 후에 메소드를 통해서 보드를 공급받도록 하고, 생성한 보드를 외부로 반환할 때 new 로 새로운 객체를 생성했습니다!!
그리고 InitializedBoard 를 주입받는 Board 객체의 생성자에서 받을 때도 new 를 통해서 받도록 수정했는데 그럼 외부와의 연결고리가 끊어져서 여러 유저 사용시 발생하는 문제도 방지가 될 수 있을까요?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stateless한 특성을 살려 상태를 가지지 않도록 설계하는 것이 중요합니다
즉 요청마다 다른 Board를 사용할 수 있는 구조라면 여러 유저 사용가 요청했을 때 문제가 없는 구조가 될 수 있겠네요!

해당 내용은 4, 5 단계를 진행하면 느낄 수 있을거에요 :)

Comment on lines +14 to +15
private static final Map<String, Position> CACHE = new LinkedHashMap<>();
private XPosition xPosition;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
private static final Map<String, Position> CACHE = new LinkedHashMap<>();
private XPosition xPosition;
private static final Map<String, Position> CACHE = new LinkedHashMap<>();
private XPosition xPosition;

클래스 변수와 인스턴스 변수 사이에 공백을 추가하면 가독성이 상승합니다


@BeforeEach
void setUp() {
position = Position.of('a', 1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

setUp에서 공통적으로 초기화 할 작업을 선택하는 기준을 중복이 발생하는가?의 기준이 아닌 동일한 테스트 초기화(준비) 단계가 필요한가?라는 관점에서 본다면 좋을 것 같아요!
비슷한 맥락이긴 하지만 차이점이 분명이 존재한답니다!
위와 같은 맥락에서 봤을 때 해당 초기화는 공통된 초기화일까 고민해보아요

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

그런 맥락의 차이가 있군요~! 그럼 중복이 발생하는 경우에는 메서드로 분리하여 중복을 제거하고, 테스트 초기화 작업을 필요한 경우에는 setUp을 사용하는게 좋겠네요! 알려주셔서 감사합니다~!

import static spark.Spark.get;
import chess.controller.ChessController;

public class WebUIChessApplication {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

클래스 이름과 역할이 달라보이네요 @_@

private static final Piece VOID_PIECE = new Piece(PieceKind.VOID, PieceColor.VOID);

private final Map<Position, Piece> board;
private PieceColor deadKingColor;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

크게 죽은 왕의 색을 가지고 있는 것과 매 요청이 왔을 때 왕이 죽었는지 확인하는 방법 두 방법이 존재할 것 같은데요
두 방식엔 장/단점이 존재하고 생각에 맞게 트레이드 오프하면 되는 부분이긴 하지만 개인적으론 매번 확인하는 것이 더욱 좋다고 생각하는데 어떻게 생각하시나요?
관리해야하는 변수를 줄이고 아는 정보를 줄였을 때 어떠한 이점이 있을지 고민해보면 좋겠습니다 :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

확실히 매번 왕을 체크하는 것이 동기화 측면에서 더 안정적일 뿐 아니라 관리하는 변수가 줄어서 좋다는 생각이 드네요!!
감사합니다 :)


import java.util.Map;

public class Board {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Q. 현재 보드에서는 기물을 움직이는 기능, 왕이 죽었는지 확인하는 기능, 각 칸에 존재하는 기물 확인 기능을 가지고 있어요. 그런데 코드가 다소 길어서 어떻게 분리를 해야 할지 고민스러운데 어떻게 생각하시나요?
움직이는 기능을 따로 분리하더라도, 제대로 된 객체가 아니라, utility 와 같은 기능을 하는 객체가 될 것 같아서 분리하기 적절하지 않다는 생각이 들기도 했어요! 혹시 이 생각이 어떠한지 조언해 주시면 감사합니다 😊

A. 말씀하신 것 처럼 지금의 상태에서 분리는 오히려 가독성을 떨어트릴 수 있겠네요 ㅎㅎ
체스는 요구사항 자체가 복잡하기 때문에 코드도 어느정도 복잡할 수 없다는 것을 염두하면 좋을 것 같아요!
개인적으론 코드를 봤을 때 문제가 있어보인다! 라는 생각은 들지 않아 지금의 상태를 유지하셔도 괜찮을 것 같습니다 :)

import java.util.Arrays;
import java.util.function.BiConsumer;

public enum Command {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Q. 두번째는 사용자 입력으로 들어오는 키워드를 기반으로 다음 실행 명령이 달라 지는 부분에 고민이 많았습니다!
현재 저희 코드는 컨트롤러에서 인풋으로 들어온 문자열을 Command enum 으로 보내서 분석하여 enum의 값으로 있는 함수형 인터페이스 함수를 호출합니다! 이 부분이 모두 컨트롤러에서 이루어지는데, Command 클래스가 domain에 포함 되어야 할 것 같기도 하고,, 그러자니 domain에서 다시 컨트롤러의 함수를 호출하는 격이 되니 어떻게 구현해야할지 고민이 되었어요! 이 부분에 대해서는 어떻게 생각하시나요?

A. 단순하게 보면 if나 switch로 단순하게 처리가 가능하고 말씀하신 것 처럼 클래스로도 분리가 가능하며 지금처럼 처리도 가능합니다
클래스로 분리할 경우 Controller에서의 역할을 줄이고 Command 상태가 변화하는 방식(일종의 상태 패턴)으로 구현이 가능할 것 같은데요
개인적으로 복잡할 필요가 없는 부분이라 생각되어 지금과 같은 구조나 if, switch로 처리하는 것이 좋아보이네요 ㅎㅎ

@yjksw
Copy link
Copy Markdown
Author

yjksw commented Mar 27, 2021

재연링! 안녕하세요!!
정말 꼼꼼하고 깊은 인사이트가 있는 피드백 감사해요.
이번 체스미션이 굉장히 어려웠는데 재연링의 피드백을 받고 더 많이 공부하고 알아갈 수 있어서 어려운 미션을 제대로 짚고 넘어가서 어렵지만 정말 즐거운 시간이었습니다!!

특히 웹 환경에서 여러 유저가 사용하는 상황도 고민하라고 한 것과 불필요한 커뮤니케이션 비용을 줄이는 것, 필드 변수와 아는 정보를 줄여서 코드를 간결하고 동기화 측면에서 안정적이도록 한 것 정말 많은 것 배워가요!! 👍

나름 고민하며 리팩토링 해보았는데, 추가적인 리뷰가 있다면 부탁드립니다 ㅎㅎ 이번에 체스 미션이 콘솔로 테스트 해보기도 어렵고 그랬는데 테스트 코드로 테스트하면서 리팩토링을 진행하니까 확실히 편리한 것을 몸소 체험할 수 있었어요 !

구현하면서 들었던 몇가지 질문이 있습니다 !

  1. 현재 Position에 from 두개의 of 등등을 통해 많은 정적 팩토리 메소드가 존재해요! 수업 때 많은 생성자의 수는 클라이언트가 유연하게 객체를 핸들링 할 수 있는 이점을 주어서 많은 생성자가 있는 것이 좋은 클래스다 라고 했었어요. 정적 팩토리 메소드도 생성자와 비슷한 역할을 한다는 의미에서 여러 인자를 overloading 하여 유연성을 주는 것이 좋은 구현인지 재연링의 의견이 궁금합니다!

  2. 이번에 코드가 복잡하여 여러 클래스를 분리하다 보니 가끔 상태를 가지지 않는 클래스를 구현하게 되었어요. 상태(필드)를 가지는 클래스는 new 연산자를 통해 여러 객체를 생성하여 각기 다른 상태를 관리하는 것이 좋아보였어요. 그런데 상태를 가지지 않는다면 굳이 해당 클래스를 객체화 할 필요가 없다고 느껴지면서 행위를 하기 위한 클래스라는 생각이 들었어요! 그래서 메소드도 static으로 선언하여 바로 호출하는 것이 편리하게 느껴졌어요. 그런데 사실 객체지향에서 static은 지양해야하고 상태를 가지지 않는 클래스가 올바르게 구현된 클래스인지 의문이 생기네요! 재연링은 어떻게 생각하시나요?

꼼꼼한 리뷰 정말 감사합니다 !! 😊

@yjksw
Copy link
Copy Markdown
Author

yjksw commented Mar 27, 2021

[Java] EnumMap - 2

내용

  • EnumMap은 Enum 을 Key로 가지는 Map 이다.

  • EnumMap 선언

    EnumMap<DayOfWeek, String> activityMap = new EnumMap<<>(DayOfWeek.class);
  • EnumMap 사용시 성능 이점이 있다.

    • Enum을 Key로 가질 때 이미 가능한 모든 key 후보를 알고 있기 때문에 해시 맵핑(hash computation)이 빠르게 이루어져서 성능 향상이 있다.
    • 기존의 HashMap 같은 경우는 hash 충돌을 방지하기 위해 다소 복잡한 자료구조와 복잡한 저장 및 꺼내기 로직이 필요하다.

링크

[Java] BiConsumer - 4

내용

  • 현재 Java의 메소드는 값에 할당되거나, 함수의 인자로 넘겨질 수 없다. 이런 한계점을 극복하고 Java로 함수형 프로그래밍을 가능하게 하기 위해 함수형 인터페이스를 사용할 수 있다.
  • 함수형 인터페이스 중 BiConsumer<T, U>는 두개인 인자를 받고 반환값은 void 이다.
  • 단 하나의 메소드만 존재한다 → BiConsumer의 경우에는 accept() 이다.
  • 사용 예시 : 값으로 함수형 인터페이스 할당

링크

[디자인패턴] Command Design Pattern - 3

내용

  • 체스 미션에서 명령어(start, end, status, move) 등을 Command Pattern으로 추상화하려고 했으나, 사용자 입력이 민감하게 작용하므로 이번에 적용하지 못했다.

  • 행위 패턴의 일종으로 Command(실행해야 할 행위)를 인터페이스로 추상화해 객체화 하는 것.

  • 실행 기능이 변경 될 때 호출자(Invoker)의 코드를 수정하지 않고 변경(OCP 원칙 준수)할 수 있도록 함.

  • 작동 예시

    Invoker  Button Class에서 각각 TvOn  LampOn 기능을 구현하고 싶을 :
    
    -> Command 인터페이스에서 turnOn() 메서드를 가지고 있음.
    -> Concrete 구현체인 Tv와 Lamp를 구현하여 각각 다른 기능을 실행하도록 .(Receiver)
    -> Invoker인 Button에서 Command setter가 존재해서 특정 기능을 가지고 on()  때마다 해당 Command  실행함. 

링크

[객체지향] 인터페이스 추상화 - 4

내용

  • 전략패턴과 같은 방식으로 동작하도록 각 말에 대한 이동 전략을 MoveStrategy라는 인터페이스 구현을 하도록 했다.
  • 각 말이 MoveStrategy 를 value로 가지고 move() 시 각자의 전략에 따라서 움직이도록 했다.
  • 공통되는 부분은 BasicMoveStrategy 추상클래스에서 일부 구현해 중복을 제거했다.

링크

[Java] SingletonList - 1

내용

  • Collection 이면서 단 하나의 객체를 리스트에 포함하는 불변을 사용하고 싶을 때 Singleton list를 사용한다.
  • add/remove 와 같은 연산을 지원하지 않으므로 사용할 시, UnsupportedOperationException 을 발생시킨다.
  • Arrays.asList 도 마찬가지로 list에 add/remove 를 할 수 없는 immutable list 이다. 이때 단 하나의 요소만 가지고 있다면 singleton list 로 변환하는 것을 추천한다.

링크

[Java] Enum - 4

내용

  • An enum is a special "class" that represents a group of constants (unchangeable variables, like final variables). (출처: w3schools)
  • An  is a special data type that enables for a variable to be a set of predefined constants. The variable must be equal to one of the values that have been predefined for it. Common examples include compass directions (values of NORTH, SOUTH, EAST, and WEST) and the days of the week. (출처: oracle)
  • enum의 사용처는 상수와 같은 것들의 열거 이다.
  • enum을 사용하여 구현이 가능할 때가 있지만 지나치게 많은 비지니스를 담당하게 된다면 해당 enum에 대한 의도에 커뮤니케이션 비용 발생하게 된다.
  • 사용 의도가 동료 개발자에게 클리어하게 전달이 되어야 한다.

링크

Copy link
Copy Markdown

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

피드백 반영 잘 해주셨습니다 👍
질문의 답변 남겨두었으니 고민해보시고 다음 단계 진행해주세요 :)

import java.util.Arrays;
import java.util.List;

public class ChessController {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

질문의 답변은 해당 위치에 남겨두도록 하겠습니다 :)

Q1. 현재 Position에 from 두개의 of 등등을 통해 많은 정적 팩토리 메소드가 존재해요! 수업 때 많은 생성자의 수는 클라이언트가 유연하게 객체를 핸들링 할 수 있는 이점을 주어서 많은 생성자가 있는 것이 좋은 클래스다 라고 했었어요. 정적 팩토리 메소드도 생성자와 비슷한 역할을 한다는 의미에서 여러 인자를 overloading 하여 유연성을 주는 것이 좋은 구현인지 재연링의 의견이 궁금합니다!

A1. 맞습니다! 생성자가 많으면 좋다 -> 생성의 방법이 많으면 좋다 라는 관점에서 동일하다고 볼 수 있어요
생성자의 경우 네이밍을 통해 의미를 들어낼 수 없다는 단점이 존재하지만 정적 팩터리 메서드의 경우 네이밍을 통해 어떠한 생성자인지 나타낼 수 있습니다
동일한 맥락일 경우 가은 이름으로 overloading하시면 되고 맥락이 다를 경우 네이밍을 통해 의미를 전달하면 더욱 좋을 것 같아요 :)

Q2. 이번에 코드가 복잡하여 여러 클래스를 분리하다 보니 가끔 상태를 가지지 않는 클래스를 구현하게 되었어요. 상태(필드)를 가지는 클래스는 new 연산자를 통해 여러 객체를 생성하여 각기 다른 상태를 관리하는 것이 좋아보였어요. 그런데 상태를 가지지 않는다면 굳이 해당 클래스를 객체화 할 필요가 없다고 느껴지면서 행위를 하기 위한 클래스라는 생각이 들었어요! 그래서 메소드도 static으로 선언하여 바로 호출하는 것이 편리하게 느껴졌어요. 그런데 사실 객체지향에서 static은 지양해야하고 상태를 가지지 않는 클래스가 올바르게 구현된 클래스인지 의문이 생기네요! 재연링은 어떻게 생각하시나요?

A2. 객체지향에서 static을 지양해야 한다는 것은 동의합니다!
하지만 추상화 레벨에 따라 static을 사용하는 것이 오히려 깔끔한 코드를 작성하는데 도움이 될 수 있습니다
객체지향에서 static을 지양해야 한다고 말하는 측의 의견을 들어본다면 극객체지향의 성향을 가지는 것을 알 수 있습니다
예를 들어 네이밍에 -er을 사용하지 않는다던가 등 많은 제약조건이 있는데요
주장하는 것 처럼 모든 코드를 객체지향적으로 작성하는 것이 좋겠지만 저희에겐 한정된 시간이 존재하기 때문에
정해진 시간 내에서 적절한 트레이드 오프를 한 코드를 작성하는 것이 좋습니다!

지금 코드에서 일부분만 극객체지향적 코드가 존재한다고 한다면 오히려 기존 코드와 추상화 레벨이 맞지 않아 프로젝트 내에 괴리감이 생기는 코드가 발생할 수 있습니다 :)

@jaeyeonling jaeyeonling merged commit 504a22b into woowacourse:yjksw Mar 28, 2021
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.

2 participants