Conversation
KoSeonJe
left a comment
KoSeonJe
left a comment
There was a problem hiding this comment.
안녕하십니까~~~ 고생하셨습니다.
코드리뷰를 많이 하지 않았는데, 너무 많아져서 여기까지 멈추려고 합니다!
수정하고 다시 요청해주세요!
핵심 변경 요구사항이 있습니다~
- Service Repository 제거하기. 이유도 같이 생각해보면 좋을 것 같아요
- 변수명 신경쓰기. 줄여쓰지 않으면 좋겠어요~
- 남겨놓은 리뷰 토대로 객체에 책임 부여해줘 좋은 객체 만들기.
이 pr은 객체지향을 공부하는 목적이어서 객체가 상호작용하는 코드를 중심으로 작성해주면 좋을 것 같아요~!
| app.run(); | ||
| } | ||
|
|
||
| private static LateRule createLateRule() { |
There was a problem hiding this comment.
이 메서드가 어플리케이션의 실행 진입점인 Application의 책임일까요?
지각 규칙을 만들어내는 책임을 분리하면 좋을 것 같네요~
| Map<DayOfWeek, LocalTime> start = new EnumMap<>(DayOfWeek.class); | ||
| start.put(DayOfWeek.MONDAY, LocalTime.of(13, 0)); | ||
| Stream.of(DayOfWeek.TUESDAY, DayOfWeek.WEDNESDAY, DayOfWeek.THURSDAY, DayOfWeek.FRIDAY) | ||
| .forEach(d -> { | ||
| start.put(d, LocalTime.of(10, 0)); | ||
| }); | ||
|
|
||
| return new LateRule( | ||
| start, | ||
| Duration.ofMinutes(5), // 지각 5분까지 | ||
| Duration.ofMinutes(30) // 결석 30분까지 | ||
| ); |
There was a problem hiding this comment.
자바 라이브러리 DayOfWeek를 직접 사용해도 좋지만,
도메인 성격을 가진 enum클래스를 새로 만들어서 정의하면 도메인 객체만 보고 요구사항을 더 쉽게 알 수 있지 않을까요?
| start.put(DayOfWeek.MONDAY, LocalTime.of(13, 0)); | ||
| Stream.of(DayOfWeek.TUESDAY, DayOfWeek.WEDNESDAY, DayOfWeek.THURSDAY, DayOfWeek.FRIDAY) | ||
| .forEach(d -> { | ||
| start.put(d, LocalTime.of(10, 0)); | ||
| }); | ||
|
|
||
| return new LateRule( | ||
| start, | ||
| Duration.ofMinutes(5), // 지각 5분까지 | ||
| Duration.ofMinutes(30) // 결석 30분까지 |
There was a problem hiding this comment.
13, 10, 0 , 5, 30이 뭔지 도메인 성격을 가진 enum클래스 추가하면 알아보기 쉬울 것 같네요
| var lateRule = createLateRule(); | ||
| var schoolDay = new SchoolDayPolicy(); | ||
| var absent = new AbsentPolicy(); | ||
| var riskPolicy = new RiskPolicy(3, 2, 3, 5); |
There was a problem hiding this comment.
3, 2, 3, 5 숫자들은 무엇인가요?
상수가 아니라 지역변수에 선언이라도 했다면, 가독성이 더 좋지 않을까요?
| var repo = new InMemoryAttendanceRepository(); | ||
| Set<String> members = CsvBootstrapper.load(repo, | ||
| Paths.get("src/main/resources/attendances.csv")); |
There was a problem hiding this comment.
어떤 repo인지 명확하게 정의해주면 좋을 것 같아요~
메서드 내에서 재사용할 때, 선언된 곳을 되돌아봐야하는 불편함이 생길 것 같아요
| var before = sheet.records().get(cmd.date()); | ||
| if (before == null) { | ||
| throw new IllegalArgumentException("해당 날짜의 기록이 없습니다."); | ||
| } | ||
|
|
||
| var afterSheet = repository.save(sheet.fix(cmd.date(), cmd.time())); |
There was a problem hiding this comment.
cmd.date()로 sheet의 출석 내역을 수정하는게, sheet의 책임같은데
sheet 객체 내부로 옮기면 좋을 것 같아요 ~!
| return afterSheet.records().get(cmd.date()); | ||
| } | ||
|
|
||
| public HistoryView history(Nickname nickname, AttendanceDate until) { |
| public HistoryView history(Nickname nickname, AttendanceDate until) { | ||
| requireRegistered(nickname); | ||
| var sheet = repository.findByNickname(nickname).orElseGet(() -> CrewAttendance.empty(nickname)); | ||
| var filled = sheet.fillUntil(until, absence); |
There was a problem hiding this comment.
타입이 명확하지 않은데, var키워드를 사용해도 괜찮을까요?
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class AttendanceService { |
There was a problem hiding this comment.
전체적으로 service는 view를 반환하지 않도록 해야할 것 같아요?
왜그럴까요?
There was a problem hiding this comment.
이 부분도 서비스가 입출력 단에 의존하지 않아야 하는 이유랑 비슷한 것 같습니다. View의 포맷 형식이 달라지면 역시 서비스에도 변경이 필요하기 때문입니다.
| var rec = sheet.records().get(date); | ||
| if (rec == null) { | ||
| return Optional.empty(); | ||
| } |
|
아 그리고 전체적인 클래스 다이어그램도 그려보며 의존성 확인해보면 좋을 것 같구요~ |
2 participants
No description provided.