[2단계 - 사다리 게임] 채채(신채원) 미션 제출합니다. #226

chaewon121 · 2023-02-23T18:11:56Z

채채 미션 제출합니다
잘부탁드립니다!

aegis1920

안녕하세요 채채! 빙봉입니다 🙂

DM으로 답변이 오지 않아서 일단 현재 코드에 대해 피드백 드렸습니다. 다만 1단계에서 말했던 피드백이 반영되지 않은 부분(DM으로도 이야기한 피드백)도 있기에 참고해서 반영해주시면 좋을 것 같습니다. 코멘트 확인 부탁드려요!

질문은 언제나 환영이니 언제든 DM주세요~

aegis1920 · 2023-02-24T13:27:19Z

src/main/java/ladder/domain/ladder/Ladder.java

@@ -1,4 +1,4 @@
-package ladder.domain;
+package ladder.domain.ladder;


1단계에서도 이야기했던 Ladder의 validateRange 예외 처리 테스트를 추가해주세요! (링크)
LadderTest에 추가해주시면 될 것 같습니다,

사다리 높이에 대한 예외를 ladder 클래스에서 진행하였더니 inputView에 구현한 repeat() (잘못 입려받았을때 다시 입력받는 기능) 함수에 값을 넘겨주기가 어렵게 되어 사다리 높이 예외를 ladder클래스가 아닌 inputView에 같이 진행하는 것이 나을거 같다는 생각이 들었습니다.
inputView 에 대한 예외를 시행할때 같이 처리하겠습니다
혹시 더 좋은 방법이 있다면 알려주시면 감사하겠습니다!

수정 완료

aegis1920 · 2023-02-26T03:30:49Z

src/main/java/ladder/Application.java


 public class Application {
    public static void main(String[] args) {
+        Names names = InputView.repeat(() -> new Names(InputView.inputPeopleNames()));


Application main 메서드에 domain이 들어가있는데요. domain은 가급적 내부 로직(Controller 등..)에서 호출해야하지 않을까요?

Application은 어떤 역할일까요?

이번 미션에서 controller의 역할이 너무 길어져서 application에서 입력을 받고 사다리만들기, 사다리 타기, 출력 행위를 controller에서 시행했는데...controller에서 진행할 역할을 application에 나누었습니다. 말씀처럼 application에 domain이 있는건 어색해 보이네요... application에서는 어떤것이 이뤄져야 할지 더 고민해보고 리팩터링 해보겠습니다!

수정 완료

aegis1920 · 2023-02-26T03:34:43Z

src/main/java/ladder/controller/LadderController.java

+
+    private void viewGameResults(HashMap<String, String> gameResults, List<String> namesList) {
+        String name = InputView.repeat(() -> InputView.inputWantGameResults(namesList));
+        if (name.equals("all")) {


equals를 사용할 때 만약 name이 null이라면 어떻게 될까요?

이미 상수형태로 되어있는 "all"에서 equals를 호출해야 NPE에 안전하지 않을까요?

name은 이미 names 리스트를 생성할때 null값 예외처리를한 후 넘어온 값이여서 생각을 안해보았습니다.
하지만 비교적 불안정한 name 변수값 보다 상수all에서 호출을 하는 것이 안전해 보이네요! 수정하겠습니다!

수정 완료

aegis1920 · 2023-02-26T03:39:08Z

src/main/java/ladder/domain/ladder/Results.java

+    }
+
+    private List<String> split(String results, int peopleSize) {
+        validateNamesInputForm(results);


현재 Result를 INPUT_NAMES_PATTERN으로 매칭하게 되면 숫자는 입력 불가능합니다. 확인 부탁드려요.

맞네요...패턴 수정하겠습니다

aegis1920 · 2023-02-26T03:43:15Z

src/main/java/ladder/domain/GameResults.java

+    private final List<String> names;
+    private final List<String> results;


현재 요구사항을 보면 특정 이름에 대해서 특정 결과를 저장하는 방식이 필요할텐데요. 두 개의 리스트로 관리하기보단 Map이라는 자료구조를 활용하면 더 쉽게 요구사항을 구현할 수 있지 않을까요?

프린트 할때도 names와 results를 따로 출력해서 만약 map으로 바꾸게 된다면 GameResults 클래스 생성자에 names와 results를 받아 map을 생성하는 과정을 추가해야 하는데 굳이 그과정을 추가해서 map으로 변환할 필요가 있을까요!? 데이터 관리면에서 그 점이 더 좋아서 추천해 주시는 걸까요!?

일단 출력하는 면에서는 entrySet을 활용하면 따로 출력이 가능할 것 같고, 데이터 관리라는 면보다 역할을 생각했을 때 Map이 더 적절하다고 생각했어요. 2개의 리스트로 나눠서 관리한다면 각 name과 각 result가 종속적이기 때문에 관리하기가 불편할 겁니다. 만약 names에서 데이터 하나를 지운다면 result에서도 지워주는 게 필요하며 그에 따른 인덱스 관리도 필수적이겠죠. Map을 사용한다면 이런 관리도 필요없게 되고요 🙂

names를 지우게 되면 result도 지워줘야 하는 경우를 생각해 보지 못했네요.. 답변 감사합니다!

aegis1920 · 2023-02-26T03:44:24Z

src/main/java/ladder/domain/GameResults.java

+    public HashMap<String, String> calculateGameResults() {
+        HashMap<String, String> gameResults = new HashMap<>();


타입 자료구조를 구현체로 특정하기 보단 Map과 같은 인터페이스로 지정하면 좀 더 유연해지지 않을까요?

유연성 면에서 더 좋을것 같습니다! 수정하겠습니다

수정완료

aegis1920 · 2023-02-26T03:46:02Z

src/main/java/ladder/util/MoveDirection.java

+    public static int directMove(Bar leftBar, Bar rightBar) {
+        if (leftBar == Bar.TRUE) {
+            return -1;
+        }
+        if (rightBar == Bar.TRUE) {
+            return 1;
+        }
+        return 0;
+    }


명확해서 좋네요 👍 다만 util 패키지가 아닌 domain 패키지에 있어야하지 않을까요?

util 패키기를 도구 패키지 라고 인식하고 사용해 왔는데 util과 도메인 클래스를 나누는 것의 기준이 어렵습니다... 찾아봐도 명확한 자료가 보이지 않아서.. 이부분에 대해 여쭤볼 수 있을까요??

유틸 패키지란 어떤 의미일까요? java.util를 직접 찾아보고 어떤 것들이 있는지봐도 좋을 것 같아요. 유틸성 클래스를 모아둔 곳인데 예를 들어, java.util.Collections가 있겠네요.

현재 MoveDirection이란 클래스의 역할은 움직이는 방향을 알려주는 클래스라고 생각이 드는데요. Bar를 인자로 받고 있고 해당 Bar라는 클래스에 대해 의존하고 있다고도 볼 수 있을 것 같아요. 그리고 움직이는 방향을 알려준다는 역할을 생각했을 때도 bar에 대해 종속적이라고 생각합니다. 클래스의 역할로 보나 코드로 보나 유틸성이 있다고 보긴 어렵다고 판단이 들었어요. 🙂

클래스 의존성과 유틸성을 생각해보니 이해가 되었네요! 💯

aegis1920 · 2023-02-26T03:48:33Z

src/main/java/ladder/domain/ladder/Line.java


 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.IntStream;

 public class Line {
    private final List<Bar> bars;
-    private final int SUBTRACT_FIRST_BAR = -1;
+    private final int FIRST_BAR = 1;


상수로 활용된다면 static을 붙여주시면 좋지 않을까요?

상수는 모든 인스턴스에서 똑같이 써야되는 값으로 인스턴스가 만들어질 때마다 새로 메모리 공간을 생성하게 되면 비효율적이라는 것을 알게되었습니다.. 수정하겠습니다!

수정완료

aegis1920

안녕하세요 채채~ 빙봉입니다 🙂

포기하지 않고 끝까지 피드백 반영해주셔서 너무 감사해요. 💯💯💯
정말 고생했고 수고 많으셨어요. 아주 약간의 피드백 남겼는데ㅎㅎ 도움이 되셨으면 좋겠습니다. 사다리 미션은 여기서 머지할게요! 그리고 따로 궁금하셨던 부분도 추가로 코멘트 남겨뒀습니다! (링크1, 링크2)

나중에라도 궁금한 점이 있으시다면 언제든 DM 주세요! 블랙잭 미션도 파이팅입니다!!

aegis1920 · 2023-03-05T14:21:16Z

src/main/java/ladder/domain/GameResults.java

+import java.util.List;
+import java.util.Map;
+
+public class GameResults {


GameResults는 결과를 나타내는 아주 중요한 도메인이라 테스트가 있으면 좋았겠네요 🙂 비즈니스 로직에서 중요한 도메인이라면 테스트는 꼭 필요하다는 걸 잊지 말아주세요!

aegis1920 · 2023-03-05T14:22:12Z

src/main/java/ladder/domain/GameResults.java

+    private int directMove(Bar leftBar, Bar rightBar) {
+        if (leftBar == Bar.TRUE) {
+            return -1;
+        }
+        if (rightBar == Bar.TRUE) {
+            return 1;
+        }
+        return 0;
+    }


Bar 인자를 하나만 받아도 구현이 가능하지 않았을까요?

aegis1920 · 2023-03-05T14:22:59Z

src/main/java/ladder/domain/ladder/Line.java


 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.IntStream;

 public class Line {
    private final List<Bar> bars;
-    private final int SUBTRACT_FIRST_BAR = -1;
+    private static final int FIRST_BAR = 1;


가급적 상수가 가장 상단에 있는 것이 보통 컨벤션입니다!

aegis1920 · 2023-03-05T14:24:14Z

src/main/java/ladder/domain/people/Position.java

@@ -0,0 +1,17 @@
+package ladder.domain.people;
+
+public class Position {


포지션은 ladder 패키지가 더 어울리지 않을까요?

chaewon121 · 2023-03-05T15:10:02Z

너무너무 자세한 리뷰...정말 감사합니다!! 덕분에 정말 많이 배울 수 있었습니다. 💯

chaewon121 added 19 commits February 22, 2023 13:57

docs: 추가된 기능 추가

3623b92

refactor: Bar 클래스 enum으로 수정

f262543

refactor: 랜덤 생성하는 클래스 인자 변경

418b370

refactor: 파일 이동

1b2e6d3

feat: 움직임을 저장하는 객체 생성

ab7dc70

feat: 움직임을 명령하는 객체 생성

4eedf04

feat: 해당 인덱스의 bar를 반환하는 함수 추가

42e4b18

feat: 실행 결과를 입력받는 기능 구현

74ccd3e

feat: 사다리 타고 내려가 결과를 저장하는 기능 구현

2d4e27e

feat: 결과 보고싶은 사람 입력받는 기능 구현

c1a7745

feat: 결과출력 기능 구현 및 게임 구현 완료

597e33c

fix: 이름 예외처리 오류 수정

78931c2

fix: 결과 출력 오류 수정

3a267c8

refactor: 파일 이동

4e11d04

refactor: 문자열 분이하는 기능 함수 util파일로 이동

e5abe55

refactor: public -> private 으로 수정

a95c2b5

refactor: OutView 기능 분리

4172a6c

refactor: InputView의 예외 검증 로직 분리

e8d00bf

fix: 출력 오류 수정

e2b6e8c

chaewon121 changed the title ~~Step2~~ [2단계 - 사다리 게임] 채채(신채원) 미션 제출합니다. Feb 23, 2023

chaewon121 added 6 commits February 24, 2023 03:22

style: 라인 수정

3ee53ba

refactor: 함수 이름 수정

89eaae5

refactor: 컨트롤러에서 입력기능을 main으로 부여

496f16d

refactor: 함수 축소

b220c44

refactor: 함수 축소

10f3c09

refactor: 중복적으로 이용하는 함수 수정

b042e0b

aegis1920 requested changes Feb 26, 2023

View reviewed changes

chaewon121 added 3 commits February 27, 2023 15:53

refactor: 사다리 높이 예외처리 InputView로 이동

de29615

refactor: 상수 static 으로 변경

b83bc54

refactor: application -> controller에 포함

ff4ee44

chaewon121 added 11 commits February 27, 2023 15:54

refactor: 사다리 높이 예외처리 수정

612d0f8

refactor: directMove()함수 이동

bc6fcbc

fix: 예외처리 수정

b9ebb74

refactor: 패턴 상수화

39019ce

refactor: 함수명 변경

8d27ca0

refactor: 검증 위치 변경

92b8e10

refactor: view패키지 파일 이동

0d7b273

refactor: 컨트롤러 줄수를 위한 수정

39de1fe

refactor: 컨트롤러 줄수를 위한 수정

11c6cb8

refactor: 생성자 빈리스트 생성 수정

f424311

refactor: outView 로 넘겨주는 변수 수정

e3151aa

aegis1920 approved these changes Mar 5, 2023

View reviewed changes

aegis1920 merged commit 2c03eea into woowacourse:chaewon121 Mar 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 사다리 게임] 채채(신채원) 미션 제출합니다. #226

[2단계 - 사다리 게임] 채채(신채원) 미션 제출합니다. #226

chaewon121 commented Feb 23, 2023 •

edited

Loading

aegis1920 left a comment

aegis1920 Feb 24, 2023

chaewon121 Feb 26, 2023

chaewon121 Feb 27, 2023

aegis1920 Feb 26, 2023

chaewon121 Feb 26, 2023

chaewon121 Feb 27, 2023

aegis1920 Feb 26, 2023

chaewon121 Feb 26, 2023

chaewon121 Feb 26, 2023

aegis1920 Feb 26, 2023

chaewon121 Feb 26, 2023

aegis1920 Feb 26, 2023

chaewon121 Feb 27, 2023

aegis1920 Mar 2, 2023

chaewon121 Mar 4, 2023

aegis1920 Feb 26, 2023

chaewon121 Feb 27, 2023

chaewon121 Feb 27, 2023

aegis1920 Feb 26, 2023

chaewon121 Feb 26, 2023

aegis1920 Mar 2, 2023

chaewon121 Mar 4, 2023

aegis1920 Feb 26, 2023

chaewon121 Feb 26, 2023

chaewon121 Feb 26, 2023

aegis1920 left a comment

aegis1920 Mar 5, 2023

aegis1920 Mar 5, 2023

aegis1920 Mar 5, 2023

aegis1920 Mar 5, 2023

chaewon121 commented Mar 5, 2023

		@@ -1,4 +1,4 @@
		package ladder.domain;
		package ladder.domain.ladder;

		private final List<String> names;
		private final List<String> results;

		public HashMap<String, String> calculateGameResults() {
		HashMap<String, String> gameResults = new HashMap<>();

		@@ -0,0 +1,17 @@
		package ladder.domain.people;

		public class Position {

[2단계 - 사다리 게임] 채채(신채원) 미션 제출합니다. #226

[2단계 - 사다리 게임] 채채(신채원) 미션 제출합니다. #226

Conversation

chaewon121 commented Feb 23, 2023 • edited Loading

aegis1920 left a 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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aegis1920 left a 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

Choose a reason for hiding this comment

chaewon121 commented Mar 5, 2023

chaewon121 commented Feb 23, 2023 •

edited

Loading