[지하철 노선도] 임규리 - 리뷰 부탁드리겠습니다! #124
Conversation
- 노선이 포함하는 역들을 연결 리스트로 관리 - 구간 등록/삭제 용이
…찾는 로직을 구현한다. - findStation() : Optional 객체를 반환
- stations.md 파일을 처리하여 역들을 세팅한다. - lines.md 파일을 처리하여 노선들을 세팅한다.
…yController 객체를 관리한다.
- INVALID_FEATURE("선택할 수 없는 기능입니다.")
- TOO_MANY_INVALID_INPUT("유효하지 않은 입력이 5회 반복되어 프로그램이 종료됩니다.")
…ssThanTwo()로 수정한다.
…ssThanTwo()로 수정한다.
- getFeatureFromInput() : MainFeature에 input과 동일한 값이 존재한다면 반환, 아니라면 에러
- printMain() : 메인 화면 출력 - printSubwayLinesInformation() : 지하철 노선도 출력 (4번 선택지) - printError() : 에러 메세지 출력 - formatLineAndStations() : 지하철 노선도 포맷 형성
- getValidInput() : 에러 발생 시 재입력 유도 (최대 5회)
- start() : 파일을 처리하여 초기 세팅 후 isRunning이 true일 경우 계속해서 프로세스 처리 - getSelectFeature() : 메인 화면 출력 후 입력 받음 - processSelectFeature() : 입력받은 선택지에 따라 이후 프로세스 처리 - showSubwayLines() : 메인 화면 4번 선택지 - 지하철 노선도 출력 - quitProgram() : 메인 화면 Q 선택지 - 종료
- printStationManagement() : 역 관리 화면 출력 - printEnrollStationSuccess() : 역 등록 성공 메세지 출력 - printDeleteStationSuccess() : 역 등록 삭제 메세지 출력 - printStations() : 모든 역 목록 조회 - formatStation() : 역 이름을 출력 문구로 포맷팅
- 오버로딩 - 함수명 수정 : getValidInput -> handleRetry
- boolean -> void 변경 - name으로 Station을 찾았을 경우 Optional이 비어있다면 에러 처리 - 아니라면 (존재한다면) 삭제
- processSelectFeature() -> handledMain() 함수명 변경 - selectMainFeature() 내부에 검증 로직 추가 - handledMain() : 메인 화면의 1번 선택 시 로직 구현 - selectStationFeature() : 역 관리 화면에서 사용자 입력을 처리 - processStationManagement() : 역 관리 화면의 선택지들 구현 - enrollStation() : 역 등록 - deleteStation() : 역 삭제 - showStations() : 역 조회
- ALREADY_IN_LINE("노선에 등록되어 있는 역은 삭제할 수 없습니다.")
- LineService - MainService - SectionService - StationService
- mainService - stationService - lineService - sectionService
- LINE_DOES_NOT_HAVE_THIS_STATION("노선에 해당 역이 존재하지 않습니다.")
- 테스트 커버리지 62% - 파일을 읽어오는 로직 제외
- 테스트 커버리지 100%
- 테스트 커버리지 100%
- 테스트 커버리지 100%
- 테스트 커버리지 100%
- 테스트 커버리지 100%
- 테스트 커버리지 100%
There was a problem hiding this comment.
enum을 활용하여 controller에 대해 singleton을 보장하는 방식이 인상 깊습니다~!
There was a problem hiding this comment.
"Q"와 같은 경우 "QUIT" 이라는 의미를 명확하게 부여했지만, 1이나 2와 같은 경우에 상대적으로 명확하지 않은 것 같습니다.
이를 더 명확하게 할 방법에 대해 더 고민해보면 좋을 것 같습니다.
There was a problem hiding this comment.
감사합니다! 노선 관리와 역 관리를 한 번에 처리하다 보니 더 명확하지 않은 것도 있는 것 같습니당...
수정한다면 ADD, DELETE, SHOW 등으로 수정할 것 같습니다!
| private final LineService lineService; | ||
| private final SectionService sectionService; | ||
|
|
||
| private boolean isRunning = true; |
There was a problem hiding this comment.
프로그램의 종료 값을 멤버변수로 관리하여 코드 간소화의 장점이 있지만, 단점이 더 많은 방법이라고 생각합니다.
어느 시점에 isRunning이 변경되는지를 추적해야하며, 그 과정에서 가독성에도 문제가 생길 수 있을거라고 생각합니다.
handledMain의 입력과 반환값을 더 명확히 하거나, 주석을 통해 설명을 적어둔다면 더 괜찮을 것 같습니다. 어떻게 생각하시나요?
There was a problem hiding this comment.
음 이 부분도 고민이 됐던 부분입니다.
boolean 값을 매번 넘겨주고 반환하는 것이 효율적이지 못하다는 생각이 들어 멤버 변수로 관리하였습니다.
수정한다면 주석을 추가할 것 같습니다!
There was a problem hiding this comment.
Controller의 내용이 많은 것 같습니다. 클래스 분리를 해도 괜찮을 것 같아요..!
There was a problem hiding this comment.
이 부분도 고민됐던 부분입니다 ㅜ.ㅜ
service는 view에 의존하지 않아야 한다는 조건을 지키려다 보니 controller가 역할이 커졌습니다.
또, controller가 controller를 의존하게 하고 싶지 않아 고민하다 하나의 controller에서 처리하게 두었습니다.
결과적으로 아쉬움이 많이 남는 코드입니당...
| } | ||
|
|
||
| private void executeAction(Runnable action) { | ||
| if (action != null) { |
There was a problem hiding this comment.
프로그램사용자가 잘못된 입력을 하면 예외처리를 하고 다시 입력하기 때문에 action은 항상 유효하다고 생각해서(null이 안들어오기에) 지워도 될 것 같습니다!
| actions.put(MainFeature.SELECT_FOUR, this::showSubwayLines); | ||
| actions.put(MainFeature.QUIT, this::quitProgram); | ||
|
|
||
| Runnable action = actions.get(selectedFeature); |
| } | ||
|
|
||
| private void processStationManagement(StationLineFeature selectedFeature) { | ||
| EnumMap<StationLineFeature, Runnable> actions = new EnumMap<>(StationLineFeature.class); |
There was a problem hiding this comment.
StationLineFeature 과 MainFeature, SectionFeature를 각각 key로 사용하기에 애매해서 매번 생성하였습니다ㅜ
하지만 로직이 겹치는 부분이긴 합니다... 좀 더 고민하면 멤버변수로 뺄 수 있을 것 같습니닷
| private final StationService stationService = new StationService(); | ||
| private final LineService lineService = new LineService(); | ||
| private final SectionService sectionService = new SectionService(); | ||
| private final SubwayController subwayController = new SubwayController(inputView, outputView, mainService, |
There was a problem hiding this comment.
저도 프리코스 기간동안 코드리뷰를 하며 알게된 사실인데, 함수 매개변수의 개수가 3개가 넘어가면 안좋다고 합니다. 3개 이하로 줄여보는 것은 어떨까요?
There was a problem hiding this comment.
해당 코드는 의존성 주입을 위한 부분이라고 생각되는데, DI를 적용하지 않고 클래스 내부에서 의존성을 직접 생성하는 것을 지향하시는걸까요??
| @Override | ||
| public String toString() { | ||
| return message; | ||
| } |
There was a problem hiding this comment.
저는 getter를 사용하는 것이 메서드 이름을 통해 명확히 "어떤 값(message)을 가져오는지" 드러낼 수 있어 의도에 맞는 함수라고 생각했기 때문에 getter 대신에 toString을 재정의하셔서 사용한 이유가 궁금합니다.
There was a problem hiding this comment.
다른 코드에서 이렇게 활용하시는 걸 봐서 따라해 본 것인데, 말씀해주신 부분을 생각해보니 getXXX로 사용하는 편이 의도가 확실할 것 같습니다.
|
|
||
| public static void validateNameLength(String input) { | ||
| ValidatorBuilder.from(input) | ||
| .validate(value -> value.length() < 2, ErrorMessage.INVALID_NAME_LENGTH); |
| if (isDuplicate(line.getName())) { | ||
| throw new IllegalArgumentException(ErrorMessage.DUPLICATE_LINE_NAME.toString()); | ||
| } |
There was a problem hiding this comment.
이런 검증 조건은, 저장소에서 처리하기 보다는 Line 이라는 객체를 생성하는 시점에 막아야 한다고 생각하는데 이에 대해서는 어떻게 생각하시나요?
There was a problem hiding this comment.
확실히 그렇네요. Line 도메인이 하는 역할이 너무 적어 고민이었는데 검증을 내부에서 처리한다면 해결될 것 같습니다!
| lines.add(newLine); | ||
| } | ||
|
|
||
| public static void deleteSection(Line line, Station station) { |
There was a problem hiding this comment.
하나의 함수가 너무 많은 역할을 하는 것 같아요! 함수를 분리해보시는 건 어떨까요?
| lines.add(newLine); | ||
| } | ||
|
|
||
| public static Optional<Line> findLine(String name) { |
There was a problem hiding this comment.
Optional로 반환하기 보다는, 해당 메서드에서 Line객체가 null 일 경우, 예외를 터뜨리도록 구현하는 것은 어떨까요?
해당 함수에서 처리한다면, findLine 메서드를 호출하는 다른 메서들에서 null 값을 반복적으로 처리해야하는 중복 작업을 줄일 수 있을 것 같습니다.
| InputValidator.validateDuplicateInput(upperStation, lowerStation); | ||
|
|
||
| return StationRepository.findStation(lowerStation) | ||
| .orElseThrow(); |
There was a problem hiding this comment.
기본적으로, orElseThrow()는 NoSuchElementException을 던지도록 설계되어있기 때문에 새로운 예외인 IllegalArgumentExcepion을 던지도록 하는 작업이 필요해보여요. 그렇지 않을 경우, RetryHandler가 해당 예외를 잡지 못할 것 같습니다.
There was a problem hiding this comment.
생각지 못한 부분인데 감사합니다! 따로 처리를 꼭 해야 할 것 같습니다
| Optional<Station> station = StationRepository.findStation(stationName); | ||
|
|
||
| if (station.isEmpty()) { | ||
| throw new IllegalArgumentException(ErrorMessage.STATION_NOT_EXISTS.toString()); | ||
| } | ||
|
|
||
| stationsOfLine.add(station.get()); |
There was a problem hiding this comment.
이 부분 Optional이 제공하는 api(isPresent)를 사용한다면 더 간단히 정리할 수 있을 것 같아요.
🧐고민했던 점
초기 설정 처리 방법
controller의 역할
테스트 코드를 위한 함수
어떤 도메인이 추가될 수 있을지