최종 코딩 테스트 피어리뷰 PR #1

reddevilmidzy · 2023-12-18T14:00:23Z

No description provided.

zangsu

마지막 코테까지 정말 고생 많으셨습니다!!!
연말 잘 보내시고, 함께 좋은 결과 있었으면 좋겠네요 ㅎㅎ

zangsu · 2023-12-19T07:46:03Z

src/main/java/oncall/constant/ErrorMessage.java

+    INVALID_VAlUE("유효하지 않은 입력 값입니다."),
+
+    RETRY_INPUT("다시 입력해 주세요.");


두 메시지의 역할이 조금 다르다고 느껴지는데요.
INVALID_VALUE는 특정 오류상황을 설명해 주는 메시지인 반면, RETRY_INPUT은 모든 오류상황에서 사용되는 재입력 메시지라는 점이 다르다고 느껴집니다.

저는 이 경우에 "모든 상황에서 사용되는 값"은 구분을 두는 편을 선호하는데요. 그 이유는 단순히 values() 메서드 호출시 두 값이 함께 배열에 담기는 것이 어색하게 느껴져서 입니다.
다만, 이 부분은 취향의 영역에 해당하는 것 같으니 한번 고민 해 보시고 더 편하신 방법을 사용하시면 좋을 것 같습니다!

Suggested change

INVALID_VAlUE("유효하지 않은 입력 값입니다."),

RETRY_INPUT("다시 입력해 주세요.");

INVALID_VAlUE("유효하지 않은 입력 값입니다."),

public static final String RETRY_INPUT = "다시 입력해 주세요.";

RETRY_INPUT 이라는 이름으로 ErrorMessage 이넘에 포함되어 있는게 부자연스럽긴 하네요!

zangsu · 2023-12-19T07:57:26Z

src/main/java/oncall/model/WorkingMonth.java

+    }
+
+    private static void validateSeparator(String value) {
+        if (value.trim().isEmpty()) {


String.trim().isEmpty()의 흐름은 해당 String이 공백만으로 이루어져 있는지를 확인하기 위한 로직으로 알고 있는데요.
해당 기능을 해 주는 메서드가 JDK 11부터 제공되는 것으로 알고 있습니다!

zangsu · 2023-12-19T09:46:32Z

src/main/java/oncall/model/WorkingMonth.java

+        List<String> target = Arrays.stream(value.split(SEPARATOR)).toList();
+        Month month = Month.valueOfName(target.get(MONTH_INDEX));
+        Day day = Day.valueOfName(target.get(DAY_INDEX));


String.split()의 결과인 String[]를 그대로 사용하더라도 충분히 자연스러운 코드였을 것 같습니다.

어쩌면 공통 피드백이었던 "배열보다는 Collection을 사용하라"는 피드백의 결과였을 수도 있을 것 같은데요.
많은 부분에서 Collection은 제공되는 API를 사용할 수 있기에 좋은 선택지가 되겠지만, 모든 상황에서 해당 피드백이 적용될 필요는 없다고 생각해요.
이번의 경우는

사용할 데이터가 값을 저장하는 용도로밖에 사용되지 않는다.

해당 데이터가 만들어지는 초기 형태가 배열이다.

라는 점을 고려했을 때 오히려 List로 변환하는 것이 불필요한 연산의 횟수를 늘릴 것 같다는 의견입니다.

zangsu · 2023-12-19T09:49:54Z

src/main/java/oncall/model/Month.java

+import java.util.Arrays;
+import oncall.constant.ErrorMessage;
+
+public enum Month {


놀랍도록 저와 비슷한 클래스를 구현하셨군요!
저도 오늘에서야 깨달은 부분인데, 해당 클래스는 java.time.Month enum으로 완벽히 대체 가능하더라고요...ㅎㅎ

zangsu · 2023-12-19T09:55:57Z

src/main/java/oncall/model/Date.java

+    private final Month month;
+    private final int date;
+    private final Day day;
+    private final boolean holiday;


해당 인스턴스 필드는 생성자에서 사용한 메서드 그대로 판별이 가능한데요.
그렇다면 인스턴스 필드로 두는 것 보다 메서드에서 직접 판별을 해주는 것은 어떨까요?
인스턴스 필드의 개수를 최소화 하라던 공통 피드백이 있었습니다!

zangsu · 2023-12-19T10:00:25Z

src/main/java/oncall/model/Date.java

+    @Override
+    public String toString() {
+        if (holiday) {
+            return month.getValue() + "월 " + date + "일 " + day.getName() + "(휴일)";
+        }
+        return month.getValue() + "월 " + date + "일 " + day.getName();
+    }


객체가 출력의 책임을 가지고 있으면 출력 형태의 변경이 필요할 때 일반적으로 출력의 기능을 제공하는 view 계층이 아닌 객체 내부가 수정되기에 DTO를 사용하는 등의 방법으로 출력의 책임을 분리할 수 있습니다.

toString()을 이용해 객체가 출력의 책임을 가지도록 한 것은 시간 제약때문에 타협했던 부분이라고 이해해도 괜찮을까요?

+) 추가적으로, toString()의 용도에는 의견이 갈리지만 많은 사람들이 toString() 메서드를 디버깅의 용도로 사용하는 것이 옳다고 주장합니다. 관련 아티클 첨부 드립니다! Java 에서 toString 메소드의 올바른 사용 용도에 대하여

맞습니다! 저도 toString() 메서드는 디버깅 용으로 사용하는 것이 용도가 맞다고 생각해요
사실 이렇게 toString()으로 특정한 정보까지 표현하여 출력하는 방식을 자동차경주 미션에서 썼다가 이펙티브 자바 도서를 읽고 이렇게 쓰면 안되겠다라고 생각을 했습니다
이번 미션에서는 시간 단축을 위해 toString에게 좀 많은 책임을 부여했네요 ㅎㅎ

이 부분에서도 day가 null이라면 NullPointerException가 발생할 수 있겠네요 :)

zangsu · 2023-12-19T10:01:53Z

src/main/java/oncall/model/Date.java

+        if (this == obj) {
+            return true;
+        }
+        if (!(obj instanceof Date target)) {


자바 17에서 제공하는 instanceof의 패턴 매칭 기능을 사용하셨군요!!
며칠전 새로 알게되어 너무 좋았던 기능이라 반갑네요 👍

zangsu · 2023-12-19T10:09:00Z

src/main/java/oncall/controller/InputController.java

+        return WorkingMonth.from(value);
+    }
+
+    public List<Employees> getWorkers() {


평일 근무 순번과 휴일 근무 순번을 List<Employees>로 저장하게 되면 평일 근무 순번이 몇 번째 인덱스에 저장되어야 하는지를 다른 클래스에서도 알고 있어야 할 것 같아요.
List<Employees>를 래핑하는 일급 컬렉션을 사용해 보는 것은 어떨까요?

get~~은 어떤 객체의 속성값을 그대로 가져온다는 의미로 많이 사용된다고 알고 있습니다!
여기서는 input, generate 등의 메서드명을 써보면 어떨까요? :)

zangsu · 2023-12-19T10:12:05Z

src/main/java/oncall/controller/InputController.java

+        while (true) {
+            try {
+                return readWorkingMonth();
+            } catch (IllegalArgumentException exception) {
+                outputView.printErrorMessage(exception);
+            }
+        }


해당 클래스에선 다음의 흐름이 중복되고 있어요.

while(true){ try{ return 얻고자 했던 값; } catch (IllegalArgumentException exception){ outputView.printErrorMessage(exception); } }

해당 흐름은 입력받을 데이터가 늘어날 때 마다 동일하게 사용될 수 있는 흐름인데요.
이런 경우에는 해당 흐름을 메서드로 분리하면서 내부에 변경되어야 하는 로직을 함수형 인터페이스로 파라미터화 할 수 있습니다!

혹여나 해당 부분에 대한 추가 학습이 필요하시다면 "동작 파라미터화", "함수형 인터페이스", "전략 패턴" 등을 키워드로 학습해 보시면 좋을 것 같습니다!

parksangchu

최종 코테하시느라 정말 고생 많으셨습니다! 본 과정에서 함께 꼭 뵙길바라겠습니다!!

parksangchu · 2023-12-19T12:02:00Z

src/main/java/oncall/service/Assignment.java

+    private int weekdayIndex;
+    private int holidayIndex;
+
+    public Assignment(int weekdayIndex, int holidayIndex) {


weekdayIndex와 holidayIndex가 둘다 0으로 초기화 되는 상황이라면 생성자를 통해 외부에서 받아오지 않아도 됐을것 같습니다!

동의합니다! 생성자를 private로 감싸고 정적 팩토리 메서드를 사용하는게 더 좋아보이네요

parksangchu · 2023-12-19T12:05:48Z

src/main/java/oncall/service/Assignment.java

+            preWorker = assignWorker(result, weekdayWorker, weekdayWorker);
+        }
+        return result;
+    }


초기에는 순번대로 근무편성을 하고 이후에 겹치는 부분을 찾아 재편성하는 식으로 로직을 분리하는게 괜찮을것 같습니다!

확실히 이번 문제에서 이 로직이 가장 어려운 부분이었던 것 같아요...
메서드가 길어지더라도 잘 작동하는게 최우선이라는 것을 동글님 코드를 보고 뼈저리게 느끼고갑니다 😭

저는 이 부분에 대해서 테스트 코드를 제대로 작성하지 않았던 게 한탄스럽더라구요...ㅜㅜ 아무래도 이번에 출력되는 것이 30개로 길다보니 출력해서 하나 하나 확인하디보단, 테스트 코드를 제대로 짜두는 것이 구현 실수를 하지 않았는지 쉽게 확인할 수 있었다고 생각합니다.
동글님께서는 테스트코드 없이도 로직을 매우 잘 짜신 것으로 보입니다...!! 이와 관련해 비결이 궁금하네요 ㅎ🤲

맞아요 이번 미션 출력문이 좀 길어서 눈 디버깅보다는 테스트 코드를 작성하는 것이 시간도 더 아낄 수 있었다고 생각합니다!

테스트코드는 없어도 계속 눈 디버깅으로 돌려서 요구 사항을 맞출 수 있었던 거 같네요😀

parksangchu · 2023-12-19T12:08:29Z

src/main/java/oncall/model/Day.java

+import java.util.Arrays;
+import oncall.constant.ErrorMessage;
+
+public enum Day {


Day라는 클래스 명은 너무 포괄적인거 같아서 다른 명칭을 사용하시는건 어떠실까요?

동의합니다! 아예 이 enum을 제거하고 자바가 제공하는 DayOfWeek를 사용하는 것도 방법일 거 같네요

parksangchu · 2023-12-19T12:12:51Z

src/main/java/oncall/model/Employees.java

+        if (value.contains(SEPARATOR.repeat(2))) {
+            throw new IllegalArgumentException(ErrorMessage.INVALID_VAlUE.getMessage());
+        }
+    }


해당 부분들은 Employee 생성자에서 닉네임 길이 검증으로 처리 가능한 부분이기에 추가로 검증하지 않아도 괜찮지 않을까요???

newh08

최종코테 수고하셨습니다!
함께 좋은 결과가 있었으면 좋겠네요 ㅎㅎ

newh08 · 2023-12-18T20:24:00Z

src/main/java/oncall/model/Employees.java

+        if (!(obj instanceof Employees target)) {
+            return false;
+        }
+        return Set.copyOf(workers).equals(Set.copyOf(target.workers));


set 을 활용해서 평일, 공휴일 근무 인원이 동일한지 확인하는 방법 너무 좋네요!

newh08 · 2023-12-18T20:28:21Z

src/main/java/oncall/controller/InputController.java

+public class InputController {
+
+    private final InputView inputView;
+    private final OutputView outputView;


Input 값을 InputController 에서 모두 받아 변환하는 방법은 너무 깔끔하고 좋은 것 같습니다!

outputView 는 에러 출력을 위해 있는데 처음에 이 필드변수를 보고 InputController 에 outputView 가 왜있지? 라는 생각이 들었습니다. 혹시 outputView 에서 에러출력만 분리하는건 어떻게 생각하실까요?

충분히 고려해볼만한 점인 거 같네요👍👍

newh08 · 2023-12-18T20:38:58Z

src/main/java/oncall/controller/OnCallController.java

+        WorkerRepository workerRepository = new WorkerRepository();
+        workerRepository.register(Week.WEEKDAY, workers.get(0));
+        workerRepository.register(Week.HOLIDAY, workers.get(1));


제 코드를 셀프리뷰 했을 때 아쉬웠던 부분과 같은게 보여서 코멘트 남겼습니다!
Repository 를 생성하고 저장하는 기능이 Controller 에 있기 보단 Service 에서 진행하는 것에 대해 어떻게 생각하실까요?
제 코드를 셀프리뷰 했을 때 컨트롤러는 뷰와 도메인을 연결하는 역할만 하는게 아니라 다른 기능이 추가로 있는 점이 수정해야 한다고 생각했는데 이 부분을 어떻게 생각하실지 궁금하네요.

저도 repository에 대한 접근은 service단에서 하는 것이 더 좋다고 생각합니다 :)
https://dev-setung.tistory.com/19
layerd architecture에 대해 위의 링크를 참고해보시면 좋을 듯 싶어요!
Controller는 표현 계층, Service는 비지니스 계층, repository는 Persistence 계층에 해당된다고 생각합니다!
layerd architecture에 따르면, 각 계층을 2단계 건너뛰기보단 한 단계씩만 건너뛰어야 한다고 하네요 :)

좋은 피드백이군요👍👍 감사합니다
프리코스 미션에서는 repository를 사용하지 않다가 이전 최종 코딩 테스트 문제 풀어보면서 사용해보았었는데 미숙했었던 거 같네요 ㅎㅎ

newh08 · 2023-12-18T21:08:47Z

src/main/java/oncall/model/Date.java

+    private final Month month;
+    private final int date;
+    private final Day day;
+    private final boolean holiday;


이 필드 변수는 삭제가 가능할 것 같습니다.
프리코스에서 받은 공통 피드백 중에 가지고 있는 필드 변수에서 생성 가능한 필드 변수는 줄이는 것이 좋다는 내용이 있었어서 같은 코멘트 남겨봅니다!

newh08 · 2023-12-18T21:24:24Z

src/main/java/oncall/service/Assignment.java

+
+    public List<Employee> assign(WorkingMonth month, WorkerRepository repository) {
+        List<Date> dates = month.getMonthDate();
+        Iterator<Date> iterator = dates.iterator();


Iterator를 활용하는 방법이 있었군요,, 생각치도 못했습니다

newh08 · 2023-12-18T22:30:22Z

src/main/java/oncall/repository/WorkerRepository.java

+
+public class WorkerRepository {
+
+    private Map<Week, Employees> repository;


이런식으로 직원을 구분할 수 있었네요,, 제가 사용한 방법보다 훨씬 효율적인 것 같아 리펙토링때 적용해보려 합니다.

repository까지 쓰셨다니 좋네용 👍

inpink

안녕하세요 동글님! 😀
우선 도움이 되는 리뷰들을 남겨주셔서 다시 한 번 감사합니다 👍
동글님의 이번 프로젝트에서, 기능 요구사항을 매우 꼼꼼히 분석하시고 실수 없이 잘 작동하도록 구현하신 점이 최고최고 👍 라고 생각합니다!
오늘 추가로 만들어본 기능 테스트에서 동글님 코드가 전부 통과하시더라구요... 👍
시간 제한이 있는 상황에서, 기능 구현이 안 된 부분이 있다면 아무리 잘 모듈화한다고 한 들 무슨 소용일까! 라는 생각이 정말 많이 드는 시험이었습니다..ㅎㅎ 그런 의미에서 동글님의 코드에서 본받을 점이 참 많다고 생각이 드네요 👍
좋은 코드 잘 보고갑니다! 😃 좋은 결과 있으시길 바라요 😊👍👍👍👍👍

inpink · 2023-12-18T20:35:49Z

src/test/java/oncall/service/AssignmentTest.java

+
+
+    @Test
+    @DisplayName("두번 일하지 마!")


ㅋㅋㅋㅋDisplayName이 너무 귀여운데요..? 👍

inpink · 2023-12-19T16:50:05Z

src/main/java/oncall/constant/ErrorMessage.java

+        return message;
+    }
+
+}


[Error]를 OutputView에 두셨군요!
이건 정말 개인 스타일 차이일 것 같은데, 저는 아래와 같이 prefix를 ErrorMessages에서 함께 관리했습니다 ㅎㅎ
이 방법에 대해 어떻게 생각하시는지 동글님의 의견이 궁금하네요 👍🤩

package oncall.messages; public enum ErrorMessages { INVALID_INPUT("유효하지 않은 입력 값입니다. 다시 입력해 주세요."), INVALID_DATE("유효하지 않은 날짜입니다."); private static final String prefix = "[ERROR] "; private final String message; ErrorMessages(final String message) { this.message = prefix + message; } public String getMessage() { return message; } }

저도 이전 미션에서 [ERROR]를 에러 메시지를 가지고 있는 클래스에 넣고 사용해보기도 했다가 View에 넣어도 보고 했는데 두 방식에 이렇다할 엄청난 차이점이 있진 않은거 같더라구요😅

그럼에도 [ERROR] 를 위처럼 사용하지 않은 이유는 어떤 때는 [ERROR], 또 다른 때에는 [WARNING]로 표현해 단순 경고를 띄어주는 시나리오를 그렸을 때 어떤 구조가 더 아름다울지 고민하다가 결국 [ERROR]를 View로 이동시켜주었습니다

inpink · 2023-12-19T17:10:55Z

src/main/java/oncall/controller/InputController.java

+        return WorkingMonth.from(value);
+    }
+
+    public List<Employees> getWorkers() {


get~~은 어떤 객체의 속성값을 그대로 가져온다는 의미로 많이 사용된다고 알고 있습니다!
여기서는 input, generate 등의 메서드명을 써보면 어떨까요? :)

inpink · 2023-12-19T17:37:54Z

src/main/java/oncall/controller/InputController.java

+        if (!weekday.equals(holiday)) {
+            throw new IllegalArgumentException(ErrorMessage.INVALID_VAlUE.getMessage());
+        }
+    }


저같은 경우에는 이렇게 MVC 전역에서 사용될 수 있는 validator, util 기능을 따로 클래스로 분리해두었습니다! 👍
시간이 되신다면 이 부분을 Validator class로 분리해보시면 어떨까요? :)

inpink · 2023-12-19T18:05:59Z

src/main/java/oncall/model/Employees.java

+                .map(Employee::new)
+                .toList();
+        return new Employees(result);
+    }


정적 팩터리 메서드를 잘 쓰셨군요 👍
여기서도 스트림 👍 👍 👍 👍
저는 정적 팩터리 메서드가 생성자의 일종이라 생각해서 생성자 바로 아래 뒀는데,
메서드 배치 순서에 대해 동글님의 의견이 궁금합니다 :)

생성자 바로 아래에 두는 형태가 더 좋아 보이네요!!

inpink · 2023-12-19T18:47:53Z

src/main/java/oncall/model/Week.java

+
+    WEEKDAY("평일"),
+    HOLIDAY("휴일"),
+    ;


ㅎㅎ 사소한 오타 발견했습니다 😋
아무래도 5시간이라는 제한이 있다보니, 기능이 잘 작동하는 게 가장 중요한 것 같아요! 동글님의 코드를 봤을 때 비지니스 로직에 대해 상세히 분석하시고, 오류 없이 잘 구현하신 점이 대단하시다 생각합니다 👍

inpink · 2023-12-19T18:49:07Z

src/main/java/oncall/repository/WorkerRepository.java

+
+public class WorkerRepository {
+
+    private Map<Week, Employees> repository;


repository까지 쓰셨다니 좋네용 👍

inpink · 2023-12-19T18:52:26Z

src/main/java/oncall/service/Assignment.java

+            preWorker = assignWorker(result, weekdayWorker, weekdayWorker);
+        }
+        return result;
+    }


확실히 이번 문제에서 이 로직이 가장 어려운 부분이었던 것 같아요...
메서드가 길어지더라도 잘 작동하는게 최우선이라는 것을 동글님 코드를 보고 뼈저리게 느끼고갑니다 😭

inpink · 2023-12-19T18:52:56Z

src/test/java/oncall/model/EmployeesTest.java

+        assertThat(equals).isTrue();
+        assertThat(b.equals(c)).isFalse();
+    }
+}


기능 요구사항에 대해 테스트를 정말 꼼꼼히 하셨네요 👍 존경스럽습니다

inpink · 2023-12-19T18:55:10Z

src/main/java/oncall/service/Assignment.java

+            preWorker = assignWorker(result, weekdayWorker, weekdayWorker);
+        }
+        return result;
+    }


저는 이 부분에 대해서 테스트 코드를 제대로 작성하지 않았던 게 한탄스럽더라구요...ㅜㅜ 아무래도 이번에 출력되는 것이 30개로 길다보니 출력해서 하나 하나 확인하디보단, 테스트 코드를 제대로 짜두는 것이 구현 실수를 하지 않았는지 쉽게 확인할 수 있었다고 생각합니다.
동글님께서는 테스트코드 없이도 로직을 매우 잘 짜신 것으로 보입니다...!! 이와 관련해 비결이 궁금하네요 ㅎ🤲

reddevilmidzy added 16 commits December 16, 2023 13:49

docs: 구현 기능 목록 생성

e99de37

feat: 비상 근무 배정 월과 시작 요일 입력

8727979

feat: 평일 비상 근무 사원 닉네임 입력

924f343

feat: 휴일 비상 근무 순번대로 사원 닉네임 입력

7da4c1e

feat: [ERROR] 로 시작하는 에러 메시지 출력

6c1e3fe

feat: 휴일 비상 근무 입력 예외시 평일 비상 근무부터 입력

84773c9

feat: 평일 근무와 휴일 근무 일정 계획

3e55437

feat: 근무 배정 정보 출력

eb7346c

feat: 중복되지 않도록 근무 배정

c3b8594

test: 닉네임 길이 5초과시 예외

8135c0f

feat: 평일 순번과 휴일 순번에 1회씩 편성

50bd40a

docs: 미션 해결 전략 추가

5015327

refactor: 코드 메서드 분리

81c3704

refactor: 하드 코딩 제거

cd70297

refactor: 공휴일 List 생성

ee9790a

docs: 핵심 기능 파악

9bb326a

zangsu reviewed Dec 19, 2023

View reviewed changes

parksangchu reviewed Dec 19, 2023

View reviewed changes

newh08 reviewed Dec 19, 2023

View reviewed changes

inpink reviewed Dec 19, 2023

View reviewed changes

		INVALID_VAlUE("유효하지 않은 입력 값입니다."),

		RETRY_INPUT("다시 입력해 주세요.");


		public class WorkerRepository {

		private Map<Week, Employees> repository;

최종 코딩 테스트 피어리뷰 PR #1

Are you sure you want to change the base?

최종 코딩 테스트 피어리뷰 PR #1

Conversation

reddevilmidzy commented Dec 18, 2023

zangsu 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

parksangchu 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

newh08 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

inpink left a comment • 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

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

inpink left a comment •

edited

Loading