[페어 매칭 - 스터디] 조승현 미션 제출합니다 :) #147

BackFoxx · 2022-12-04T08:36:31Z

우테코 커뮤니티에서 진행한 최종 코딩 테스트 준비 스터디에서 만든 프로젝트입니다.

… 기능 구현

wugawuga

5시간동안 이정도 구현을 하셨다니 놀라고 또 놀라네요,,,,,
코드가 제가 이해하기엔 너무 높은 수준이어서 클론하고 한시간 넘게 코드를 봤던 것 같아요!!
제 짧은 식견이지만 이정도 리뷰밖에 하지못해 죄송합니다 ㅠㅠ 정말 수고 많으셨어요!!!

wugawuga · 2022-12-05T06:17:34Z

src/main/java/pairmatching/controller/ReadingCrewsFileController.java

+        File file = new File(fileDirectory);
+        FileReader fileReader = new FileReader(file);
+        BufferedReader bufferedReader = new BufferedReader(fileReader);
+        readLine(crewNames, bufferedReader);
+        bufferedReader.close();


파일리더를 사용하는 것도 있다는 걸 배워서 감사합니다!

wugawuga · 2022-12-05T06:21:00Z

src/main/java/pairmatching/inputview/SelectingMissionInputView.java

+    protected static String readInput() {
+        return Console.readLine();
+    }


GettingFeatureCommandInputView 와 같이 readInput 을 protected 로 지정해 주셨는데
나중에 이 클래스를 상속해서 기능 확장을 예측하신 것 같은데 어떤 경우인지 여쭈어 봐도 될까요?
두 클래스다 똑같은 함수를 protected 지정했는데 인터페이스 쪽에서 디폴트로 만들어 놓는건 어떻게 생각하시나요

제가 2~4주차 프로젝트를 작성할 때 Console.readLine() 을 사용하는 readInput 메서드를 protected 접근자로 분리했던 습관이 남아 생긴 코드에요!

테스트코드에서 InputView를 사용해야 하면
MockInputView라는 이름으로 InputView를 상속한 테스트 전용 클래스를 만들어요.
그 후에 protected 접근자인 readInput 메서드를 오버라이딩해서 외부에서 List 등을 주입하여
테스트용 가짜 입력값을 사용할 수 있도록 개조를 해서 사용했었습니다.
아래에 예시를 남겨둘게요!

public class LottoInputView { public String getInput() { String input = readInput(); // ... 입력을 이용한 무언가 실행 } protected String readInput() { return Console.readLine(); } <- protected 접근자인 메서드는 자식 클래스에서 오버라이딩이 가능하다. }

public class MockInputView extends LottoInputView { private String mockValue; public MockInputView(String mockValue) { this.mockValue = mockValue; } @Override protected String readInput() { return mockValue; // 모킹용 InputView 를 생성할 때 넘긴 문자열을 readInput 호출 시 반환하도록 설계 } }

인터페이스 쪽에서 디폴트로 만드는 의견도 좋다고 생각하지만 인터페이스에서는 protected 접근자로 메서드 선언이 안되기 때문에,
제 코드에서 이 함수를 디폴트로 만들고자 한다면 아마

public interface InputView { String getInput(); }

public abstract class AbstractInputView implements InputView { protected String readInput() { return Console.readLine(); } <- 추상 클래스에서 protected 접근자인 중복 메서드를 만들어 놓음 }

public class LottoInputView extends AbstractInputView { <- 추상 메서드를 상속해서 @Override public String getInput() { String input = readInput(); <- 추상 메서드에 있는 readInput 메서드를 사용! // ... 입력을 이용한 무언가 실행 } }

public class BridgeInputView extends AbstractInputView { <- 추상 메서드를 상속해서 @Override public String getInput() { String input = readInput(); <- 추상 메서드에 있는 readInput 메서드를 사용! // ... 입력을 이용한 무언가 실행 } }

이런 식으로 추상 메서드를 중간 다리로 끼운 후에
추상 메서드 안에서 중복되는 readInput 메서드를 구현해볼 수 있을 것 같아요!

하지만 우테코의 피드백 문서의 내용을 충실히 따른다면
이 protected 접근자도 결국 테스트코드만을 위해서 본래 코드가 영향을 받은 것이니
좋은 형태는 아니라고 생각하고 있어요!

다음 주차에는 이 부분을 개선해서, InputView 내부에서 Console.readLine() 메서드를 직접 사용하는 것이 아니라
Console.readLine() 메서드를 가지고 있는 별도의 클래스를 또 외부에서 주입받아 사용하는 형태로 설계한다면
좀 더 좋은 코드가 되지 않을까 생각합니다.

답변 감사합니다!!! InputView 함수형 인터페이스였네요 코드를 꼼꼼히 보지 않아서 죄송합니다ㅠㅠㅠ
제가 코드를 보면서 고민했던 것은 아래와 같은 코드인데 아래 방식으로 하는 것은 어떤 것 같나요?

public interface InputView { default String readInput() { return Console.readLine(); } String getInput(); }

좋다고 생각해요! 저처럼 같은 메서드를 중복으로 만들지 않고 인터페이스에서 한 번에 관리할 수 있게 하였다는 점에서 제가 제출한 코드보다 더 낫다고 생각해요 :)

wugawuga · 2022-12-05T06:22:50Z

src/main/java/pairmatching/repository/MissionRepository.java

+
+    public List<String> findAllNamesByLevel(Level level) {
+        return missions.stream()
+                .filter(mission -> mission.getLevel() == level)


Suggested change

.filter(mission -> mission.getLevel() == level)

.filter(mission -> mission.checkLevel(level))

get 를 이용해 가져와서 비교한다기 보다 객체로 메세지를 보내 처리하는 방법도 좋을 것 같아요!!

음! 감사합니다. :)

reddevilmidzy

코드 잘봤습니다!!
천천히 그리고 열심히 곱씹어보고 싶은 코드네요:)

reddevilmidzy · 2022-12-05T12:44:16Z

src/main/java/pairmatching/controller/MatchingPairController.java

+    private final PairsMaker pairsMaker;
+    private final OutputView outputView;
+
+    public MatchingPairController(CrewRepository crewRepository, PairMatchingRepository pairMatchingRepository, PairsMaker pairsMaker, OutputView outputView) {


이 부분 컨벤션의 열제한(120)과 파라미터 갯수 제한을 초과한거 같아요!

reddevilmidzy · 2022-12-05T13:37:46Z

src/main/java/pairmatching/controller/SelectingMissionController.java

+    private final InputView<PairMatchingInfo> inputView;
+    private final MissionRepository missionRepository;
+
+    public SelectingMissionController(OutputView outputView, InputView<PairMatchingInfo> inputView, MissionRepository missionRepository) {


이부분도 열제한(120)을 초과해서 inputView, 다음에 줄바꿈 해주셔야 할거 같아요!

reddevilmidzy · 2022-12-05T13:58:41Z

src/main/java/pairmatching/controller/MatchingPairController.java

+        do {
+            pairs = pairsMaker.makePairs(getShuffledCrewNames(crews));
+            notMatchedCount++;
+            if (notMatchedCount == 3) {


숫자 3은 상수로 변경해주어도 좋을거 같아요!

reddevilmidzy · 2022-12-05T14:09:00Z

src/main/java/pairmatching/controller/MatchingPairController.java

+import java.util.stream.Collectors;
+
+public class MatchingPairController extends AbstractController {
+    public static final String CREW_MATCHING_FAILED_THREE_TIMES_MESSAGE = "크루 매칭이 3회 이상 실패하였습니다.";


해당 상수는 MatchingPairController 에서만 사용되는거 같은데 혹시 public 으로 선언하신 이유가 있을까요?!
다른 코드들도 전부 상수는 public 인거 같은데 private랑 public 이랑 어떤 차이를 두셨는지 궁금해요!

테스트코드를에서 적절한 에러 메시지가 출력되는 지 검증하는 코드를 작성할 때 저 상수들을 사용합니다!
생각해보니 테스트 작성을 위해서 상수를 퍼블릭하게 두는 것도 바람직하지 않은 것 같네요. 이번 주 스터디 때 반영하겠습니다!

jinny-l

짧은 시간 안에 정말 많은 기능을 구현하셨네요!
저는 아직 Overide나 인터페이스를 익숙하게 다루지 못하는데,
자유자재로 다루시는 코드에 많이 배우고 갑니다.

jinny-l · 2022-12-07T11:14:53Z

docs/README.md

@@ -0,0 +1,29 @@
+- 사전 제공 정보


5시간 동안 모든 기능을 구현하셨나보네요!
대단합니다!! 👍

jinny-l · 2022-12-07T11:17:07Z

src/main/java/pairmatching/controller/ReadingCrewsFileController.java

+
+    public static final String BACKEND_CREWS_FILE_DIRECTORY = "src/main/resources/backend-crew.md";
+    public static final String FRONTEND_CREWS_FILE_DIRECTORY = "src/main/resources/frontend-crew.md";
+


상대경로/절대경로와 파일 입출력에 대해서 이번에 첨 접했는데
상대경로에 대해 배우고 갑니다!

jinny-l · 2022-12-07T11:19:36Z

src/main/java/pairmatching/repository/CrewRepository.java

+
+    public List<Crew> findByCourse(Course course) {
+        return crews.stream()
+                .filter(crew -> crew.getCourse() == course)


객체를 비교할 때는 보통 equals를 쓰는 것으로 알고 있는데, "==" 연산자를 사용하신 이유가 궁금합니다!

열거 타입(enum)은 인스턴스들이 고정되어 있기 때문에
equals() 메서드를 사용해 열거 타입 비교를 진행하더라도 내부적으로 결국 == 를 시행한다고 알고있기 때문이에요!

참고한 자료: https://webfirewood.tistory.com/132

이건 저도 몰랐는데 배웠네요. 감사합니다:)

Ridealist

저랑은 느낌이 많이 다른 설계방식을 보면서
아 이렇게도 설계할 수 있구나 많이 배우는 계기였습니다:)
남은 기간까지 화이팅입니다!

Ridealist · 2022-12-07T11:42:10Z

src/main/java/pairmatching/model/Course.java

+        this.name = name;
+    }
+
+    public static Course findByName(String courseName) {


findByName에서 Exception throwing 처리하는 것 아이디어 좋네요.
도메인 객체에서 validation을 진행해줄 수 있으니, 로직 간의 응집성도 높아지는 것 같구요:)
배워 갑니다!

Ridealist · 2022-12-07T12:38:48Z

src/main/java/pairmatching/system/convertion/NamesToCrewConverter.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class NamesToCrewConverter {


이 클래스는 불필요해보입니다.
결국 크루네임 리스트를 Crew 객체화하고 repo에 저장하려는 것인데, 이는 CrewRepository에서 구현되어도 충분해 보입니다. 아래 메서드를 CrewReposotory에 private 메서드로 선언하는 것을 고려해보면 어떨까 싶습니다.

Ridealist · 2022-12-07T12:42:18Z

src/main/java/pairmatching/system/convertion/StringToFeatureCommandConverter.java

+
+import java.util.Arrays;
+
+public class StringToFeatureCommandConverter {


이 클래스도 불필요해보입니다.
Course enum 클래스에서 내부 메소드로 validation까지 구현하신 것처럼, 이 메서드도 FeatureCommand의 내부 메소드로 처리가 가능해보입니다:)

전체적으로 Converter를 굳이 분리하지 않고 내부 메서드로 설계해도 된다는 관점이시군요! 이번 주 스터디때 해당 내용을 반영하겠습니다. 감사합니다 :)

Ridealist · 2022-12-07T13:31:26Z

src/main/java/pairmatching/system/PairApplication.java

+    public static final String RESET_PAIR_CONTROLLER_PATH = "resetPair";
+    private final Map<String, Controller> controllers = new HashMap<>();
+
+    public PairApplication() {


PairApplication() 생성자도 하나의 메서드이니 10~15 제한이 그대로 적용됩니다.
함수를 분리할 필요가 있어 보입니다:)

Ridealist · 2022-12-07T13:39:41Z

src/main/java/pairmatching/controller/Controller.java

+
+import java.util.Map;
+
+public interface Controller {


Controller를 interface로 선언하신게 신선하네요!

Ridealist · 2022-12-07T13:41:00Z

src/main/java/pairmatching/controller/AbstractController.java

+
+import java.util.Map;
+
+public abstract class AbstractController implements Controller {


Controller에서 오류가 발생할 경우 다시 해당 기능을 재요청 하기 위한 방법으로
abstract 클래스를 만들고 이를 controller 들에게 상속시킨 구조도 인상깊습니다.
다만, 객체지향적 설계의 입장에서 봤을 때는 조금 문제가 있을수도 있을 것 같습니다.
결국 각각의 단위 controller 들도 '클래스'인데 클래스를 형태를 한 메소드의 모음 같은 구조가 되버리지 않나 생각이 들어서요... 클래스 분리가 너무 '많이'된 느낌이 드는 건 사실인 것 같습니다.
이 부분 저도 더 고민이 필요할 것 같습니다. 확실히 프로그램 단위가 커지니 구조를 짜는게 쉽지 않네요...ㅎㅎ

Ridealist · 2022-12-07T13:53:25Z

src/main/java/pairmatching/model/Mission.java

+
+import java.util.Objects;
+
+public class Mission {


확장성의 차원에서 Mission을 일반 class로 선언한 것이 이해가 갑니다.
저도 클린 코드에 입장에서는 이 설계가 맞다고 생각하구요:)

하지만, 이 것을 enum으로 선언하면 enum 내부에 값을 넣고 시작할 수 있어서
Mission repository도 필요가 없어지고, 여기에 데이터를 넣는 로직도 생략할 수 있습니다.
시험이라는 환경에서는 위 선택지도 고려해볼 필요가 있다고 생각합니다.

음! 감사합니다. 이번 주 스터디에 해당 내용을 반영하겠습니다.

BackFoxx added 18 commits December 4, 2022 11:38

docs(readme): 사전 정보 전처리와 관련한 구현 목록 작성

823effa

feat(crew): 크루 목록이 담긴 파일을 읽어 리스트에 저장하는 기능 구현

a8c1848

feat(mission): 미션 목록을 리스트에 저장하는 기능 구현

4784324

feat(pairmatch): 페어 정보를 관리하는 저장소 설계

52365e6

docs(readme): 애플리케이션과 관련한 구현 목록 작성

e09784d

feature(featurecommand): 기능의 종류를 출력하고, 그 중 하나의 입력을 받는 기능 구현

0a2beb8

feature(outputview): 과정과 미션을 출력하는 기능 구현

0443fa4

feature(pairmatchinginfo): 매칭하고자 하는 과정, 레벨, 미션을 입력 받는 기능 구현

6bbe90f

docs(readme): 크루 매칭과 관련된 기능 구현 목록 작성

758efa0

feat(pairsmaker): 크루 목록을 섞은 후 매칭해 저장하는 기능 구현

bc26be2

feat(paircontroller): 이미 페어로 만난적이 있는 크루끼리 매칭 된다면 다시 랜덤으로 섞어서 매칭을 시도하는…

59bc36a

… 기능 구현

feat(outputview): 페어매칭 결과를 출력하는 기능 구현

1de38de

feat(findpaircontroller): 페어 결과를 조회하는 기능 구현

80b4a20

feat(resetpaircontroller): 페어 결과를 초기화하는 기능 구현

1769c1c

feat(pairapplication): 애플리케이션을 종료하는 기능 구현

dd79965

fix(pairmaker): 테스트 코드의 모킹 방식에 부합하도록 코드 수정

278fd48

feat(exceptionhandling): 예외 발생시 에러 메시지를 출력하고 값을 다시 받는 기능 구현

923ce8e

refactor: 메서드를 역할에 맞게 세부분리

f0d92e2

wugawuga reviewed Dec 5, 2022

View reviewed changes

reddevilmidzy reviewed Dec 5, 2022

View reviewed changes

jinny-l reviewed Dec 7, 2022

View reviewed changes

Ridealist reviewed Dec 7, 2022

View reviewed changes

	.filter(mission -> mission.getLevel() == level)
	.filter(mission -> mission.checkLevel(level))


		public static final String BACKEND_CREWS_FILE_DIRECTORY = "src/main/resources/backend-crew.md";
		public static final String FRONTEND_CREWS_FILE_DIRECTORY = "src/main/resources/frontend-crew.md";


		import java.util.Arrays;

		public class StringToFeatureCommandConverter {


		import java.util.Map;

		public abstract class AbstractController implements Controller {

[페어 매칭 - 스터디] 조승현 미션 제출합니다 :) #147

Are you sure you want to change the base?

[페어 매칭 - 스터디] 조승현 미션 제출합니다 :) #147

Uh oh!

Conversation

BackFoxx commented Dec 4, 2022

Uh oh!

wugawuga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BackFoxx Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reddevilmidzy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinny-l left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ridealist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

BackFoxx Dec 5, 2022 •

edited

Loading