[2단계 - 출석 다시 구현하기] 다로(이정민) 미션 제출합니다. #122
Conversation
|
PR보낼 때 원래 어떤 부분에 집중하여 리뷰해야 할까요? 아래에 TDD 관련 설계에 대한 내용들을 다 적었었는데 제가 잘못눌러서 삭제되었는지 안 보이네요.. 다시 여기에 작성해서 여쭤보자면 항상 요구사항을 다 읽고 클래스 설계를 먼저 한 후 작업을 들어가는 방식으로만 진행하였으나, 이번에 TDD로 코드를 짜면서 README.md에 기능명세서를 자세하게 작성하고 기능들을 하나하나 TDD로 만들면서 자연스럽게 객체들을 생성하고 분리하는 식으로 step2를 진행했습니다. 이런 방식은 처음이다보니 TDD를 통한 여러 기능을 구현하면서 클래스 분리하는 과정에서 클래스 설계가 자꾸 이상하게 꼬인다는 생각이 자꾸 들었습니다. 이 부분에 대하여 다른 크루들이랑도 이야기를 나눠보니 어떤 크루는 미리 기능명세서를 작성하고 클래스 설계까지 끝낸 다음 그 이후에 TDD를 진행하였고, 어떤 크루는 저와 비슷하게 TDD를 하며 red-green 이후 refactor 과정에서 클래스 분리를 진행한다는 이야기를 들었습니다. 여러가지 의견들이 있어 현재 TDD 와 클래스 설계 부분에서 갈피를 못 잡는 중인데 이와 관련해서 조언해주실게 있으시다면 부탁드리겠습니다! |
| private final String state; | ||
| private final int attendanceJudgementTime; | ||
| private final String attendanceStatus; | ||
| private final int thresholdMinutes; |
There was a problem hiding this comment.
이 부분 step1 피드백 받고 수정했었는데 step2 시작하면서 다시 처음부터 프로그래밍 하니까 원래대로 돌아가버렸네요..
enum class도 하나의 객체라고 생각해주시면, 객체에게 메시지를 보내는 방식이 자연스러워지실 것 같아요. 😃
안 익숙한게 당연한거고 익숙해지기위해 미션을 수행하는 것이니, 차차 익숙해지실거에요. 😉
src/main/java/model/Student.java
Outdated
| final String name; | ||
| final AttendanceTimeRecord attendanceTimeRecord; | ||
| final AttendanceStatusRecord attendanceStatusRecord; | ||
| final AttendanceStatusCount attendanceStatusCount; |
There was a problem hiding this comment.
앗... 지금은 DB 설계에 대한 이야기를 하려는 의도는 아니였어요.
단순히 필요한 시점에 계산하면될 것 같은데, 굳이 이 값들을 미리 저장해둘 필요가 있는지에 대한 이야기였어요.
지금의 시점에서 DB나 성능적인 부분은 중요하게 생각하지 않으셔도 될 것 같습니다. 😃 (물론 코멘트를 드리면서 가끔은 이런 이야기가 나올 수도 있지만 😅)
| @@ -1,34 +1,66 @@ | |||
| # java-attendance | |||
|
|
|||
| ## 기능 목록 | |||
| - [X] src/main/resources/attendances.csv 파일을 이용하여 필요한 정보를 가져와 저장한다. | |||
There was a problem hiding this comment.
(질문)
다시 여기에 작성해서 여쭤보자면 항상 요구사항을 다 읽고 클래스 설계를 먼저 한 후 작업을 들어가는 방식으로만 진행하였으나, 이번에 TDD로 코드를 짜면서 README.md에 기능명세서를 자세하게 작성하고 기능들을 하나하나 TDD로 만들면서 자연스럽게 객체들을 생성하고 분리하는 식으로 step2를 진행했습니다.
이런 방식은 처음이다보니 TDD를 통한 여러 기능을 구현하면서 클래스 분리하는 과정에서 클래스 설계가 자꾸 이상하게 꼬인다는 생각이 자꾸 들었습니다.
이 부분에 대하여 다른 크루들이랑도 이야기를 나눠보니 어떤 크루는 미리 기능명세서를 작성하고 클래스 설계까지 끝낸 다음 그 이후에 TDD를 진행하였고, 어떤 크루는 저와 비슷하게 TDD를 하며 red-green 이후 refactor 과정에서 클래스 분리를 진행한다는 이야기를 들었습니다.
여러가지 의견들이 있어 현재 TDD 와 클래스 설계 부분에서 갈피를 못 잡는 중인데 이와 관련해서 조언해주실게 있으시다면 부탁드리겠습니다!
개인적인 생각으로는 두 가지 방식 모두 가능한 접근 방식이라 생각해요.
그래서 상황에 따라서 유동적으로 두 가지 방식중 하나를 선택하는게 맞다고 생각하지만
도메인이 처음부터 명확하지 않거나, 작은 기능부터 점진적으로 개발하는 경우에 TDD를 진행하면서 객체 설계를 발전시켜나가는 방식이 어울린다고 생각하는데, 보통은 이 경우가 많다고 생각합니다.
레거시 코드를 갈아 엎는 작업을 한다거나한다면 주요 도메인 객체를 먼저 정의하고, 이후에 세부적인 로직을 TDD로 채워나가는 방식으로 진행해볼 수도있을 것 같아요.
PR보낼 때 원래 어떤 부분에 집중하여 리뷰해야 할까요? 아래에 TDD 관련 설계에 대한 내용들을 다 적었었는데 제가 잘못눌러서 삭제되었는지 안 보이네요..
어떤 부분에 집중하여 리뷰해야 할까요? 아래에 작성을 하셨는데 삭제가 된 것 같다는 말씀이신거죠?
이 부분은 저도 원인을 잘 모르겠네요... 🤔
| .filter(student -> student.calculateTotalAbsentCount() | ||
| >= AttendancePenalty.COUNSELING.getThresholdAbsenceCount()) |
There was a problem hiding this comment.
student.isCouseling() 이라고 메시지를 보내볼 수 있을 것 같아요.
또한 AttendancePenalty.COUNSELING의 ThresholdAbsenceCount를 직접 꺼내기 보다는 totalAbsentCount 를 받았을 때, 반환하는 AttendancePenalty이 COUNSELING인지를 확인하는 정도로 수정해보면 어떨까 싶어요.
| public static AttendancePenalty findPenaltyByAbsentCount(long totalAbsentCount) { | ||
| return Arrays.stream(AttendancePenalty.values()) | ||
| .sorted(Comparator.comparingInt(AttendancePenalty::getThresholdAbsenceCount).reversed()) | ||
| .filter(attendancePenalty -> totalAbsentCount > attendancePenalty.getThresholdAbsenceCount()) |
There was a problem hiding this comment.
AttendancePenalty 클래스 내부에서 AttendancePenalty 객체의 상태를 가져올 때에는 getter를 쓸 필요 없이 상태 값에 직접 접근할 수 있어요.
| .filter(attendancePenalty -> totalAbsentCount > attendancePenalty.getThresholdAbsenceCount()) | |
| .filter(attendancePenalty -> totalAbsentCount > attendancePenalty.thresholdAbsenceCount) |
| } | ||
| this.attendanceTimeRecords = map; | ||
| } | ||
|
|
There was a problem hiding this comment.
modifyAttendanceTimeRecord, registerAttendanceTimeRecord, putNullLocalTime 을 따로 만들어주실 필요가 있으신가요 🤔 현재 AttendanceTimeRecord 클래스 기준으로 봤을 때는 차이를 잘 모르겠어요.
현재는 Student 객체에서 출석에 대한 유효성 검증을 수행하고 있던데, 사실 �출석에 대한 유효성 검증은 AttendanceTimeRecord 내부에서 수행하는게 맞지 않을까 싶은데, 다로는 어떻게 생각하시나요? 😃
validateDuplicateAttendance(todayDate);
CampusOperatingHours.validateOperatingHours(attendanceTime);
There was a problem hiding this comment.
modifyAttendanceTimeRecord, registerAttendanceTimeRecord, putNullLocalTime
이 메서드들에 대해서는 이런 단순 로직들은 테스트가 필요한가? 에 대해서만 생각하고있었는데 질문해주신 이야기 듣고나니까 굳이 이렇게 따로 나눌필요가 없는게 보이네요
출석에 대한 유효성 검증은 AttendanceTimeRecord 내부에서 수행하는게 맞는 것 같아요! 객체의 역할과 책임에 대해서 더 생각하면서 코드를 만들어봐야겠어요
| class WeeklyAttendanceScheduleTest { | ||
|
|
||
| @Test | ||
| @DisplayName("월요일의 경우 출석 시작 시간 찾기") |
There was a problem hiding this comment.
출석 요일과 주말 및 휴일의 케이스를 묶어서 Parameterized Test를 활용해볼 수도 있을 것 같아요. 😃
| .filter(weeklyAttendanceSchedule -> weeklyAttendanceSchedule.dayOfWeek.equals(localDate.getDayOfWeek())) | ||
| .map(WeeklyAttendanceSchedule::getAttendanceStartTime) | ||
| .findFirst() | ||
| .orElseThrow(); |
There was a problem hiding this comment.
적절한 예외 메시지를 지정해보면 어떨까요?
에러가 발생했을 때, 아무런 에러 메시지도 없다면 에러의 원인을 파악하기가 더 어려워질거에요. 🥲
| } | ||
|
|
||
| public static boolean checkHoliday(LocalDate date) { | ||
| return date.equals(LocalDate.of(2024, 12, 25)) || |
There was a problem hiding this comment.
추후 휴일은 더 늘어날 수 있을 것 같은데, 추후 늘어나는 휴일을 유동적으로 대응할 수 있도록 수정해보시면 어떨까요?
| if (localDate.getDayOfWeek().equals(DayOfWeek.SATURDAY) || | ||
| localDate.getDayOfWeek().equals(DayOfWeek.SUNDAY) || | ||
| localDate.equals(LocalDate.of(2024, 12, 25))) { |
There was a problem hiding this comment.
마찬가지로 너무 특정 휴일에 의존하는 코드라고 생각되는데, 휴일이 추가되더라도 해당 코드가 수정이 없도록 변경해주시면 좋을 것 같아요.
| String expectStudentName = student.getName(); | ||
| assertThat(name).isEqualTo(expectStudentName); | ||
| @DisplayName("제적 위험자 조건에 맞는 크루 찾기 테스트") | ||
| void 재적_위험자_조건에_맞는_크루_찾기_테스트() { |
There was a problem hiding this comment.
현재 이 테스트가 깨지고있는 것 같네요.
통과하도록 수정해주신 뒤, 리뷰 요청 부탁드릴게요. 🙏
|
6시 마감 맞춘다고 하다가 테스트 통과를 신경 안 썻네요 죄송합니다 ㅠ |
| } | ||
|
|
||
| private static String localTimeFormatter(LocalTime localTime) { | ||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("HH:mm"); |
There was a problem hiding this comment.
현재 localTimeFormatter가 호출될 때마다 DateTimeFormatter 객체가 생성될 것 같은데요.
불필요하게 매번 재생성할 필요 없이 상수로 선언한 뒤, 재사용하도록 변경해볼 수도 있을 것 같네요. 😃
| (arr -> arr[0]), | ||
| Collectors.mapping(arr -> LocalDateTime.parse(arr[1], formatter), Collectors.toList()) |
| String filePath = "src/main/resources/attendances.csv"; | ||
| try (BufferedReader br = new BufferedReader(new FileReader(filePath))) { | ||
| return br.lines() | ||
| .skip(1) |
There was a problem hiding this comment.
마찬가지로 1은 Header라서 제끼는거겠죠?
매직 넘버에 대한 처리가 필요해보여요. 😃
| "1. 출석 확인\n" | ||
| + "2. 출석 수정\n" | ||
| + "3. 크루별 출석 기록 확인\n" | ||
| + "4. 제적 위험자 확인\n" | ||
| + "Q. 종료\n"); |
There was a problem hiding this comment.
Java 15에서 처음 나온 Text Blocks를 활용해볼 수도 있을 것 같아요. 😃
| @DisplayName("월요일을 제외한 입력 받은 출석 시간 전에 도착한 경우 출석 상태 가져오기") | ||
| void 입력_받은_출석_시간으로_출석_상태_가져오기() { | ||
| LocalDate today = LocalDate.of(2024, 12, 13); | ||
| LocalTime input = LocalTime.parse("09:03"); |
There was a problem hiding this comment.
잘보면 calculateAttendanceStatus를 검증하는 모든 테스트가 input,expect,result만 다르고 전체적인 구조가 똑같다고 느껴지는데요.
이런 경우에 Parameterized Test를 활용해서 하나의 테스트 케이스로 여러 인자에 대한 테스트를 수행할 수도 있을 것 같아요. 😉
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
일급컬렉션 분리와 이번에 한 줄에 하나의 점만 사용한다에 중점을 두었기에 보통으로 스텝 1보다 점수를 올려보았습니다
어떤 부분에 집중하여 리뷰해야 할까요?