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

[4기][3주차] Wordle 과제 제출 - John #14

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

jhsong2580
Copy link

ㅇ Pair 정보 : 스페셜님
ㅇ Pair Programing 사용 툴 : Code With Me

  • Code With Me를 통해 저(John)의 컴퓨터로 진행하였고, 스페셜님과 저의 Git Repository에 각각 두번 푸쉬하는 방법으로 진행하였습니다.
    • git remote add pair
    • git push origin main & git push pair main

ㅇ 느낀점

  • 페어프로그래밍을 진행하며 저의 말이 정말 빠르고 제가 생각한 정보를 전달할때 많이 누락하여 전달하는 것을 알게 되었습니다. 이번 과정을 통해 이 부분을 중점으로 생각하며 진행하였습니다.
  • 똑같은 기능을 구현하면서 서로의 의견을 논의하며 더욱 나은 결과를 도출해 낸것 같습니다.
  • 또한 서로 말하면서 하니 혼자할때보다 집중이 잘 되고 더욱 명확하게 목표점에 도달할수 있었던것 같습니다.

jhsong2580 and others added 30 commits March 24, 2023 20:08
Copy link

@dlwnsgus777 dlwnsgus777 left a comment

Choose a reason for hiding this comment

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

안녕하세요 John님!

아! 저 분은 참 개발을 좋아하시는구나

라는 생각이 들 정도로 개발을 좋아하시고 즐기시는 모습에 늘 자극받고 배우고 있습니다ㅎ

부족하지만 리뷰 한번 달아봤습니다!
개인적으로 부족한 점이 많아 같이 소통하면서 리뷰 과제 진행하면 좋을 것 같습니다!ㅎ
잘부탁드립니다~

Comment on lines 44 to 47
private GameMachine init(){
AnswerWord word = wordsGenerator.getTodayWord();
return new GameMachine(word, START_COIN);
}

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.

GameMachine

  • AnswerWord를 초기화 한다
  • 사용자의 입력에 대해 정답과 비교하여, 결과를 출력한다.

User

  • Coin을 가지고, Coin을 사용한다.

위 두가지로 분리를 하였습니다.

GameService가 위 두 Domain을 통해 게임을 진행합니다.


위처럼 설계 분리하는걸 의미하는게 맞으실까요 ? ?

Comment on lines 20 to 25
public GameHost(InputManager inputManagerProxy, ViewManager viewManager,
WordsGenerator wordsGenerator) {
this.inputManagerProxy = inputManagerProxy;
this.viewManager = viewManager;
this.wordsGenerator = wordsGenerator;
}

Choose a reason for hiding this comment

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

게임 호스트가 인풋과 뷰, 워드 제네레이터까지 가지고 있다면

추후 추가 사항이 생겨 게임 호스트의 코드를 수정하게 되면 다음의 이유가 될 것입니다.

  • 가지고 있는 코인 수정
  • 인풋 매니저를 다른 매니저로 변경
  • 뷰 매니저를 다른 매니저로 변경
  • 워드 제너레이터를 다른 제너레이터로 변경

GameHost 는 코인을 가지고 있는 역할과 게임 실행을 담당하는 역할 2가지를 겸하고 있다고 생각이 드는데

지금도 너무 좋은 코드이지만 역할을 분리해보는 것도 더 좋지 않을까 아쉬움이 듭니다ㅎ

Choose a reason for hiding this comment

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

코드 재사용성을 위해 InputManager , ViewManager , WordsGenerator 를 인터페이스로 관리해보는 건 어떨까요??

Copy link
Author

Choose a reason for hiding this comment

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

사이에 interface를 두어서 Class간 결합을 느슨하게 하라는 말씀이시군요!

InputManager, WordGenerator, ViewManager을 추상화 하였습니다.

String todayWord = wordList.get(wordIndex);
return new AnswerWord(todayWord);
} catch (RuntimeException e) {
throw new RuntimeException("단어를 셋팅할수 없습니다", e);

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.

:ㅇ 그렇네요 수정하겠습니다

private final boolean[] alphabetExistInfoStore = new boolean[26];

public AnswerWord(String word) {
super(word);

Choose a reason for hiding this comment

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

super 클래스인 Word 에서는 생성자에서

    private void validate(String word) {
        if (!matchesFiveSmallAlphabet(word)) {
            throw new IllegalArgumentException("user input require 5 small alphabet");
        }
    }

위와 같은 메서드를 호출하는데 단순히 StringUtils 라는 클래스의 static 메서드를 호출합니다.

전체적으로 Word 라는 클래스를 사용하는 이유는 추상화된 타입을 사용하기 위함으로 생각되어집니다.

생성자에서 메서드를 호출하는 클래스를 상속하면 객체 생성시 super에서 호출되는 메서드들을 제어하기 힘들어 지고 이 경우로 인해 뜻하지 않는 문제가 발생한다고 알고 있습니다.

ex) 누군가가 Word의 validate 메서드를 수정 -> Word 를 상속한 클래스에서 원하지 않은 동작을 할 수도 있음
-> 사이드 이펙트 발생

이 경우에 Word 를 클래스로 만들어 상속하기보다

interface Word {
 char[] getWordArray();
}

위와 같이 인터페이스로 사용해보는 건 어떨까요??
부족하지만... 제 개인적인 의견입니다ㅠ

Copy link
Author

Choose a reason for hiding this comment

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

AnswerWord가 Word를 상속받는 이유는 Word가 제공하는 검증 기능과 AnswerWord가 가지고있는 정답 추론 기능을 함께 사용하기 위함입니다.

그리고 말씀하신 것처럼 Word의 생성자에 validate()를 제거해야한다면...
Word에서 제공하는 단어 검증 기능을 Validator Class를 만들어서 분리를 해야할까요?

Choose a reason for hiding this comment

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

validate 메서드가 private 메서드라 괜찮아 보이긴하지만..
이 부분에 대해서는 많은 고민이 필요할 듯 합니다.
Composition 을 이용해 상속대신 AnswerWordword 를 포함하는 구조로 변경하는 방법도 있을 수 있고,
다른 다양한 방법이 있을 것 같습니다!

ㅠ경험이 부족해 어떤 방법이 최선의 방법인지는 잘 모르겠습니다ㅠ


public class AnswerWord extends Word {

private final boolean[] alphabetExistInfoStore = new boolean[26];

Choose a reason for hiding this comment

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

이렇게 정답 체크를 해결하셨군요ㅎㅎ
좋은 방법 배워갑니다ㅎ


import com.wodle.domain.Word;

public class InputMangerProxy extends InputManager {

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.

제가 생각한 Class별 기능은 아래와 같습니다.

  • InputManager : 사용자의 입력을 담당
  • ViewManager : 사용자에게 출력을 담당

이때 입력과 출력이 동시에 필요한 "사용자의 입력 기능"에 대해서 InputManager와 ViewManager을 동시에 어떻게 사용하면 좋을까를 많이 고민했던 것 같습니다. 그래서 아래 두가지를 고민하였는데요.

  1. InputManager, ViewManager을 주입받는 InputViewFacadeService
  2. Proxy Pattern으로 InputManager과 ViewManager을 분리

1번으로 하게 되면 현재 GameHost가 ViewManager과 InputViewFacadeService까지 주입받게 되더라구요.
그래서 2번으로 진행하여 InputManager을 주입받는 GameHost에 InputManagerProxy를 주입받게 하였습니다.

private final List<List<TileColor>> wordMatchResults;

public ViewManager() {
wordMatchResults = new LinkedList<>();

Choose a reason for hiding this comment

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

클래스를 보면 리스트를 addfor 문에 사용하시는데 new LinkedList<>(); 를 사용하신 이유가 있을지요??

Copy link
Author

Choose a reason for hiding this comment

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

해당 List는 단순 순차접근과 Tail에 데이터 추가만 진행하는 기능밖에 없어 LinkedList가 효율적이라고 생각하였습니다.


import java.util.regex.Pattern;

public class StringUtils {

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.

해당 클래스는 static method만 선언하려고 하였습니다.

private 생성자를 통해 의미를 좀더 명확히 해두겠습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

관련하여 좀더 검색해보니 이렇게 하게되면 GC 효율에도 좋지 않고 객체지향 위반을 많이 한다고 하네요!

말씀하신 것 처럼 Singleton으로 관리해보겠습니다.

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

class GameHostTest extends StaticMockingUtils {

Choose a reason for hiding this comment

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

테스트 코드는 프로덕트 코드와는 또다른 세계인 것 같습니다ㅎ
리뷰 하다가 큰 인사이트를 얻었네요ㅎ 감사합니다!

import java.util.stream.Stream;
import org.mockito.MockedStatic;

public abstract class StaticMockingUtils {

Choose a reason for hiding this comment

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

이런 테스트 방법도 있군요!!!
테스트 부분에 대해 더 공부가 필요함을 절실히 깨달았습니다ㅠㅠ
좋은 리뷰드리고 싶은데... 도움 받고 가네요ㅎㅎ

Copy link

@dlwnsgus777 dlwnsgus777 left a comment

Choose a reason for hiding this comment

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

안녕하세요 john님!
늦은 리뷰 죄송합니다...
또한 word 클래스 상속에 대해서는 정확한 답변을 드리지 못해 죄송합니다ㅠㅠ

Comment on lines +10 to +12
ViewManagerImpl viewManager = new ViewManagerImpl();
InputManagerImpl inputMangerProxy = new InputMangerProxy(viewManager);
WordGeneratorImpl wordGenerator = new WordGeneratorImpl();

Choose a reason for hiding this comment

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

위의 변수들의 타입이 구체화된 타입으로 선언되어 있네요!

구체적인 타입으로 변수를 선언하기 보다는 추상화한 인터페이스의 타입으로 변수를 선언하는게 재사용성과 다형성에 이점이 있다고 생각하는데 John님은 어떻게 생각하시나요??

private final boolean[] alphabetExistInfoStore = new boolean[26];

public AnswerWord(String word) {
super(word);

Choose a reason for hiding this comment

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

validate 메서드가 private 메서드라 괜찮아 보이긴하지만..
이 부분에 대해서는 많은 고민이 필요할 듯 합니다.
Composition 을 이용해 상속대신 AnswerWordword 를 포함하는 구조로 변경하는 방법도 있을 수 있고,
다른 다양한 방법이 있을 것 같습니다!

ㅠ경험이 부족해 어떤 방법이 최선의 방법인지는 잘 모르겠습니다ㅠ

Comment on lines +16 to +17
AnswerWord answerWord = new AnswerWord("answe");
gameMachine = new GameMachine(answerWord);

Choose a reason for hiding this comment

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

 AnswerWord answerWord = new AnswerWord("answe");
 gameMachine = new GameMachine(answerWord);

한 테스트에서 중복되어 만들어지는 객체라면 setUp을 통해 처리하는게 어떨까요??

@@ -6,13 +6,13 @@
import java.io.PrintStream;
import java.nio.charset.StandardCharsets;

public class SystemInOutUtils {
public interface SystemInOutUtils {

Choose a reason for hiding this comment

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

class에서 interface로 변환하신 이유가 있을까요??
궁금해서 질문드려봅니다!ㅎ

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