[2단계 - 사다리 타기] 이든(최승준) 미션 제출합니다. #366

PgmJun · 2024-02-28T09:40:18Z

안녕하세요 제이!
리뷰받은 내용과 개인적으로 고민되었던 부분 생각하면서 코드를 열심히 개선시켜보았습니다!

이번에도 잘 부탁드립니다!

요청 🙌

제이가 말씀해주실 문제에 대한 중요도를 수치로서 인지하고, 이를 통해 투자하는 시간을 적절히 분산하여
작은 시간동안 최고의 효율을 내는 리뷰 환경을 만들어보고 싶습니다!

때문에 Pn 리뷰 룰 적용을 요청드려도 괜찮을 지 궁금합니다!

* [P1] : 꼭 반영해 주세요 (Request Changes) - 이슈가 발생하거나 취약점이 발견되는 케이스 등
* [P2] : 반영을 적극적으로 고려해 주시면 좋을 것 같아요 (Comment)
* [P3] : 이런 방법도 있을 것 같아요~ 등의 사소한 의견입니다 (Chore)
* [질문] : 이런 부분이 궁금합니다

절대 필수적으로 부탁드리는 부분이 아니며, 괜찮으시다면 부탁드리겠습니다 시간내어 리뷰해주셔서 감사합니다🙂

궁금해요 🤔

클래스 멤버 중 메서드의 배치 순서에 관한 고민

제이는 클래스의 멤버 중 메서드를 배치할 때, 어떤 기준을 가지고 배치하시는 지 궁금합니다.

오라클 문서를 살펴보니 가독성 좋은 순서대로 사용해라 라고 말하고 있기 때문에 저는 가독성을 위해 메서드는 아래 기준에 따라 정렬하고 있습니다.

1. 생성자에서 사용되는 메서드
    - 접근제어자 관련 없이 사용 순서대로
2. public static 메서드
    - 접근제어자 관련 없이 사용 순서대로
3. public 메서드
    - 접근제어자 관련 없이 사용 순서대로
4. getter 메서드

제이는 어떤 생각과 의견을 가지고 계신지 궁금해요!

상수화의 기준에 관한 고민

제이는 상수화를 자주 사용하시는 지, 그리고 어떤 기준으로 상수화를 사용하시는 지 궁금합니다.

저는 값 자체에서 의미가 드러나지 않는 경우에 상수화를 사용하는 편입니다.
예를 들어 "%s : %s" 이러한 포맷 관련 문자열이나, 1, 2와 같이 덩그러니 있을 때 어떤 의미인 지 알 수 없는 값이 존재하는 경우
상수화를 통해 값에 이름을 붙여주어 조금 더 명확하고 가독성 좋은 코드를 만들려고 합니다.

이런 생각에 대해 어떻게 생각하시는 지와 제이의 의견을 듣고 싶습니다!

또한 만들어진 상수를 외부에서 관리한다 라는 의견과 사용하는 곳에서 관리한다 라는 의견이 있는데
저는 사용하는 곳에서 관리해야 유지보수성과 가독성이 훨씬 증가한다고 생각합니다.
이 부분에 대해서도 의견이 궁금해요!

감사합니다:)

generate -> generateBridge

- 사용하는 이유가 따로 없기 때문에 RuntimeException 바로 상속받도록 변경

경# feat : 기능 추가

PlayerName이 Command에 포함되는 text인지 검증하기 위해 Command Enum 생성

outputView의 Map에서 bridgeMarker 관리

"실행 결과"와 같이 의미하는 바가 확실한 일반 문자열은 따로 상수화해주지 않아도 알아보기 편하지만 "%d %s" 등과 같은 format은 이름없이 존재하면 의미를 파악하기 어렵기 때문에 상수화를 통해 관리

JunHoPark93

안녕하세요 이든
2단계 구현하느라 고생많으셨어요~
전체적으로 확인주시고 의견 및 반영부탁드려요.
질문주신 부분은 코멘트 남겨두었어요.

JunHoPark93 · 2024-02-29T00:15:11Z

src/main/java/controller/LadderController.java

@@ -42,4 +56,56 @@ public PlayerNames createPlayerNames(String[] playerNamesInput) {
    private LadderHeight readLadderHeight() {
        return retry(() -> new LadderHeight(inputView.readLadderHeight()));
    }
+
+    private LadderResults readLadderResults(final int playerCount) {


제이는 클래스의 멤버 중 메서드를 배치할 때, 어떤 기준을 가지고 배치하시는 지 궁금합니다.

질문주신 부분 요기다가 남겨둘게요.

이든이 남겨주신 문서를 보면 the goal is to make reading and understanding code easier 이라고 되어있네요. 이 말에 공감하고 저는 사실 프로젝트에 따라 그때그때 다르게 하고있어요.
예를 들어, 어떤 프로젝트는 public메서드만 위로 올려두는곳도있고 어떤 프로젝트는 호출하는 메서드를 바로 밑에 두기도하고 다양한 경우가 존재합니다. 그 프로젝트안에서의 규칙을 따르는걸 1차로 생각하고있고, 그것이 맘에 들지 않는다면 전체를 다 고칠생각으로 규칙을 변경해야합니다. 다만 전체를 다 바꾸려면 투자대비 이득이 나와야하는데 실제로는 양이 방대하기도하고 이득이 많지도 않아서 그러기가 쉽지 않더라고요...

결론은 말씀해주신 가독성을 위한 순서로 배치하신건 잘하셨어요. 이 규칙을 모든 곳에 일관되게 적용이 되면 좋을거같습니다.

혹시 답변이 되었을까요?

제이는 클래스의 멤버 중 메서드를 배치할 때, 어떤 기준을 가지고 배치하시는 지 궁금합니다.

질문주신 부분 요기다가 남겨둘게요.

이든이 남겨주신 문서를 보면 the goal is to make reading and understanding code easier 이라고 되어있네요. 이 말에 공감하고 저는 사실 프로젝트에 따라 그때그때 다르게 하고있어요. 예를 들어, 어떤 프로젝트는 public메서드만 위로 올려두는곳도있고 어떤 프로젝트는 호출하는 메서드를 바로 밑에 두기도하고 다양한 경우가 존재합니다. 그 프로젝트안에서의 규칙을 따르는걸 1차로 생각하고있고, 그것이 맘에 들지 않는다면 전체를 다 고칠생각으로 규칙을 변경해야합니다. 다만 전체를 다 바꾸려면 투자대비 이득이 나와야하는데 실제로는 양이 방대하기도하고 이득이 많지도 않아서 그러기가 쉽지 않더라고요...

"프로젝트의 요구 사항에 맞게 따라가라"이군요!
좋은 인사이트가 된 것 같습니다.
꼼꼼히 살펴보면서 작은 실수라도 발생하지 않고 일관되게 적용할 수 있도록 노력해보겠습니다!

JunHoPark93 · 2024-02-29T00:19:05Z

src/main/java/controller/LadderController.java


-public class LadderController extends Controller {
+public class LadderController {
+    public static final int READ_LIMIT = 10;


제이는 상수화를 자주 사용하시는 지, 그리고 어떤 기준으로 상수화를 사용하시는 지 궁금합니다.

저는 값 자체에서 의미가 드러나지 않는 경우에 상수화를 사용하는 편입니다.
예를 들어 "%s : %s" 이러한 포맷 관련 문자열이나, 1, 2와 같이 덩그러니 있을 때 어떤 의미인 지 알 수 없는 값이 존재하는 경우
상수화를 통해 값에 이름을 붙여주어 조금 더 명확하고 가독성 좋은 코드를 만들려고 합니다.

이런 생각에 대해 어떻게 생각하시는 지와 제이의 의견을 듣고 싶습니다!

의미가 드러나지 않는 경우에 사용한다는거에 공감을 하고 더 나아가서 유지보수측면에서도 좋다고생각합니다. 예를 들어, 한 군데에서만 사용되고있는 숫자가 로직이 발전되면서 다른곳에서도 같은 숫자를 쓰게되는경우가 있을거에요. 요구사항 변경으로 해당 숫자를 변경해야할때 모두 찾아서 수정해주어야합니다. 실수로 한군데라도 빠트리게되면 버그가 생기겠죠.
그리고 문서화측면(self-documenting)에서도 좋습니다. 상수만보고 코드가 어떤 일들을 하는지 알 수 있습니다. 마지막으로 보안측면도있는데요, 값을 변경할 수 없으므로 런타임때 값이 의도치않게 바뀌는걸 막아줍니다.

또한 만들어진 상수를 외부에서 관리한다 라는 의견과 사용하는 곳에서 관리한다 라는 의견이 있는데
저는 사용하는 곳에서 관리해야 유지보수성과 가독성이 훨씬 증가한다고 생각합니다.
이 부분에 대해서도 의견이 궁금해요!

상황에 따라 다르다고생각해요. 1차적으로는 저도 사용하는곳에서 관리를 해야 관리가 편하다고생각하는데요, 다른곳에서도 그 상수를 참조할경우에는 공통으로 만드는게 낫겟죠. 다만 필요해지는 순간에 공통으로 정의합니다.

필요해지는 순간에 공통으로 라는 말에 공감합니다! 좋은 의견 감사합니다.

JunHoPark93 · 2024-02-29T00:21:37Z

src/main/java/controller/LadderController.java

+    private void findPlayerResult(final Ladder ladder) {
+        String playerName;
+        while (!(playerName = inputView.readPlayerNameForGetResult()).equals(Command.FINISH.getText())) {


아무값을 담지 않고 초기화를 하셨는데 의도하신 동작일까요?

playerName이 초기화 되지 않으면 컴파일 시점에서
variable playerName might not have been initialized 에러가 발생해서 문제로 이어질 확률이 적을 것이라고 생각했습니다.
그래도 NPE 등과 같은 잠재적인 에러 발생 가능성을 염두해서 Blank로라도 초기화 해두는 편이 괜찮을까요??

private void findPlayerResult(final Ladder ladder) { String playerName = inputView.readPlayerNameForGetResult(); while (!playerName.equals(Command.FINISH.getText())) { outputView.printPlayerLadderResult(getPlayerLadderResult(ladder, playerName)); playerName = inputView.readPlayerNameForGetResult(); } }

초기화를 먼저 하고 동작하게 하려면 간단한 방법은 이런 방법도 있을 것 같습니다!

아래 방법이 좀 더 낫네요!
첫번째 하신방식에서는 null pointer 예외발생가능성과 초기화되었는지에 대한 추적이 필요하기에 가독성이 떨어질수있어요

JunHoPark93 · 2024-02-29T00:22:43Z

src/main/java/domain/ladder/Floor.java

+    public static final String SEQUENCE_ERROR_MESSAGE = "한 층 내에서 사디리의 다리는 연속될 수 없습니다";
+    private final List<LadderBridge> bridges;


개행하나 있으면 좋을거같아요

확인하였습니다 감사합니다!

JunHoPark93 · 2024-02-29T00:23:08Z

src/main/java/domain/ladder/Floor.java

+    public BridgeDirection getBridgeAroundAt(final int playerPosition) {
+        if (hasLeftBridge(playerPosition)) {
+            return BridgeDirection.LEFT;
+        } else if (hasRightBridge(playerPosition)) {


요구사항중에 else를 사용하지 않는다는 조건이 있는데 어떻게 개선하면 좋을까요?

ealry return 방식 적용을 통해 해결해보겠습니다!

public BridgeDirection getBridgeAroundAt(final int playerPosition) { if (hasLeftBridge(playerPosition)) { return BridgeDirection.LEFT; } if (hasRightBridge(playerPosition)) { return BridgeDirection.RIGHT; } return BridgeDirection.NONE; }

JunHoPark93 · 2024-02-29T00:24:00Z

src/main/java/domain/ladder/Floor.java

+import java.util.List;
+
+public class Floor {
+    public static final String SEQUENCE_ERROR_MESSAGE = "한 층 내에서 사디리의 다리는 연속될 수 없습니다";


상수이름이 에러메세지를 잘 나타내는 이름이 맞을까요? 정답은 없지만 의미가 잘 드러나게 고민해보면 어떨까요?

+에러메세지에 오타가있네요

상수이름이 에러메세지를 잘 나타내는 이름이 맞을까요? 정답은 없지만 의미가 잘 드러나게 고민해보면 어떨까요?

+에러메세지에 오타가있네요

연속된 사다리의 다리에 대한 오류이니까 CONTINUOUS_BRIDGE_ERROR_MESSAGE 정도로 사용해보겠습니다.
의미가 잘 드러나지 않고 있는 것 같네요..
저도 작성하면서 고민이 있었는데 앞으로 고민있던 부분은 TODO로 체크해두고 다시 한 번 살펴보는 습관을 들여야겠습니다

오타 체크 감사합니다!

JunHoPark93 · 2024-02-29T00:24:38Z

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

+public class Ladder {
+    private final List<Floor> floors;
+    private final PlayerNames playerNames;
+    private final LadderResults ladderResults;


Ladder라는 도메인에 꼭 필요한 인스턴스변수들이 무엇일지 고민해보면 어떨까요? 너무 많은 책임을 들고있는건 아닌지 생각해봐도 좋습니다

Ladder라는 도메인에 꼭 필요한 인스턴스변수들이 무엇일지 고민해보면 어떨까요? 너무 많은 책임을 들고있는건 아닌지 생각해봐도 좋습니다

음... 생각해보니 이렇게 설계하면 같은 사다리에 다른 사람들과, 다른 결과들을 넣고싶다면 새로운 Ladder 객체를 생성해야할 것 같네요..
굳이 Ladder가 PlayerNames와 LadderResults를 알고 있어야한다는 책임을 주는 것보다
사다리 결과를 얻는 match 메서드에서 메서드 인자로 필요할 때 받아서 사용하는 것이 더 적합하다고 생각이 드네요.
한번 변경해보겠습니다!

JunHoPark93 · 2024-02-29T00:26:24Z

src/main/java/domain/player/PlayerNames.java

+    public int getIndexOfName(String name) {
+        for (int i = 0; i < values.size(); i++) {
+            if(values.get(i).getValue().equals(name)) {
+                return i;
+            }
+        }
+        throw new NotFoundException(NOT_FOUND_ERROR_MESSAGE);
+    }


컨벤션 확인해주시고 depth를 1까지만 허용한다는 요구사항도 있어요. 전체적으로 요구사항들 다시 살펴봐주시면 좋을거같아요

컨벤션 확인하겠습니다! 돌아가는 쓰레기를 먼저 만들고 리팩토링를 하다 보니 빼먹는 부분이 조금씩 생기네요.. 더 신경써보겠습니다!

현재와 같이 설계하면 같은 사다리에 다른 사람들과, 다른 결과들을 넣고싶다면 새로운 Ladder 객체를 생성해야 함 때문에 굳이 Ladder가 PlayerNames와 LadderResults를 알고 있어야한다는 책임을 주는 것보다 사다리 결과를 얻는 match 메서드에서 메서드 인자로 필요할 때 받아서 사용하는 것이 더 적합하다고 생각

JunHoPark93

안녕하세요 이든~
피드백 반영하시느라 고생많으셨습니다.
질문주신 부분 코멘트 남겨두었는데 추가적으로 더 궁금한부분있으면 dm주세요!
다음 미션도 화이팅입니다

JunHoPark93 · 2024-03-01T17:13:26Z

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

+import java.util.Map;
+
+public class Ladder {
+    private final List<Floor> floors;


인스턴스변수를 필요한 부분들만 남겨주셨군요 👍🏼

PgmJun added 30 commits February 28, 2024 17:22

refactor : 클래스명이 의미를 더 잘 표현하도록 수정

65cc025

refactor : 오라클 자바 컨벤션에 맞도록 클래스 멤버 위치 조정

7226a19

refactor : 중복되는 네이밍 변경

d8cbe46

generate -> generateBridge

refactor : BridgeGeneratorStub 클래스, NPE 문제 방지

9ab2f29

remove : 입력 테스트 제거

8cc519d

docs : Step2 기능 구현 문서 추가

b46f1d5

feat : 사다리 결과 출력 기능 구현

94292ee

refactor : 참가자 이름 입력 시, 구분자가 가장 마지막에 오는 경우 검증하도록 수정

4c13691

refactor : 패키지 구조 변경

d52cf71

feat : 사다리 실행 결과 확인 기능 구현

55d9d3d

refactor : CustomException 제거

585db12

- 사용하는 이유가 따로 없기 때문에 RuntimeException 바로 상속받도록 변경

refactor : 예외 메시지는 발생시키는 곳에서 관리하도록 변경

fef4282

경# feat : 기능 추가

refactor : 상수 네이밍 깔끔하게 변경

c021e37

refactor : 중복되는 bridge 랜덤 생성 로직 메서드 분리

fa2fd59

remove : 의미없는 상수 제거

0f8e40f

feat : 사용할 수 없는 이름 추가

e44fd39

refactor : 재입력 메서드 네이밍 동일하게 변경(오버로딩)

027deda

refactor : 커맨드 관련 상수 열거 클래스로 분리

62b6580

PlayerName이 Command에 포함되는 text인지 검증하기 위해 Command Enum 생성

refactor : 필드명으로 value 사용

a61097e

refactor : 불필요한 확장성을 위해 만든 RetryableController 삭제

c22e650

refactor : InputView에서만 사용되는 상수에 private 접근 제어 설정

6e8f2cf

refactor : 코드라인 정리

c38d1bd

refactor : 코드 컨벤션에 맞춰 클래스 멤버 위치 조정

3dfe45e

refactor : ui 로직에 의존하던 LadderBridge enum 코드 제거

17f859e

outputView의 Map에서 bridgeMarker 관리

docs : 사다리 종료 커멘드 관련 문서 수정

bca81d8

remove : LadderBridge getter 삭제

9543acc

refactor : 오라클 컨벤션에 맞게 멤버 위치 변경

65a578c

refactor : 테스트 클래스 위치, 네이밍 수정

ea3e039

refactor : 테스트 내부 클래스로 분리

04f71c7

PgmJun added 3 commits February 28, 2024 18:25

refactor : 중복 로직 메서드 분리

48c6982

refactor : final 키워드 사용해서 재할당 불가능하게 수정

5c2a355

refactor : final 키워드 사용해서 재할당 불가능하게 수정

dc65b6e

JunHoPark93 requested changes Feb 29, 2024

View reviewed changes

PgmJun added 2 commits February 29, 2024 09:58

refactor : 직관적인 에러 메시지 네이밍 사용

7b9f32b

refactor : ealry return 적용하여 else 키워드 제거

4359d9d

PgmJun changed the title ~~[Step2 사다리 타기] 이든(최승준) 미션 제출~~ [2단계 - 사다리 타기] 이든(최승준) 미션 제출합니다. Feb 29, 2024

PgmJun added 5 commits March 1, 2024 00:10

refactor : 한번에 index, name 조회해오도록 변경

f96ca82

refactor : 클래스 변수, 인스턴스 변수 구분

9b281d0

refactor : n번 조회 방식에서 단일 조회 방식으로 변경

1ba0367

refactor : OS 별로 다른 이스케이프 문자 고려하여 코드 수정

9670383

JunHoPark93 approved these changes Mar 1, 2024

View reviewed changes

JunHoPark93 reviewed Mar 1, 2024

View reviewed changes

JunHoPark93 merged commit 2073642 into woowacourse:pgmjun Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 사다리 타기] 이든(최승준) 미션 제출합니다. #366

[2단계 - 사다리 타기] 이든(최승준) 미션 제출합니다. #366

PgmJun commented Feb 28, 2024 •

edited

Loading

JunHoPark93 left a comment

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024 •

edited

Loading

JunHoPark93 Feb 29, 2024

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024 •

edited

Loading

PgmJun Feb 29, 2024 •

edited

Loading

JunHoPark93 Mar 1, 2024

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024 •

edited

Loading

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024

PgmJun Feb 29, 2024

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024 •

edited

Loading

JunHoPark93 Feb 29, 2024

PgmJun Feb 29, 2024

JunHoPark93 left a comment

JunHoPark93 Mar 1, 2024

		public static final String SEQUENCE_ERROR_MESSAGE = "한 층 내에서 사디리의 다리는 연속될 수 없습니다";
		private final List<LadderBridge> bridges;

[2단계 - 사다리 타기] 이든(최승준) 미션 제출합니다. #366

[2단계 - 사다리 타기] 이든(최승준) 미션 제출합니다. #366

Conversation

PgmJun commented Feb 28, 2024 • edited Loading

요청 🙌

궁금해요 🤔

클래스 멤버 중 메서드의 배치 순서에 관한 고민

상수화의 기준에 관한 고민

JunHoPark93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgmJun Feb 29, 2024 • edited Loading

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

PgmJun Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

PgmJun Feb 29, 2024 • edited Loading

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

PgmJun Feb 29, 2024 • edited Loading

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

PgmJun Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunHoPark93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgmJun commented Feb 28, 2024 •

edited

Loading

PgmJun Feb 29, 2024 •

edited

Loading

PgmJun Feb 29, 2024 •

edited

Loading

PgmJun Feb 29, 2024 •

edited

Loading

PgmJun Feb 29, 2024 •

edited

Loading

PgmJun Feb 29, 2024 •

edited

Loading