[2단계 - 출석 다시 구현하기] 줄리(한승아) 미션 제출합니다.

[jbilee]

체크 리스트

• 미션의 필수 요구사항을 모두 구현했나요?
• Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
• 애플리케이션이 정상적으로 실행되나요?

객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?

1~5점 중에서 선택해주세요.

• 1 (전혀 충족하지 못함)
• 2
• 3 (보통)
• 4
• 5 (완벽하게 충족)

선택한 점수의 이유를 적어주세요.

컨트롤러가 상당히 많은 인스턴스 변수를 갖게 됐고, 주요 도메인을 getter로 노출시킨 부분에 대한 마땅한 대안책을 아직 찾지 못했습니다.

어떤 부분에 집중하여 리뷰해야 할까요?

안녕하세요, 범블비! 👋

2단계에서는 코멘트 주셨던 것처럼 1단계와의 차이점을 의식해보며 진행했습니다. 1단계에서는 페어와 함께 어느정도 탄탄한 설계를 먼저 짜둔 다음에 TDD를 시도했었는데 2단계에서는 제가 기대해야 하는 반환값들을 이미 알고 있어서인지 초반 설계에 시간을 들이지 않고도 단위 테스트를 큰 고민 없이 작성할 수 있었습니다. 추가로 1단계와는 다르게 테스트를 짜는 막막함이나 부담감이 덜한 느낌을 받았습니다. 도메인을 대부분 알고 있기도 했고, 테스트가 로직을 완벽하게 보장해주지는 않으며 리팩터링을 통해 계속 바뀔 수 있다는 점을 체험해봐서인 것 같습니다.

1단계 때 피드백 주신 부분과 이번주에 학습한 개념들을 잘 활용해보려고 했는데, 아직 결론을 못내려 코드에 반영하지 못한 깊은 고민 포인트가 있습니다..

지난 며칠간 불변 객체에 대해 학습하면서 상태를 외부에서 자유롭게 변경할 수 없도록 방지하는 방법에 대해 알게 됐습니다. 해서 AttendanceRecords가 본인이 가진 TreeSet 컬렉션을 그대로 반환하는게 아니라 toList 메서드를 써서 불변 List로 반환하는 등 복사본을 반환하도록 최대한 구현했는데, CrewRecords나 AttendanceRecords 인스턴스 자체를 매개변수로 전달해야 하는 경우에는 해당 객체들을 보호하지 못하는 이슈가 있었습니다.

단순히 콘솔에 출력하기 위해서 주요 비즈니스 로직을 가진 CrewRecords와 AttendanceRecords를 뷰에게 전달하게 됐는데, 그로 인해 뷰에서 직접적으로 해당 객체들의 메서드를 호출해서 내부 상태값을 수정할 수 있는 상황을 만들어버린 것 같습니다. 이전에는 이런 문제를 출력을 위한 문자열을 반환하는 메서드를 도메인 클래스 안에 구현해서 해결하고는 했는데, 1단계에서 언급해 주셨던 것처럼 뷰와 도메인을 분리하자는 관점에서 보면 적절한 해결책은 아닌 것 같다는 생각이 듭니다. 😵

그렇다고 도메인 클래스에 출력을 위한 메서드를 추가로 구현하면 일부 중복되는 코드가 발생하는 것은 물론, 컨트롤러에서 뷰의 메서드만 호출하면 간결하고 좋을텐데 컨트롤러 단에서 값을 전부 달라는 메세지를 CrewRecords나 다른 도메인 객체들에게 보내기에는 컨트롤러의 역할도 너무 광범위하게 주는 것 같습니다.... 이 부분에 관련해서는 가장 문제되는 코드에 코멘트 달아두도록 하겠습니다!

리뷰해주셔서 감사합니다! 🙏

[jbilee]

고민한 부분에 대한 코드입니다. 제적 위험자의 이름뿐만 아니라 결석과 지각 횟수도 필요해서 CrewRecords가 가진 메서드를 호출해 받아오도록 했는데, 결국 뷰가 CrewRecords의 상태를 바꿀 수 있는 메서드(addRecord 등)도 호출할 수 있는 점이 문제될 것 같습니다. CrewRecords가 아예 "- %s: 결석 %d회, 지각 %d회 (%s)%n" 문자열을 반환하도록 하는게 더 나은 선택일까요..?

[ddaaac]

이 질문도 뷰와 도메인을 깊게 고민하다보면 항상 나오는 질문입니다. 잘 고민해주셨어요.

일단 제 생각은 이전 코멘트와 동일하게 상관없다는 쪽입니다.
결국 객제 지향이니 뷰와 도메인이 분리되어야하느니 하는 건 모두 같은 어플리케이션을 만드는 개발자들 간의 약속입니다.
"뷰에서는 도메인 로직을 호출하지 않는다"라는 약속 정도만으로도 충분하다고 생각해요.

또, 콘솔 뷰이기 때문에 발생하는 고민이기도 하구요. 나중에 웹 뷰를 사용하게 되면 애초에 이런 고민을 안하게 될 수도 있어요.

[ddaaac]

또 다른 접근을 해보자면

도메인인 CrewRecords를 넘기긴 싫은데 제적 위험자의 정보는 넘겨야해야 되는 상황이죠.
그렇다면 그 정보만을 위한 클래스를 하나 만들어보는 건 어떨까요?
이 글의 첫번째 단락을 보면 좋을 것 같아요(뒤 내용은 당장 여기와는 관계가 없으니 무시해주세요. 괜히 더 헷갈릴 것 같네요)

[ddaaac]

안녕하세요 줄리 👋

2단계도 잘 구현해주셨네요.

몇 가지 코멘트 남겼으니 확인해주세요.

[ddaaac]

지난 며칠간 불변 객체에 대해 학습하면서 상태를 외부에서 자유롭게 변경할 수 없도록 방지하는 방법에 대해 알게 됐습니다. 해서 AttendanceRecords가 본인이 가진 TreeSet 컬렉션을 그대로 반환하는게 아니라 toList 메서드를 써서 불변 List로 반환하는 등 복사본을 반환하도록 최대한 구현했는데,

내부 상태의 보호를 고민했을 때 항상 나오는 질문이네요!
내부 상태를 완벽히 보호하기 위해서는 방어적 복사를 해볼 수 있겠지만, 사실 굳이? 싶긴합니다.
"내부 상태를 get해서 변경하지 않는다"라는 약속 정도로도 충분하다고 생각해요.
개인적으로 방어적 복사를 구현하기 위해서 코드가 추가되는게 더 비용이 증가하는 게 아닌가 싶다는 생각을 해요.

CrewRecords나 AttendanceRecords 인스턴스 자체를 매개변수로 전달해야 하는 경우에는 해당 객체들을 보호하지 못하는 이슈가 있었습니다.

이 부분 질문이 잘 이해가 되지 않는데 코드 예시가 있을까요?

[jbilee]

전혀 생각지도 못했는데 내부 상태를 변경하지 말자는 약속을 할 수도 있는거군요! 애플리케이션을 함께 만드는 개발자들 간의 약속이라는 부분이 더 이해되는 것 같습니다.

이 부분 질문이 잘 이해가 되지 않는데 코드 예시가 있을까요?

OutputView에 있는 아래 두 메서드도 제적 위험자를 출력하는 printWarnedCrews 메서드처럼 주요 비즈니스 로직을 가진 도메인 객체를 인자로 받는 구조가 나와서 고민이 됐었습니다. AttendanceRecords 일급 객체 또한 말씀주신 대로 값을 가진 전용 클래스를 하나 만드는 방향으로 구현해서 기존의 방식과 어떻게 다른지 비교해보려고 합니다!

// OutputView.java
public void printCrewRecord(LocalDate currentDate, AttendanceRecords attendanceRecords, Crew crew) {
    System.out.printf("%n이번 달 %s의 출석 기록입니다.%n", crew.name());
    attendanceRecords.getRecordsUntilBefore(currentDate)
            .forEach(this::printAttendanceRecord);
    printAttendanceStatus(attendanceRecords);
}

private void printAttendanceStatus(AttendanceRecords attendanceRecords) {
    System.out.println(System.lineSeparator());
    for (AttendanceStatus status : AttendanceStatus.values()) {
        int count = attendanceRecords.getAttendanceCount(status);
        System.out.printf("%s: %d회%n", status.getName(), count);
    }
}

[jbilee]

refactor(crew_records, view): 제적 대상자 추출 메서드 리팩터링

refactor(crew_records, view): 출석 횟수 출력 로직 리팩터링

제적 대상자와 관련된 로직의 경우 출력해야 하는 값이 많아서(크루 이름, 지각 횟수, 결석 횟수, 제적 상태) 별도 클래스를 생성했습니다.
출석, 지각, 결석 횟수에 대한 통계는 클래스를 생성하는 대신 Map<AttendanceStatus, Integer> 자료구조에 저장하여 뷰에게 전달하도록 수정해봤습니다.

[ddaaac]

이 질문도 뷰와 도메인을 깊게 고민하다보면 항상 나오는 질문입니다. 잘 고민해주셨어요.

일단 제 생각은 이전 코멘트와 동일하게 상관없다는 쪽입니다.
결국 객제 지향이니 뷰와 도메인이 분리되어야하느니 하는 건 모두 같은 어플리케이션을 만드는 개발자들 간의 약속입니다.
"뷰에서는 도메인 로직을 호출하지 않는다"라는 약속 정도만으로도 충분하다고 생각해요.

또, 콘솔 뷰이기 때문에 발생하는 고민이기도 하구요. 나중에 웹 뷰를 사용하게 되면 애초에 이런 고민을 안하게 될 수도 있어요.

[ddaaac]

또 다른 접근을 해보자면

도메인인 CrewRecords를 넘기긴 싫은데 제적 위험자의 정보는 넘겨야해야 되는 상황이죠.
그렇다면 그 정보만을 위한 클래스를 하나 만들어보는 건 어떨까요?
이 글의 첫번째 단락을 보면 좋을 것 같아요(뒤 내용은 당장 여기와는 관계가 없으니 무시해주세요. 괜히 더 헷갈릴 것 같네요)

[ddaaac]

TDD 사이클을 정말 잘게 나눠서 잘 지켜주셨어요.

다만, 어느 정도 단위의 기능들은 합쳐서 커밋해보셔도 좋을 것 같습니다.
몇 가지 테스트들을 죽 만들고, 그 기능을 구현하는 방식으로요.

각 방식에 장단점이 있긴 할텐데, 작게 나누는게 흐름을 끊어먹는 경우가 많다고 생각해요.
적당한 크기를 찾는 연습도 해보시면 좋을 것 같습니다.

[jbilee]

피드백 감사합니다! 구현 초반에는 특히 단위 테스트라는 점에 집중하느라 한번씩 커밋할 때마다 흐름이 끊기는 부분이 있었던 것 같습니다. 😅 도메인 내 좀 더 넓은 영역의 관점에서도 생각해보겠습니다!