[2단계 - 출석 다시 구현하기] 후유(김대동) 미션 제출합니다. #165
Conversation
| if (dateStr.length() < 2) { | ||
| dateStr = "0" + dateStr; | ||
| } | ||
| return new AttendanceRecord(LocalDate.parse(Current.getStringOfThisMonth() + "-" + dateStr), null); |
There was a problem hiding this comment.
문자열을 이용하면 로직을 이해하기 어렵고 유지보수도 어려워지는 것 같아요. 문자열을 만들지 말고 다른 방법으로 LocalDate를 만들 순 없을까요? null도 가능하면 사용하지 않는 게 좋습니다. 해당 값을 사용하는 곳마다 null check를 해줘야 하고 깜빡하고 검증을 빼먹으면 NPE 발생 가능성도 높구요. 가능하면 null 값을 사용하지 마시고 반드시 필요하다면 Optional을 사용하는 것이 좋습니다. 물론 Optional도 사용하면 모든 곳에서 null check를 해줘야해서 가독성이 떨어지고 사용이 불편해서 가능하면 안 쓰는 게 좋긴 합니다..!
There was a problem hiding this comment.
컨트롤러에서 문자열을 파싱하여 LocalDate, LocalTime을 주입하여 생성하도록 수정해보겠습니다.
null check에 대한 코드 길이 증가로 가독성 하락의 단점을 보니 null 사용을 최대한 자제해야겠네요.
| MONDAY(DayOfWeek.MONDAY, LocalTime.of(13, 0)), TUESDAY(DayOfWeek.TUESDAY, LocalTime.of(10, 0)), WEDNESDAY( | ||
| DayOfWeek.WEDNESDAY, LocalTime.of(10, 0)), THURSDAY(DayOfWeek.THURSDAY, LocalTime.of(10, 0)), FRIDAY( | ||
| DayOfWeek.FRIDAY, LocalTime.of(10, 0)); |
There was a problem hiding this comment.
줄바꿈도 컨벤션입니다!
| MONDAY(DayOfWeek.MONDAY, LocalTime.of(13, 0)), TUESDAY(DayOfWeek.TUESDAY, LocalTime.of(10, 0)), WEDNESDAY( | |
| DayOfWeek.WEDNESDAY, LocalTime.of(10, 0)), THURSDAY(DayOfWeek.THURSDAY, LocalTime.of(10, 0)), FRIDAY( | |
| DayOfWeek.FRIDAY, LocalTime.of(10, 0)); | |
| MONDAY(DayOfWeek.MONDAY, LocalTime.of(13, 0)), | |
| TUESDAY(DayOfWeek.TUESDAY, LocalTime.of(10, 0)), | |
| WEDNESDAY(DayOfWeek.WEDNESDAY, LocalTime.of(10, 0)), | |
| THURSDAY(DayOfWeek.THURSDAY, LocalTime.of(10, 0)), | |
| FRIDAY(DayOfWeek.FRIDAY, LocalTime.of(10, 0)); |
There was a problem hiding this comment.
가독성을 위한 줄바꿈 추가했습니다.
코드 작성 완료 후 코드정렬 기능을 자주 사용하는데 연속해서 2번 사용하면 최대한 한 줄로 정렬이 되려고 하네요.
커밋 전 잘 확인해야겠습니다. 알려주셔서 감사합니다! 😄
style: 가독성을 위한 줄바꿈 추가
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class NickName { |
There was a problem hiding this comment.
아마 NickName과 WarningCrew 등은 모든 원시값을 포장하라는 요구사항을 지키기 위함인 것 같은데 맞을까요? 원시값을 객체로 포장했을 때 어떤 장단점을 느끼셨나요?
There was a problem hiding this comment.
객체의 검증과 책임을 분리할 수 있는 장점이 있지만
이번 프로그램에서 NickName과 WarningCrew는 아무런 추가 동작 없이 요구사항에 의해 감싸기만 했네요...
다시 생각해보면 없어도 되는 객체가 아닌가 생각이 듭니다.
WarningCrews 객체에서 Map<NickName, AttendanceStatusCount> 과 같이 사용한다면 WarningCrew는 없어도 될 것 같습니다.
다음 미션에서는 바로 포장하지 않고, 원시값이 검증 로직이나 동작을 가지게 될 때 리팩토링으로 원시값 포장을 해 보겠습니다. 생각 해 볼만한 주제 알려주셔서 감사합니다 ❤️
README.md
Outdated
There was a problem hiding this comment.
그리고 미션의 필수 요구사항 제적 위험자 확인에서 전날까지의 크루 출석 기록을 바탕으로 제적 위험자를 파악한다. 기능과
출석 기록이 없으면 결석 처리 하는 기능을 구현하지 못하고 제출했습니다.
따라서 프로그램을 실행하여 4번 기능을 동작시켜도 존재하는 출석 객체들을 토대로 경고, 면담, 제적 상태가 계산됩니다.
변경에 유연하고 가독성이 좋고, TDD 사이클을 지키며 코드를 작성하는 것도 좋지만 가장 중요한 건 요구사항대로 돌아가는 프로그램을 만드는 것입니다. 어떤 부분들 때문에 위 기능들을 구현하지 못하셨나요? 시간이 부족했다면 어떤 부분에서 시간을 많이 소모했나요? 어디서 시간을 줄일 수 있을지 파악해보시고, 앞으로는 요구되는 기능을 모두 만족하는 프로그램을 만드는 걸 1번 목표로 잡아보면 더 좋을 것 같습니다 :)
There was a problem hiding this comment.
TDD 사이클을 적응해 나가는 과정에서 시간이 많이 걸린 것 같습니다.
다음 미션에서는 요구 사항 구현 완료를 최우선 목표로 노력하겠습니다.
좋은 말씀 감사합니다 😄
README.md
Outdated
There was a problem hiding this comment.
- tdd 사이클 자체를 모두 커밋하기?
- 깃 커밋을 잘게 나눠서 TDD Red 실패 테스트 작성 후 test 태그로 커밋
- TDD Green 성공하게 리팩터링 후 refector 태그로 커밋 이렇게 진행해야 할까요?
- 실패하는 Red 테스트를 성공하는 테스트로 고쳤으니 fix 사용하기?
이런 부분에 대해 리뷰어님께서는 어떻게 표현하실지 궁금합니다.
저는 커밋을 너무 나눠도 코드 변경 이력을 보기에, 추후 작업을 변경하거나 되돌릴 때 불편하다고 생각해요. 그런 의미에서 실패 테스트 작성까지 커밋을 나누는 게 불필요해 보이더라구요. 저는 주로 기능 단위로 나눠서 테스트와 프로덕션 코드를 함께 커밋하고 있습니다. test와 상관 없이 기능을 구현했다면 feat, 코드 리팩터링을 진행했다면 refactor를 사용할 것 같네요!
There was a problem hiding this comment.
기능 단위로 커밋을 한다면 해당 커밋이 변경을 되돌릴만한 지점이 되겠네요.
이 지점이 커밋 할 부분인가? 에 대한 고민이 많았는데 도움이 되었습니다. 감사합니다! 😄
src/main/java/util/DateUtil.java
Outdated
| public static boolean isDayOff(Attend attend) { | ||
| return isDayOff(attend.getDay()); | ||
| public static List<Integer> getAttendAbleDates(int day) { | ||
| return Stream.iterate(1, i -> i + 1) |
There was a problem hiding this comment.
각 변수명이 좀 더 자세한 의미를 가지도록 i와 함께 매개변수로 들어오는 day의 이름도 변경했습니다.
네이밍에 좀 더 신경쓰겠습니다. 알려주셔서 감사해요 😆
refector: 매직 넘버로 사용하던 날짜를 Current 객체를 사용하도록 수정
src/main/java/util/DateUtil.java
Outdated
| public static boolean isTimeOff(Attend attend, LocalTime startTime, LocalTime endTime) { | ||
| return attend.time.isBefore(startTime) || attend.time.isAfter(endTime); | ||
| private static boolean isWeekdays(int day) { | ||
| DayOfWeek dayOfWeek = LocalDate.of(2024, 12, day) |
There was a problem hiding this comment.
2024, 12는 결국 Current 내에 위치한 현재 날짜와 같은 거죠? 그럼 Current 값을 사용하는 게 변경에 영향을 덜 받겠네욥.
There was a problem hiding this comment.
프로그램이 2024년 12월이 아닌 다른 날짜로 변경이 될 경우 위 코드가 문제가 될 수 있겠네요.
Current를 사용하도록 수정했습니다. 알려주셔서 감사합니다!
refector: 매직 넘버로 사용하던 날짜를 Current 객체를 사용하도록 수정
| } | ||
|
|
||
| private String formatWarningStatusShort(WarningStatus warningStatus) { | ||
| if (warningStatus == WarningStatus.WARNING) { | ||
| return "경고"; | ||
| if (warningStatus.equals(WarningStatus.EXPEL)) { |
There was a problem hiding this comment.
enum을 equals로 비교하는 것과 ==로 비교하는 것은 어떤 차이가 있나요?
"제적", "면담" 등의 문자열은 출력과 관련된 것으로 보고 enum에 정의해두지 않고 view에 위치시키신거죠? 👍
There was a problem hiding this comment.
equals 메서드를 사용하면 런타임 중 Null pointer Exception 에러가 발생할 수 있습니다.
equals를 호출하는 enum이 null일 경우 발생합니다.
지금 작성한 코드의 경우 warningStatus 매개변수가 null 일 경우 NPE가 발생할 위험이 있겠네요.
도메인이 출력 형식의 책임을 가지지 않도록 신경 쓴 점 알아봐 주셔서 감사합니다 👍
| assertAll(() -> assertThatThrownBy(() -> { | ||
| attendanceManager.attend(nickName, attendanceRecord); | ||
| }).isInstanceOf(IllegalArgumentException.class), () -> assertThatThrownBy(() -> { |
There was a problem hiding this comment.
| assertAll(() -> assertThatThrownBy(() -> { | |
| attendanceManager.attend(nickName, attendanceRecord); | |
| }).isInstanceOf(IllegalArgumentException.class), () -> assertThatThrownBy(() -> { | |
| assertAll( | |
| () -> assertThatThrownBy(() -> attendanceManager.attend(nickName, attendanceRecord)) | |
| .isInstanceOf(IllegalArgumentException.class), |
여기도 이런식으로 줄바꿈을 변경하면 훨씬 읽기 좋겠네요.
There was a problem hiding this comment.
IDE의 코드 정렬 기능을 연속해서 2번 사용하면 한 줄에 120자 이내로 이상하게 코드가 정렬이 되네요.
커밋 전 코드 줄바꿈 상태를 확인하고 커밋 하겠습니다.
컨벤션 오류 찾아주셔서 감사합니다. 👍
| assertEquals(1L, attendantCount); | ||
| assertEquals(2L, lateCount); | ||
| assertEquals(3L, absentCount); |
There was a problem hiding this comment.
TDD 잘 챙겨주셨습니다 👍 여기서는 assertAll을 쓸 수 있겠네요. 이렇게 작성하는 것과 assertAll을 사용하는 것의 차이는 무엇일까요?
There was a problem hiding this comment.
assertAll로 여러 개의 검증 메서드를 감싸지 않으면,
검증이 실패한 코드 뒤에 있는 코드들이 동작하지 않고 그대로 실패를 뱉습니다.
예시로 위 코드에서 두 번째에 있는
assertEquals(2L, lateCount);
이 메서드가 실패했다면
그 다음에 있는
assertEquals(3L, absentCount);
위 테스트를 검사하지 않고 실패했다고 알려줍니다.
그러면 한 번의 테스트 실행으로 모든 테스트를 돌려볼 수 없게 됩니다. 😎
(실패하는 2번째 테스트가 통과되도록 수정한 후에 3번째에 있는 테스트를 확인할 수 있음)
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?https://prolog.techcourse.co.kr/studylogs/3980
객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
view 계층에서 메서드 길이가 10이 넘는 코드가 하나 존재합니다.
그 외 원시 값 포장 미비 등이 있습니다.
어떤 부분에 집중하여 리뷰해야 할까요?
객체 줄이기
이번 미션은 TDD 사이클 사용하기, 최소한의 객체를 사용해 코드 작성하기를 목표로 진행했습니다.
그래서 코드 중 더 삭제가 가능한 객체가 있을지 궁금합니다!
미구현한 기능들
그리고 미션의 필수 요구사항
제적 위험자 확인에서전날까지의 크루 출석 기록을 바탕으로 제적 위험자를 파악한다. 기능과출석 기록이 없으면 결석 처리 하는 기능을 구현하지 못하고 제출했습니다.
따라서 프로그램을 실행하여 4번 기능을 동작시켜도 존재하는 출석 객체들을 토대로
경고,면담,제적상태가 계산됩니다.깃 커밋 태그 사용
refector: 동일 날짜의 출석 기록을 확인하는 테스트 케이스 추가
이 과정에서 깃 커밋 태그를 어떤 것을 사용할지 고민이 있습니다.
TDD 사이클이 테스트 실패 -> 테스트 성공 -> 리팩터링 과정을 진행하는데
위 커밋은 (실패하는 테스트 작성 + 테스트가 성공하도록 도메인 작성)을 묶은 것입니다.
여기서 Test, Refector, Fix 이 셋중 무엇을 써야할지 고민했습니다.
저는 수정 기능을 이전에 구현(feat) 했고, 이 수정 기능은 같은 동작은 하지만 도메인 코드를 수정해서 refector를 썼습니다.
다른 떠오른 생각으로는
이런 부분에 대해 리뷰어님께서는 어떻게 표현하실지 궁금합니다.
컨트롤러 분리를 해야 할까요?
컨트롤러를 기능 단위로
메서드 분리했지만클래스 본문이 제가 보기에도 엄청 비대해보이는데요.이렇게 사용해도 괜찮은지, 아니면 또 다른 작은 단위의 클래스로 분리해야 할지 궁금합니다.
함수형 인터페이스 잘 사용했을까요?
이번에 처음으로 함수형 인터페이스를 공부하고 적용해봤습니다.
분명 프로그램은 돌아가지만,,, 이렇게 사용하는 것이 맞는지 잘 모르겠습니다.
코드 리뷰 해 주셔서 감사합니다!