[Lv1] 출석부 미션 회고

출석부 미션 회고

10줄 요구사항

10줄 제한 요구사항을 지키기 위해 충분히 명확함에도 불구하고 메서드를 분리하는 경우가 있었다. 페어와 함께 이 메서드를 분리하면 좋을지, 10줄 제한 요구사항을 지키지 않을지 토론했다.

    private void process(final Crews crews, final Command command) {
        if (command.equals(Command.CHECK_ATTENDANCE)) {
            checkAttendance(crews);
        }
        if (command.equals(Command.MODIFY_ATTENDANCE)) {
            modifyAttendance(crews);
        }
        if (command.equals(Command.CHECK_ATTENDANCE_BY_CREW)) {
            checkAttendanceHistoryByCrew(crews);
        }
        if (command.equals(Command.CHECK_DISMISSAL_CREW)) {
            checkDismissalCrews(crews);
        }
    }

 

답이 나오지 않아서, 리뷰어분에게 물어봤더니 확장성있게 리팩터링해보는 것은 어떻겠냐는 답변을 받았다.

 

함수형 인터페이스 Consumer 이용하여 리팩토링

public class AttendanceController {

    private final Map<Command, Consumer<CrewHistories>> commands = Map.of(
            Command.CHECK_ATTENDANCE, this::checkAttendance,
            Command.MODIFY_ATTENDANCE, this::modifyAttendance,
            Command.CHECK_ATTENDANCE_BY_CREW, this::checkAttendanceHistoryByCrew,
            Command.CHECK_DISMISSAL_CREW, this::checkDismissalCrews
    );

       private void process(final CrewHistories crewHistories, final Command command) {
        Consumer<CrewHistories> consumer = commands.get(command);
        consumer.accept(crewHistories);
    }

• 분기문이 줄어들어 명령어가 더 늘어나더라도 훨씬 가독성이 좋다.
• 하지만 Controller에 각 커맨드별 매핑되는 메서드가 존재하여 Controller의 크기가 매우 커지는 문제가 존재하였다.

 

커맨드 패턴 이용한 리팩터링

• 각 명령별로 객체를 만들어 실행 흐름을 넘겨준다.
• 이를 통해 Controller가 명령별 메서드로 인해 너무 커지는 문제를 방지할 수 있다.
  • Controller는 단순히 명령을 실행하는 역할만 수행한다.
• 또한 명령별 로직이 분리되어 훨씬 유지보수하기 쉬워지고 복잡도가 줄었다.

public class AttendanceController {

    private final ResultView resultView;
    private final Campus campus;
    private final TodayClock todayClock;
    private final Map<MenuOption, Command> commands = initializeCommands();

    private void process(final CrewHistories crewHistories, final MenuOption menuOption) {
        Command command = commands.get(menuOption);
        command.execute(crewHistories); 
    }
}

public class FindByCrewCommand implements Command {

    private final InputView inputView;
    private final TodayClock todayClock;
    private final ResultView resultView;

    public FindByCrewCommand(final InputView inputView, final TodayClock todayClock, final ResultView resultView) {
        this.inputView = inputView;
        this.todayClock = todayClock;
        this.resultView = resultView;
    }

    @Override
    public void execute(final CrewHistories crewHistories) {
        String nickname = inputView.readNickname();
        CrewHistory crewHistory = crewHistories.findCrewByNickname(nickname);

        LocalDate todayDate = todayClock.getTodayDate();

        List<LocalDateTime> attendanceHistory = crewHistory.getAttendanceHistory(todayDate);
        AttendanceCounter attendanceCounter = crewHistory.countAttendanceType(todayDate);

        resultView.printAttendanceHistoryResultByCrew(nickname, attendanceHistory, attendanceCounter);
    }
}

 

확장성에 대한 고민

• 크루별 출석 기록은 Map<일자, 출석날짜시간>으로 저장되어있다.
  • 처음에는 List<LocalDateTime>으로 출석 기록을 가지고 있다가, 수정시에 출석 기록을 일자로 접근하기 때문에 성능을 위해 Map을 사용하게 되었다.
  • 요구사항이 12월 출석 기록을 관리하는 출석부이므로, key를 일자로만 두었다.

public class Crew {

    private final Map<Integer, LocalDateTime> attendance;

    public Crew(final Map<Integer, LocalDateTime> attendance) {
        this.attendance = new HashMap<>(attendance);
    }

• 그런데 리뷰어가 "현재는 12월에 한정된 요구사항이지만, 정식적으로 이 시스템이 사용된다면 어떻게 될까요?" 라는 질문을 주셨다. 만약 정식적으로 출석부 시스템을 사용하게 된다면 12월 뿐 아니라 다른 달, 년도까지 지원하는 것은 당연하기 때문이다.
• 우선 요구사항이 12월이므로 key를 일자로 두었지만, 다른 달, 년도까지 고민하여 미션을 설계하는 것이 적절했을까? 라는 고민이 들었다.

 

YAGNI vs 적절한 확장성 고려

YAGNI

• YAGNI(You Ain't Gonna Need It) : "정말 필요할 때까지는 만들지 말자"
  • 실제로 필요하다고 확실해질 때까지 기능을 추가하지 말아야 한다.
  • 미래에 필요할 것이라는 예측에 기반한 개발을 피하자.
  • YAGNI 를 통해 설계를 단순하게 유지하고, 실제 요구사항에 집중할 수 있다.

적절한 확장성 고려

• 그런데, YAGNI원칙에 압도되어 충분히 확장성을 고려하지 못한 것은 아닐까? 라는 고민이 들었다. 따라서 리뷰어에게 확장성을 어떤 기준으로 고려하시는지 질문하였다.

 

확장성 판단 기준

• 경험적으로 이 기능이 확장될 여지가 많은가?
• 확장성있게 설계하는데 훨씬 많은 리소스가 드는가?
• 확장성있게 설계하지 않았는데, 추후 예상했던 대로 기능이 확장된다면 이를 수정하는데 너무 많은 노력이 들지는 않는가?

출석부의 경우 확장될 여지가 아주 높기 때문에, 처음 설계할때부터 고려하는 것이 좋다는 의견을 주셨다.

 

결론

• YAGNI라는 원칙에 압도되어 적절하게 확장성을 고려하지 못했다라는 반성을 하게 되었다. 원칙을 지키기보다는 항상 요구사항에 가장 적절한 설계가 무엇일지 고민하고 타협하는 자세를 가져야겠다.
• 좀 더 유연한 개발자가 되자 ! 😁😁

 

enum

싱글톤 특성

• enum은 jvm에 의해 싱글톤 특성이 보장된다, 즉, 오직 하나의 인스턴스만 생성된다.

public boolean isApplicable() {
    return this != SubjectType.NOT_APPLICABLE;
}

• 참조가 동일하기 때문에, equals()가 아닌 ==을 사용하는 것이 더 명확하다.
  • 또한 enum은 컴파일 시점에 타입 체크가 보장된다.

 

한글 상수명 정의

• 가독성을 위해 enum의 상수들을 한글로 정의하였다. 그런데 intellij에서 Non-ASCII 경고가 떠서 알아봤더니 호환성 문제가 발생할 수 있다고 한다.
• 그리고 리뷰어가 영어로만 코드를 읽다가 주석이 아닌 변수명에서 한글을 만나게 되니 익숙치 않다고도 했다.

enum의 이름은 영어로 작성하자!

 

AS IS

public enum AttendanceType {

    출석(Duration.ofMinutes(0)),
    지각(Duration.ofMinutes(5)),
    결석(Duration.ofMinutes(30));

    public static final LocalTime DEFAULT_TIME = LocalTime.MIN;

 

TO BE

public enum AttendanceType {

    ATTENDANCE("출석", Duration.ofMinutes(0)),
    LATE("지각", Duration.ofMinutes(5)),
    ABSENCE("결석", Duration.ofMinutes(30));

    public static final LocalTime DEFAULT_TIME = LocalTime.MIN;

 

Enum에 비즈니스 로직을 두어도 될까?

• enum은 상수의 집합이므로, 만약 중요한 로직이 enum에 포함된다면 따로 객체를 두어 수행하는 것이 낫지 않을까? 라는 생각을 하게 되었다. 따라서 enum을 최대한 가볍게 두어 간단한 로직만 처리하도록 하게 만들었다.

• 그런데 리뷰어가 enum을 단순히 상수의 집합으로만 보는 것은 enum을 제대로 활용하는 것이 아니라는 의견을 주었다.enum은 메서드와 필드를 추가할 수도 있으므로, 상태와 함께 객체의 비즈니스 로직을 담을 수 있는 효율적인 타입이기 때문이다.
• 객체지향 관점에서도 상수별 로직이 달라지는 경우 enum 내에 정의하여 메서드를 호출하게 하는 것이 데이터를 가진 곳에서 작업을 수행하여 객체지향적이라는 생각이 들었다.

 

결론

• enum도 객체라고 볼 수 있다! 상수별 달라지는 비즈니스 로직은 enum에서 처리하자!

 

MVC 패턴 : Model vs View

• MVC 패턴에서 M은 모델 (데이터), C는 Controller(Model과 View 사이를 이어주는 부분), V는 View(사용자에게 보여지는 부분)를 말한다.
• 그럼 아래 코드는 View일까? Model일까?

public enum Command {

    CHECK_ATTENDANCE("1"),
    MODIFY_ATTENDANCE("2"),
    CHECK_ATTENDANCE_BY_CREW("3"),
    CHECK_DISMISSAL_CREW("4"),
    QUIT("Q");

    private final String command;

    Command(String command) {
        this.command = command;
    }

    public static Command from(final String input) {
        return Arrays.stream(Command.values())
                .filter(command -> command.command.equals(input))
                .findFirst()
                .orElseThrow(() -> new IllegalArgumentException("잘못된 입력입니다."));
    }
}

 

 

• 정답은 View이다! 데이터이므로 Model이라고 생각할 수 있지만, 모든 사용자에게 동일하게 보여지는 부분이므로 View이다.
• Model이기 위해서는 사용자마다 데이터에 따라 다르게 보여지는 정보여야한다.
  • ex) 출석 정보

Model : 사용자마다 다르게 보여지는 부분
View : 모든 사용자에게 동일하게 보여지는 부분

 

DTO

public class Crews {

    private final Map<String, Crew> crews;

    public Crews(final Map<String, Crew> crews) {
        this.crews = crews;
    }

    public List<DismissalCrewDto> findDismissalCrewDtos(final LocalDate todayDate) {
        List<DismissalCrewDto> dtos = new ArrayList<>();
        for (Entry<String, Crew> entry : crews.entrySet()) {
            addDismissalCrewDto(todayDate, entry, dtos);
        }
        return dtos;
    }

 

DTO란?

• dto는 계층간 데이터 교환을 위해 사용하는 객체이다. controller는 view와 model의 데이터를 주고받을 때 dto를 사용한다.

 

도메인이 DTO에 의존하면 안되는 이유

• dto는 요구사항에 의해 자주 바뀔 수 있기 때문에, 여러 곳에서 참조되는 entity가 dto를 의존하면 안된다.
• 즉, 도메인은 비즈니스 로직에만 집중해야하며, 표현 계층의 관심사가 도메인에 침투하면 변경에 취약할 수 있다.

 

DTO에 로직이 존재해도 될까?

• DTO는 단순 데이터 전달을 위해 사용하며, 보통 로직을 가지고 있지 않다.
• 예를 들어 제적 위험자를 정렬하는 경우, 정렬 로직은 보여지는 부분이므로 비즈니스 로직과는 연관이 없다. 단순히 사용자에게 보여지는 부분이므로 View에서 수행하는 것이 적절하다.

 

중복 계산 문제를 중간 결과 객체를 이용해 해결하기

• 제적 위험자를 출력할 때, 출석 유형별 횟수 계산을 중복으로 수행하는 문제가 있었다.
  • 먼저 제적 위험자인지 확인하기 위해 출석 횟수를 계산하고, 제적 위험자인 경우 출석 횟수를 출력하기 위해 또 출석 횟수를 계산한다.
• 출석 횟수 계산을 객체의 필드로 둘까 고민하였지만, 출석이 달라질때마다 매번 재계산해야하고 이미 존재하는 필드인 출석 기록에 대한 부차적 데이터라는 생각이 들어 두지 않았다. 따라서 어떻게 하면 중복 계산을 줄일지 리뷰어에게 물어보았다.
  
  

• 이야기를 듣고 생각해보니 이미 출석 기록으로부터 횟수를 세었는데, 횟수 값을 활용하지 않고 다시 출석 기록 객체를 다시 넘기고 있다.
• 횟수 계산 결과를 저장하는 중간 객체인 AttendanceCounter를 만들어 반환한다면 횟수를 한번만 계산할 수 있다.

public class AttendanceCounter {

    private final Map<AttendanceType, Integer> attendanceByType;

    public AttendanceCounter(final List<LocalDateTime> history) {
        this.attendanceByType = initialize();

        for (LocalDateTime attendanceTime : history) {
            attendanceByType.merge(AttendanceType.from(attendanceTime), 1, Integer::sum);
        }
    }

    private Map<AttendanceType, Integer> initialize() {
        Map<AttendanceType, Integer> attendanceByType = new EnumMap<>(AttendanceType.class);
        attendanceByType.put(AttendanceType.ATTENDANCE, 0);
        attendanceByType.put(AttendanceType.LATE, 0);
        attendanceByType.put(AttendanceType.ABSENCE, 0);
        return attendanceByType;
    }

 

중간 결과 객체를 사용하자!

• 객체지향 설계에서 중복 계산 문제는 캐싱이나 중간 결과 객체(여기서는 AttendanceCounter)를 활용하자!

 

초기화 책임

모든 크루에 대해 12월에 대한 출석 기록을 초기화하는 과정은 아래와 같이 동작한다.

1. Initializer에서 12월 첫 운영 날짜부터 어제 일자까지 결석으로 정의한 Map<Integer, LocalDateTime>를 생성한다.
2. CrewHistory 생성시 해당 출석 기록(Map<Integer, LocalDateTime>)을 저장한다.
3. 파일에서 출석 기록을 읽어와 크루별로 출석을 수정한다.

여기서 초기화하는 과정을 CrewHistories에서 수행할지, 지금처럼 Initializer에 할지 페어와 토론했다.

 

초기화 책임 : CrewHistories vs Initializer로 분리

• CrewHistories : CrewHistories에서 초기화를 하면, 출석 기록에 대한 로직이 모이게 되어 응집도가 높아진다.
• Initializer : 모든 크루에 대해 같은 초기화 동작을 하니 Initializer에서 수행하는 것이 낫지 않을까?
  • CrewHistories가 과도한 책임을 가지므로 Initializer에서 초기화를 수행하는 것이 낫지 않을까?

 

응집도 vs 책임 분리 (SRP)

• 이 부분도 페어와 내가 결론을 내지 못하여 리뷰어에게 물어보았다.

로또 미션에서 생성 책임 vs 응집도와 비슷한 맥락이다.

 

 

• CrewHistories 는 기능과 관련된 비즈니스 로직 (제적 학생 필터링 등)의 책임을 가지고 있고, CrewIntializer는 외부 파일에서 읽어 출석기록을 가져오는 것에 대한 책임을 가진다.
• "CrewHistories 라는 도메인 객체가 초기 데이터 생성에 관한 것을 과연 알고 있어야 할까?" 라는 말이 인상깊었다. 도메인은 도메인의 행위를 중점을 두어야지, 어떤 데이터로 생성되는지 데이터를 중심으로 사고하는 것은 객체지향에 어긋남을 깨달았다.
  • CrewHistories 의 핵심적인 책임은 CrewHistory를 이용해 제적 학생을 찾는 행위이다. 초기 데이터 생성은 따로 분리해도 좋다.

 

결론

• 객체는 협력에서 어떠한 역할을 수행하는지에 따라 책임이 결정된다. 데이터가 아닌 협력에 집중하자!

 

원시값 포장

• 크루들과 함께 열띤 토론을 펼친 주제 중 하나였다. 🔥

요구사항 : "모든 원시 값과 문자열을 포장한다"

• 미션에는 "모든 원시 값과 문자열을 포장한다" 라는 요구사항이 있다.
• 해당 요구사항은 원시값과 문자열을 포장하여 비즈니스 로직을 캡슐화한 객체를 만들기 위해서이다.

 

원시값 포장을 모든 경우에 해야하는가?

• 그런데 미션에 닉네임이라는 문자열이 등장하지만, 요구사항이 없어 단순 getter만 가진 객체가 생성되었다.
• 따라서 단순 데이터를 가진 Nickname 객체가 필요한지 의문을 가진 크루들이 많았다. 나도 그 중 하나였다.

public class Nickname {

    private final String value;

    public Nickname(final String value) {
        this.value = value;
    }

• 리뷰어는 당장 검증이나 비즈니스 로직이 존재하지 않다면 포장하지 않고, 만약 요구사항이 생긴다면 분리한다고 하셨다.

 

 

결론

• 매번 모든 원시값과 문자열을 객체로 생성하여 불필요한 클래스가 많아져 유지보수가 어렵게 하지 말자.
• 요구사항이 생길 경우처럼, 비즈니스 로직이 캡슐화될 필요가 생길 경우 객체를 생성하자!
• 요구사항을 아무 생각없이 지키려고 하기보다, 요구사항의 본래 의도를 이해하고 적절한 수준에서 적용하려고 노력하자!

 

중복 검증 문제

빠른 예외

• 출석 시스템은 입력에 대해 빠른 예외를 발생시키도록 구현되어있다. 따라서 항상 유효한 입력값이 들어온다고 가정할 수 있다.
  • ex) 출석 조회 기능 : 닉네임을 입력받은 단계에서 출석 기록에 존재하지 않은 닉네임이라면 예외가 발생하여 다음 단계로 진행할 수 없다.

 

중복 검증을 통해 견고한 메서드 만들기

@Override
    public void execute(final CrewHistories crewHistories) {
        LocalDate nowDate = LocalDate.now(clock);
        Nickname nickname = makeNickname();

        // 닉네임 검증 
        crewHistories.validateKeyExists(nickname);
        // findHistory에서도 닉네임 검증을 수행해야할까?
        CrewHistory history = crewHistories.findHistory(nickname);

        resultView.showAttendanceHistory(nickname, history, nowDate, campusScheduler);
        showAttendanceCountResult(history, nowDate);
    }

• 닉네임을 입력받은 후, 출석 기록을 조회하는 crewHistories.findHistory(nickname);에서 닉네임이 유효한지 검증하지 않아도 될까? 라는 고민이 들었다.
  • 참고) 닉네임을 입력받아 검증하는 메서드와, 출석 기록을 조회하는 메서드는 다르다.
• 그런데 만약 crewHistories.findHistory(nickname);에서 닉네임 검증을 수행하지 않는다면, 맥락 없이 바로 사용하게 된다면 존재하지 않은 닉네임 예외를 일으킬 수 있다.
  • API 사용자가 매번 모든 코드를 읽어보며 검증이 잘 수행되었는지 확인해봐야하는 귀찮음과 완성도 측면에서 떨어진다는 생각을 하였다.
• 따라서 public 메서드이므로, 안전하게 두번 nickname key 검증을 해주었다.
• 리뷰어에게 물어봤을 때도 public 메서드 자체에서 검증을 모두 수행하는 것이 견고하다라고 답해주셨다. 😊

 

병렬 스트림에서의 findFirst() vs findAny()

• 병렬 스트림에서 findFirst()는 Stream의 가장 앞 요소를 반환한다.
• 병렬 스트림에서 findAny()는 가장 먼저 찾은 요소를 반환한다.
  • 즉, findAny()는 병렬Stream의 뒤쪽에 있는 원소가 리턴될 수 있다.

public static SubjectType from(final Map<AttendanceType, Integer> result) {
    int absentCount = result.get(AttendanceType.ABSENT);
    int lateCount = result.get(AttendanceType.LATE);
    int totalAbsentCount = calculateTotalAbsentCount(lateCount, absentCount);

    return Arrays.stream(SubjectType.values())
            .sorted((subjectType1, subjectType2) -> Integer.compare(subjectType2.threshold, subjectType1.threshold)) // 정렬
            .filter(subjectType -> totalAbsentCount >= subjectType.threshold)
            .findFirst()

• 위 코드에서 만약 스트림 요소를 정렬(sorted())하지 않았다면, enum 순서에 의존하게 된다. 따라서 병렬 처리시에 예상치 못한 결과가 나올 수 있다.

 

PR

https://github.com/woowacourse/java-attendance/pull/77

[1단계 - 출석 구현] 밍트(김명지) 미션 제출합니다. by Starlight258 · Pull Request #77 · woowacourse/java-atte

 

https://github.com/woowacourse/java-attendance/pull/153

[2단계 - 출석 다시 구현하기] 밍트(김명지) 미션 제출합니다. by Starlight258 · Pull Request #153 · woowaco