[2단계 - 출석 다시 구현하기] 코기(장재현) 미션 제출합니다. #109
Conversation
- 출석 기록 생성 로직 추가 - 출석 기록 얘외 로직 추가
- 출석 결과 도출 기능 추가 - 출석 결과 도출 기능 테스트
- 출석 기능 추가 기능 추가 - 출석 기능 추가 기능 테스트
- 해당 날짜 존재 여부 확인 기능 테스트
- 해당 날짜 출석 기록 존재 검증 추가
- 이름 검증 로직 추가 - 출석 추가 기능 추가
- 크루 존재 여부 확인 로직 추가 - 특정 크루 출석 기록 추가 기능 추가 - 크루 존재 여부 확인 로직 테스트 - 특정 크루 출석 기록 추가 기능 테스트
- 메뉴 입력 기능 추가 - 닉네임 입력 기능 추가 - 출석 시간 입력 기능 추가
- 출석 결과 출력 기능 추가
- csv 파일 읽고 Map 반환 기능 추가
- 메뉴 입력 기능 추가 - 출석 기능 추가
- LocalDateTime을 LocalDate, LocalTime으로 분리
- 특정 날짜에 기록을 가져오는 로직 추가 - 특정 날짜에 기록이 업을 경우 예외 발생
- 특정 날짜 수정 기능 추가
- 휴일 체크 로직 추가
- 특정 기준 전날까지 기록을 저장한다.(csv 파일에 없으면 결석 처리) - Crew, Crews 생성자 변경
- Crew, Crews 도메인 출석 수정 기능 추가 - Crews 출석 수정 기능 테스트
- 수정 결과 출력 기능 추가 - 수정 결과 dto 생성
- 닉네임 입력 기능 추가 - 수정 날짜 입력 기능 추가 - 수정 시간 입력 기능 추가
- 공휴일은 출석 불가 검증 추가
- 제적 상태 조회 기능 추가
- 출석 상태 저장 - 제적 상태 조회 기능 추가
- 특정 시점 이전 기록인지 확인하는 메서드 추가
- 예외 발생 시 메뉴 재입력 - 예외 발생 시 메시지 출력
- 오늘의 날짜 2024년 12월로 변경 기능 추가 - input으로 들어온 시간을 LocalTime,LocalDateTime으로 변경하는 기능 추가 - LocalDateTime, LocalDate, LocalTime을 지정된 형태로 포매팅하는 기능 추가
- 메뉴 선택 예외 처리 기능 추가 - 재입력 기능 추가
- 숫자 검증 추가 - 시간 형태 검증 추가
- username 포장 객체 생성 - username 검증 기능 추가 - 테스트 수정
- 캠퍼스 운영 시간이 맞는지 확인하는 로직 추가
- 월,일 정보를 MonthDay 클래스로 변경
|
|
||
| import java.time.LocalTime; | ||
|
|
||
| public enum CampusOpenTime { |
There was a problem hiding this comment.
Holiday의 경우에는 출석도메인외에도 범용적으로 쓸 수 있는 느낌이라 constans에 두신것도 어느정도 이해가되는데요. 🤔
CampusOpenTime의 경우 출석이라는 도메인에 중요한 요소 중 하나라고 생각되는데요. 🤔
constant 패키지에두신 이유가 있을까요?
There was a problem hiding this comment.
앗.. domain 패키지에 CampusOpenTime이 있는 줄 알았습니다. 제 실수로 constant로 둔거 같습니다.. 좋은 지적 감사합니다.
| Map<AttendanceResult, Integer> statics = histories.stream().collect(Collectors.groupingBy( | ||
| AttendanceHistory::getAttendanceResult, | ||
| () -> new EnumMap<>(AttendanceResult.class), | ||
| Collectors.summingInt((e) -> 1) | ||
| )); |
There was a problem hiding this comment.
stream의 체이닝 메서드들은 한 줄에 하나의 메서드만 쓰도록해보면 어떨까요? 😃
가독성적인 측면에서도 더 좋고 체이닝 메서드를 3개이상 호출할 경우 각 메서드 체이닝을 호출했을 때의 결과 타입을
IntelliJ가 보여주기 때문에 조금 더 코드를 파악하는데에도 도움이된다고 생각해요. 😃
| Map<AttendanceResult, Integer> statics = histories.stream().collect(Collectors.groupingBy( | |
| AttendanceHistory::getAttendanceResult, | |
| () -> new EnumMap<>(AttendanceResult.class), | |
| Collectors.summingInt((e) -> 1) | |
| )); | |
| Map<AttendanceResult, Integer> statics = histories.stream() | |
| .collect(Collectors.groupingBy( | |
| AttendanceHistory::getAttendanceResult, | |
| () -> new EnumMap<>(AttendanceResult.class), | |
| Collectors.summingInt((e) -> 1) | |
| )); |
There was a problem hiding this comment.
확실히 로키의 코드가 스트림의 가독성 장점을 돋보이게 하는거 같습니다! 생각해보니 요구사항에서 한 줄에 . 하나만 찍는다는 점도 위배하고 있네요! 수정해보도록 하겠습니다!
| this.time = time; | ||
| } | ||
|
|
||
| public static boolean cantAttendance(LocalTime attendanceTime) { |
There was a problem hiding this comment.
출석이 불가능하다는 뜻인걸까요? can't를 의미한 것 같은데, cant라고 표현하니 조금 헷갈리는 것 같아요
cant를 canNot으로 바꾸는건 어떨까요?
There was a problem hiding this comment.
맞습니다! 그 의도로 이름을 지었는데 canNot이 확실한 의미전달에 도움이 되는거 같습니다!
| int thisTotalCount = absenceCount * 3 + lateCount; | ||
| int otherTotalCount = o.absenceCount * 3 + o.lateCount; | ||
| return Integer.compare(otherTotalCount, thisTotalCount); |
There was a problem hiding this comment.
이 부분은 default 정렬 로직을 정의하신 것으로 보이는데요. 🤔
아마도 결석횟수를 지각횟수로 환산한 뒤, 지각 횟수의 총 합을 기준으로 정렬을 하고있는 것 같아요.
이 기준을 나중에 변경될 가능성이 높다고 생각하는데 (ex. 순수 결석횟수 기준으로 정렬)
이런 default한 정렬 요소는 Comparable 인터페이스를 구현하기 보다는 필요할 때, 직접 sorting 기준을 명시하거나 혹은 필요할 떄, Comparable 인터페이스를 구현하도록해주는게 좋다고 생각해요.
(찾지를 못했는데, AttendanceAnalyze를 sorting 하는 부분이 있나요? 불필요하다면 이 정렬 로직을 제거해주면 어떨까 싶어요)
There was a problem hiding this comment.
나중에 변경될 가능성이 높다고 생각하는데
이 생각을 놓쳤네요..! 추가록 변경이 아니라 다른 기준으로 정렬을 해야하는 경우에는 문제가 발생할거 같습니다!
(찾지를 못했는데, AttendanceAnalyze를 sorting 하는 부분이 있나요? 불필요하다면 이 정렬 로직을 제거해주면 어떨까 싶어요)
Crews의 getExpulsionCrewsWithOrders 메서드에서 사용하고 있습니다!
private List<Crew> getExpulsionCrewsWithOrders(LocalDate standard) {
return crews.stream().filter(crew -> crew.isExpulsionTarget(standard))
.sorted(new Comparator<Crew>() {
@Override
public int compare(Crew o1, Crew o2) {
AttendanceAnalyze attendanceAnalyzeO1 = o1.getAttendanceAnalyze(standard);
AttendanceAnalyze attendanceAnalyzeO2 = o2.getAttendanceAnalyze(standard);
int compare = attendanceAnalyzeO1.compareTo(attendanceAnalyzeO2);
if (compare == 0) {
return o1.getUsername().compareTo(o2.getUsername());
}
return compare;
}
})
.toList();
}
| @DisplayName("특정 시간 전날들의 출석 기록 테스트") | ||
| public void findCrewHistoryTest() { | ||
| @DisplayName("특정 크루 출석 기록 추가하는 테스트") | ||
| void addAttendanceHistory() { |
There was a problem hiding this comment.
(질문) TDD를 진행하면서 crews의 addAttendanceHistory(특정 크루의 출석 기록을 추가하는 메서드) 테스트 만드려고 했습니다(RED 단계). 하지만 // then 에서 검증을 할 때 정상적으로 잘 들어갔는지 확인하는 로직이 필요했습니다.
그래서 이때 테스트를 위한 메서드(특정 크루의 출석 기록을 가져오는 기능)를 추가해서 검증을 해야할지 고민이였습니다. 물론 운이 좋게도 특정 크루의 출석 기록을 가져오는 기능은 추후에 사용되기에 상관없지만 만약 테스트를 위한 메서드가 추후 도메인 로직에서 사용하지 않으면 테스트를 위한 메서드가 존재해 좋지 않다고 생각이 들었습니다.
해당 상황에서 그렇다면 getter를 통해 확인하는 것이 맞는지 혹은 그 외에 테스트 방법이 있을지 궁금합니다!
코치님께 여쭤봤을 땐 테스트를 위한 생성자를 만들어서 equal 메서드로 해결할거 같다고 하셨습니다. 그런데 의문이 든 점이 테스트를 위한 메서드는 지양해야하는데 테스트를 위한 생성자는 괜찮은건지 의문입니다. 로키의 의견은 어떨지 궁금합니다!
저희가 테스트 코드를 작성하는 이유는 무엇일까요?
아마도 실제 동작하는 코드가 잘 돌아가는지를 확인하기 위해서라고 생각해요.
결국 테스트를 위해서 테스트를 위한 메서드와 테스트를 위한 생성자를 생성하는 것은 실제 동작하는 코드가 아니라고 생각해서 저는 지양하는 편이에요. 특히 테스트를 위한 생성자의 대안으로 fixture 함수(더 나아가서 fixture 함수를 모은 Fixture Class)를 잘 만들어서 활용할 수 있다고 생각해요. 😃
저는 검증을 위해 별도 시그니처를 만들기 보다는 getter를 통해 확인하는 것이 좋다고 생각해요.
There was a problem hiding this comment.
결국 테스트를 위해서 테스트를 위한 메서드와 테스트를 위한 생성자를 생성하는 것은 실제 동작하는 코드가 아니라고 생각
저도 로키와 같은 생각이여서 더 고민이되었습니다..! 그래서 다른 분들은 어떻게 이런 상황을 대체하는가 대화를 나눠보면서 저만의 기준을 만드려고 노력하고 있습니다!(아직 확실한 기준은 찾지 못했습니다 ㅠㅠ) 로키의 의견 감사합니다!🥹
Fixture 함수, Fixture 클래스는 처음 들어보는 개념이네요! 새로운 개념도 알려주셔서 감사합니다!
| public void editAttendanceHistory(LocalDateTime time) { | ||
| deleteAttendanceHistory(time); | ||
| histories.add(new AttendanceHistory(time)); | ||
| public AttendanceAnalyze getAttendanceAnalyze(LocalDate standard) { |
There was a problem hiding this comment.
getAttendanceAnalyze랑 getBeforeAttendanceHistories 는 둘 중에 하나만 있어도 되지 않을까요?
거의 똑같은 동작을하고있는 것 같은데, 단순히 랩핑을 하냐 안하냐의 차이인 것 같아서요.
둘 다, 랩핑한 객체를 전달해주면 안되는걸까요? 🤔
There was a problem hiding this comment.
아 이 부분 동감합니다! AttendanceAnalyze 객체에 출석 정보도 같이 들고 있어서 한번에 전달하면 좋을거 같다고 생각했습니다. 그런데 객체 지향 생활 체조 원칙에서 인스턴스 변수를 3개까지만 사용하라고해서 의식적으로 분리를 했습니다. 리팩토링하면서 출석 기록을 인스턴스 변수로 가지고 지각, 출석, 결석 수를 메서드로 계산하도록 하여 한번에 처리가 가능하도록 설계해보도록 하겠습니다!
| LocalDateTime standardTime = LocalDateTime.of(time.getYear(), time.getMonthValue(), time.getDayOfMonth(), 0, 0); | ||
| return attendanceTime.isBefore(standardTime); | ||
| private AttendanceResult findAttendanceResult(LocalDate attendanceDate, LocalTime attendanceTime) { | ||
| return domain.AttendanceResult.findAttendanceResult(attendanceDate, attendanceTime); |
|
|
||
| private final LocalDate attendanceDate; | ||
| private final LocalTime attendanceTime; | ||
| private final AttendanceResult AttendanceResult; |
There was a problem hiding this comment.
지금보니 오타가있군요.
상태명 첫 글자를 소문자로 변경해주셔야할 것 같아요. 👀
| private final AttendanceResult AttendanceResult; | |
| private final AttendanceResult attendanceResult; |
| private final String result; | ||
|
|
||
| AttendanceResult(String result) { | ||
| this.result = result; | ||
| } | ||
|
|
||
| public static AttendanceResult findAttendanceResult(LocalDate attendanceDate, LocalTime attendanceTime) { |
There was a problem hiding this comment.
findAttendanceResult 메서드에 대한 테스트가 누락된 것 같네요.
추가 부탁드릴게요. 🙏
|
|
||
| public class Crew implements Comparable<Crew> { | ||
| private final String userName; | ||
| public class Crew { |
There was a problem hiding this comment.
Crew의 public 메서드에 대한 테스트가 대부분 누락된 것 같은데요.
아마도 대부분 단순히 상태로 가진 객체들에게 포워딩하는 메서드이기 때문에 테스트가 불필요하다고 생각하신게 아닐까 싶어요. 🤔
지금은 다소 번거롭고 불필요한 테스트라고 생각할지 모르겠지만 추후 팀원들이 이 코드를 수정하게됐을 떄는 도움이 많이 될거에요.
There was a problem hiding this comment.
맞습니다. 사실 현재 Crew 객체의 역할이 다른 도메인의 메서드를 호출하는 일만하기에 다른 도메인의 메서드만 테스트하면 충분하다고 판단했습니다!
팀원들 생각까지하면 테스트를 하는게 좋아보입니다! 생각치도 못한 관점입니다! 로키의 피드백 덕분에 관점이 넓어집니다 👍
- 출석 기록 인스턴스 변수 지정 - 출석, 지각, 결석 횟수 조회 메서드 추가
| return crews.stream().filter(crew -> crew.isExpulsionTarget(standard)) | ||
| .sorted(new Comparator<Crew>() { | ||
| @Override | ||
| public int compare(Crew o1, Crew o2) { | ||
| AttendanceAnalyze attendanceAnalyzeO1 = o1.getAttendanceAnalyze(standard); | ||
| AttendanceAnalyze attendanceAnalyzeO2 = o2.getAttendanceAnalyze(standard); | ||
| int compare = Integer.compare(attendanceAnalyzeO2.calculatePenaltyPoints(), | ||
| attendanceAnalyzeO1.calculatePenaltyPoints()); | ||
| if (compare == 0) { | ||
| return o1.getUsername().compareTo(o2.getUsername()); | ||
| } | ||
| return compare; | ||
| } | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
Comparator는 함수형 인터페이스니까 익명 클래스 방식 대신에 람다식을 활용해도 될 것 같네요. 😃
| return crews.stream().filter(crew -> crew.isExpulsionTarget(standard)) | |
| .sorted(new Comparator<Crew>() { | |
| @Override | |
| public int compare(Crew o1, Crew o2) { | |
| AttendanceAnalyze attendanceAnalyzeO1 = o1.getAttendanceAnalyze(standard); | |
| AttendanceAnalyze attendanceAnalyzeO2 = o2.getAttendanceAnalyze(standard); | |
| int compare = Integer.compare(attendanceAnalyzeO2.calculatePenaltyPoints(), | |
| attendanceAnalyzeO1.calculatePenaltyPoints()); | |
| if (compare == 0) { | |
| return o1.getUsername().compareTo(o2.getUsername()); | |
| } | |
| return compare; | |
| } | |
| }) | |
| .toList(); | |
| return crews.stream().filter(crew -> crew.isExpulsionTarget(standard)) | |
| .sorted((o1, o2) -> { | |
| AttendanceAnalyze attendanceAnalyzeO1 = o1.getAttendanceAnalyze(standard); | |
| AttendanceAnalyze attendanceAnalyzeO2 = o2.getAttendanceAnalyze(standard); | |
| int compare = Integer.compare(attendanceAnalyzeO2.calculatePenaltyPoints(), | |
| attendanceAnalyzeO1.calculatePenaltyPoints()); | |
| if (compare == 0) { | |
| return o1.getUsername().compareTo(o2.getUsername()); | |
| } | |
| return compare; | |
| }) | |
| .toList(); |
| return crews.stream() | ||
| .filter(crew -> crew.getUserName().equals(username)).findAny() | ||
| .orElseThrow(() -> new IllegalArgumentException("[ERROR] 존재하지 않은 크루입니다.")); | ||
| private LinkedHashMap<Username, AttendanceAnalyze> createExpulsionCrewsInfo(LocalDate standard, |
There was a problem hiding this comment.
하위 구현체인 LinkedHashMap을 반환 타입으로 두기 보다는 상위 타입인 Map을 반환 타입으로 두면 좋을 것 같아요. 😃
| private static AttendanceResult getMondayAttendanceResult(LocalTime attendanceTime) { | ||
| if (attendanceTime.isAfter(LocalTime.of(MONDAY_ATTENDANCE_HOUR, ABSENCE_MINUTES))) { | ||
| return ABSENCE; | ||
| } | ||
| if (attendanceTime.isAfter(LocalTime.of(MONDAY_ATTENDANCE_HOUR, LATE_MINUTES))) { | ||
| return LATE; | ||
| } | ||
| return ATTENDANCE; | ||
| } | ||
|
|
||
| private static AttendanceResult getDefaultAttendanceResult(LocalTime attendanceTime) { | ||
| if (attendanceTime.isAfter(LocalTime.of(OTHER_ATTENDANCE_HOUR, ABSENCE_MINUTES))) { | ||
| return ABSENCE; | ||
| } | ||
| if (attendanceTime.isAfter(LocalTime.of(OTHER_ATTENDANCE_HOUR, LATE_MINUTES))) { | ||
| return LATE; | ||
| } | ||
| return ATTENDANCE; | ||
| } |
There was a problem hiding this comment.
지금의 로직은 출석요일에 매우 의존적인 코드가 될 것 같아요.
예를들어, 현재 화수목금은 출석 시간이 같지만 이 출석시간이 요일별로 달라진다거나 혹은 출석 종료 시간이을 지난다거나.. 등의 상황을 생각해보면 이해가 빠르게 되실 것 같아요. 😃
각 출석 요일별 출석 시작시간 , 종료시간을 알고있는 객체가 있고, 이 객체에게 정상 출석, 지각, 결석 여부를 확인해달라고 메시지를 보내본다면, 지금처럼 메서드를 분리할 필요 없이 하나의 메서드로 관리할 수 있을 것 같네요.
Step 1에서의 DailyAttendanceTime를 떠올리시면 될 것 같아요. 😉
이미 1단계에서 다뤄본 적이 있으니, 해당 코멘트는 금방 이해하셨으리라 생각됩니다. 😃
| } | ||
|
|
||
| public static AttendanceStatus findAttendanceStatus(final int lateCount, final int absenceCount) { | ||
| int totalCount = absenceCount + (lateCount / 3); |
| } | ||
|
|
||
| public static AttendanceStatus findAttendanceStatus(final int lateCount, final int absenceCount) { | ||
| int totalCount = absenceCount + (lateCount / 3); |
There was a problem hiding this comment.
totalCount 라는 표현이 조금 어색하게 느껴지는 것 같아요.
생각해보면 (최종?, 보정된?) 결석 횟수 정도의 의미를 가진 것 같은데, 이 의미가 포함되면 좋을 것 같아요.
AttendanceStatus 객체의 상태 값도 count로 두셨군요?
이런 네이밍은 출석 도메인에 익숙하지 않은 사람이라면 코드를 파악하기가 많이 어려웠을 수도 있겠다는 생각이 들어요.
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
규칙 4: 한 줄에 점을 하나만 찍는다.해당 규칙을 지키지 못한 코드가 존재한다.
어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요 로키!😊 step1에서 로키 덕분에 제 코드에 대해 의구심을 가지고 생각하는 기회를 얻을 수 있었습니다. 감사합니다 👍
step1에서 머지된 후 마지막에 코멘트 받은 부분에 대해서도 적용했습니다!
👨💻 피드백 적용
🤔 궁금한 점
하지만 로키의 말대로 현재는 08:00 ~ 23:00만 출석 가능하지만 추후에 해당 요구사항이 변경이 되면 큰 문제가 발생할거라 생각하고 수정을 했습니다.
수정한 방법으로는 LocalDate와 LocalTime 으로 분리 후 LocalTime이 null인 경우 결석으로 간주하도록 했습니다.
하지만 null을 이용한 방법은 null 포인트 예외와 메서드를 사용하는 입장에서 위험하다고 판단했습니다. 그래서 LocalTime을 리턴하는 메서드인 경우 Optional을 통해 null이 있을 수 있음을 알렸습니다. 또한 LocalTime을 이용한 로직은 최소화하고 만약 사용한다면 null 체크를 통해 예외를 방지했습니다.
현재는 결석을 처리하는 방법이 이정도 밖에 생각나지 않네요 ㅠㅠ 사실 이렇게 null로 처리하는 것도 문제가 있다고 생각합니다..
현재 선택한 방법에 대해 로키의 생각과 혹시 추천하시는 다른 방법이 있을지 피드백을 받고 싶습니다!
TDD를 진행하면서 crews의 addAttendanceHistory(특정 크루의 출석 기록을 추가하는 메서드) 테스트 만드려고 했습니다(RED 단계). 하지만 // then 에서 검증을 할 때 정상적으로 잘 들어갔는지 확인하는 로직이 필요했습니다.
그래서 이때 테스트를 위한 메서드(특정 크루의 출석 기록을 가져오는 기능)를 추가해서 검증을 해야할지 고민이였습니다. 물론 운이 좋게도 특정 크루의 출석 기록을 가져오는 기능은 추후에 사용되기에 상관없지만 만약 테스트를 위한 메서드가 추후 도메인 로직에서 사용하지 않으면 테스트를 위한 메서드가 존재해 좋지 않다고 생각이 들었습니다.
해당 상황에서 그렇다면 getter를 통해 확인하는 것이 맞는지 혹은 그 외에 테스트 방법이 있을지 궁금합니다!
코치님께 여쭤봤을 땐 테스트를 위한 생성자를 만들어서 equal 메서드로 해결할거 같다고 하셨습니다. 그런데 의문이 든 점이 테스트를 위한 메서드는 지양해야하는데 테스트를 위한 생성자는 괜찮은건지 의문입니다. 로키의 의견은 어떨지 궁금합니다!