[지하철 노선도] 구현 완료 #1
Conversation
There was a problem hiding this comment.
안녕하세요.
지나가다가 재밌는 챌린지를 한다는 소식을 보고 제가 생각했었던 내용들을 코멘트로 남겨봤어요.
아래 짜잘하게 코멘트를 남기긴했지만 크게 전 두가지 생각을했어요
- 예상치 못한 동작에 대해서 고려가 되어있는가?
- 구현을 표현하고 있는 메서드 명이 적절한가?
개발을 진행하면서 내가 생각치도 않는 예상치 못한 동작이 될 가능성도 열어둬야하기때문에
이번에 챌린지 진행하시면서 테스트코드도 한번 작성해보시면서 내가 의도한 동작대로 각 메서드들이 수행되고 있는지도 점검해보는것도 좋을 것 같습니다.
| public Station(final String name) { | ||
| validateNameLength(name); | ||
| validateNameSuffix(name); | ||
| validateNameRegex(name); |
There was a problem hiding this comment.
생성함수의 기대 값은 특정 값을 넣었을때 해당 값을 기반으로한 객체를 반환하는것이라고 생각해요.
생성함수에 Validation이 들어있을 경우 만약 예외가 발생된다면 이는 Java에서 생각하는 생성함수의 기대와 다르게 동작된다고 생각될 것 같아요.
그렇기에, 팩토리 메서드를 통한 생성으로 수정해보시는건 어떠신가요.
또한, String의 경우 Boxing Type으로 Null값이 허용되는데 만약 Null값 처리에 대해서도 한번 고민해보시는것도 좋겠어요
There was a problem hiding this comment.
- 사실 펙토리 메서드를 사용할시에 나름대로 기준을 세웠는데
- 객체를 생성하는 상황에서 메서드명이 필요한 경우
- 객체를 생성하는 상황마다 각각 주입되는 값이 다른경우
현재 요구사항에 있어서는 단순하고 위와 같은 경우가 없는거 같아 생성자에서 예외처리를 하고 별다른 팩토리 메서드를 선언을 안했습니다.
의빈님께서는 객체가 생성을 하는 담당을 하는 경우 전부 팩토리 메서드를 사용하시는 편인가요??
아래는 예시입니다!
public class Member {
private final String name;
private final Gender gender;
public Member(final String name, final Gender gender) {
this.name = name;
this.gender = gender;
}
public static Member initializeMaleMember(final String name) {
return new Member(name, Gender.MALE);
}
}input == null || input.isBlank()를 추가 하겠습니다😀😀
|
|
||
| private void validateNameRegex(final String input) { | ||
| if (!NAME_PATTERN.matcher(input).matches()) { | ||
| throw new IllegalArgumentException(ErrorMessage.ERROR_PREFIX.toMessage() + "한글만 입력 가능합니다."); |
There was a problem hiding this comment.
메서드에서 동작하는 방향은 아래와 같도 생각했어요
-> input의 패턴비교를 통해 패턴과 일치하는지를 확인
하지만 에러 메시지에는 "한글만 입력 가능합니다."라는 반환이 현재 나와있어 해당 메서드의 결과 값과 일치하지 않는다고 생각이 들었어요.
결과값을 예상할 수 있는 메서드면 읽는 입장에서 더 좋을 것 같아요.
There was a problem hiding this comment.
좋은 에러 메시지에서 글을 읽었는데 스스로 해결할 수 있는 방법을 알려주기를 따라 다음과 같은 에러 메시지를 반환 했던거 같아요. 실제로 해당 메시지가 UI 상으로 전달이 되기도 하고요!
만약 UI에 전달이 안되고 서버에서 에러를 본다면 이름이 NAME_PATTERN과 일치하지 않습니다 라고 생각을 해봤는데 더 적절한 에러메시지 에러 메시지가 있을까요??🤔🤔
There was a problem hiding this comment.
UI를 통한 플레이어가 읽는 입장이 아닌, 코드를 읽는 동료 개발자 시점으로 드렸었던 말씀이었습니다.
어떤 규칙을 검증하는건지가 명확했으면 하는 의미로 말씀드렸어요.
| public List<String> getAllLine() { | ||
| return lineRepository.getAll().stream() | ||
| .map(Line::getName) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
객체를 반환할때 .collect(Collectors.toList())를 사용하는 방법도 있지만, .toList()를 통해 반환하는것도 좋겠어요
Collectors라는 객체 내부에 있는 toList()를 가져올텐데, 기존 stream API 에서도 동일하게 사용가능한 .toList()가 존재해요.
해당 방법으로 바꾼다면 런타임시점에 객체 생성비용에 대해서도 절약할 수 있겠어요.
There was a problem hiding this comment.
해당 미션의 자바 버전이 .toList()가 추가된 java16 버전 이하여서 아쉽게도 해당 메서드를 사용을 못했어요 🥶🥶
런타임 시점 객체 생성 비용은 생각을 못해봤는데 정보 감사합니다!
There was a problem hiding this comment.
| if (mainCommand.equals(MainCommand.PRINT_SUBWAY)) { | ||
| final Subway subway = subwayService.getSubway(); | ||
| outputView.printSubway(subway); | ||
| } |
There was a problem hiding this comment.
현재 모든 메서드가 While(true)라고 선언되어있어요.
프로그램을 시작할때 시작 -> 수행 -> 종료와 같은 기본적인 프로세스 동작을 기대하는데 현재 코드로 살펴봤을때 시작 -> 수행은 보이지만 종료에 대해서는 확인할 수 없다고 생각했어요
또한, startSubWay()라는 메서드명을 봤을땐 지하철 노선 동작의 시작이라는 기대를 하고 있는데 전체 동작을 다 수행한다고 생각이 들어 기대값과 메서드명이 일치하지 않는다고 생각이 들었어요.
There was a problem hiding this comment.
말씀해주신 방법을 고려해봤는데 enum으로 INPROGRESS,END상태를 표현해 while의 조건문의 상태를 변경해주는 방법으로 해결하는건 어떻게 생각을 하시나요?
There was a problem hiding this comment.
탈출조건이 명확하게 보이면 좋을 것 같아요
현재 해당 로직은 시작,수행,탈출에 대한 수행을 최상위로 표현하는 유스케이스 느낌이라고 생각하는데 보다 적절한 네이밍으로 사용되면 좋겠고
해당 메서드에서는 동작의 흐름만을 볼 수 있도록 수정한다면 보다 가독성도 올라갈 것같단 생각이 들었어요
유스케이스가 주는 의미를 생각해보시면 좋겠어요.
| public static StationCommand from(final String input) { | ||
| return Arrays.stream(StationCommand.values()) | ||
| .filter(e -> e.command.equals(input)) | ||
| .findAny() |
There was a problem hiding this comment.
특정 값을 필터링한 이후, findAny()를 통해 값을 찾고있어요.
findAny()를 하게되면 필터링한 값들 중 for과 같은 반복을 통해 내부 필터링된 값을 스캔하여 찾는다고 생각했어요.
그것보다 현재 해당 메서드에서 기대하는건 내가 눌렀던 커맨드 키를 찾는것이니 findFirst가 더 적절하겠어요.
또한, from 이라는 메서드는 특정 값을 통한 인스턴스 생성을 기대하는데, 내부 동작에서는 필터링을 통한 "내가 선택한 커맨드 키"를 찾는과정이 더 강하다고 느껴졌어요.
해당 메서드명도 적절하게 수정하면 좋겠어요.
There was a problem hiding this comment.
- 구현을 할 당시 findAny(), findFirst()를 검색해보았는데 Parallel한 환경이 아닌 이상 둘 중 무엇을 사용하더라도 문제가 없다고 생각을 했어요.. 의빈민 말씀을 들으니 findFirst()가 적절하다고 느껴지네요
- 사실 enum을 사용해서 특정 버튼을 누르면 함수형 인터페이스를 사용해 해당 함수를 호출하는 로직을 작성을 하려고 했는데 static으로 선언을 하지 않는 이상 호출을 할 수 없더라고요..
결국 인스턴스를 반환하는 형식이 되버려 정적펙토리 메서드 네이밍 컨벤션인from()을 사용했습니다. 말씀해 주신것처럼 무슨 과정을 처리하는지 고려하는게 좋은거 같네요
ps. 혹시 메서드명을 적절하게 선정하는 팁같은게 있으신가요?? 🤔🤔
There was a problem hiding this comment.
추가적으로 findAny 내부를 들어가서 살펴보시면 아래와 같은 내용을 찾아보실 수 있어요.
해당 사진을 참고하시면 답을 얻으실 수 있을수도 있겠네요.
| public static StationCommand from(final String input) { | ||
| return Arrays.stream(StationCommand.values()) | ||
| .filter(e -> e.command.equals(input)) | ||
| .findAny() |
There was a problem hiding this comment.
- 구현을 할 당시 findAny(), findFirst()를 검색해보았는데 Parallel한 환경이 아닌 이상 둘 중 무엇을 사용하더라도 문제가 없다고 생각을 했어요.. 의빈민 말씀을 들으니 findFirst()가 적절하다고 느껴지네요
- 사실 enum을 사용해서 특정 버튼을 누르면 함수형 인터페이스를 사용해 해당 함수를 호출하는 로직을 작성을 하려고 했는데 static으로 선언을 하지 않는 이상 호출을 할 수 없더라고요..
결국 인스턴스를 반환하는 형식이 되버려 정적펙토리 메서드 네이밍 컨벤션인from()을 사용했습니다. 말씀해 주신것처럼 무슨 과정을 처리하는지 고려하는게 좋은거 같네요
ps. 혹시 메서드명을 적절하게 선정하는 팁같은게 있으신가요?? 🤔🤔
| if (mainCommand.equals(MainCommand.PRINT_SUBWAY)) { | ||
| final Subway subway = subwayService.getSubway(); | ||
| outputView.printSubway(subway); | ||
| } |
There was a problem hiding this comment.
말씀해주신 방법을 고려해봤는데 enum으로 INPROGRESS,END상태를 표현해 while의 조건문의 상태를 변경해주는 방법으로 해결하는건 어떻게 생각을 하시나요?
| public Station(final String name) { | ||
| validateNameLength(name); | ||
| validateNameSuffix(name); | ||
| validateNameRegex(name); |
There was a problem hiding this comment.
- 사실 펙토리 메서드를 사용할시에 나름대로 기준을 세웠는데
- 객체를 생성하는 상황에서 메서드명이 필요한 경우
- 객체를 생성하는 상황마다 각각 주입되는 값이 다른경우
현재 요구사항에 있어서는 단순하고 위와 같은 경우가 없는거 같아 생성자에서 예외처리를 하고 별다른 팩토리 메서드를 선언을 안했습니다.
의빈님께서는 객체가 생성을 하는 담당을 하는 경우 전부 팩토리 메서드를 사용하시는 편인가요??
아래는 예시입니다!
public class Member {
private final String name;
private final Gender gender;
public Member(final String name, final Gender gender) {
this.name = name;
this.gender = gender;
}
public static Member initializeMaleMember(final String name) {
return new Member(name, Gender.MALE);
}
}input == null || input.isBlank()를 추가 하겠습니다😀😀
|
|
||
| private void validateNameRegex(final String input) { | ||
| if (!NAME_PATTERN.matcher(input).matches()) { | ||
| throw new IllegalArgumentException(ErrorMessage.ERROR_PREFIX.toMessage() + "한글만 입력 가능합니다."); |
There was a problem hiding this comment.
좋은 에러 메시지에서 글을 읽었는데 스스로 해결할 수 있는 방법을 알려주기를 따라 다음과 같은 에러 메시지를 반환 했던거 같아요. 실제로 해당 메시지가 UI 상으로 전달이 되기도 하고요!
만약 UI에 전달이 안되고 서버에서 에러를 본다면 이름이 NAME_PATTERN과 일치하지 않습니다 라고 생각을 해봤는데 더 적절한 에러메시지 에러 메시지가 있을까요??🤔🤔
| public List<String> getAllLine() { | ||
| return lineRepository.getAll().stream() | ||
| .map(Line::getName) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
해당 미션의 자바 버전이 .toList()가 추가된 java16 버전 이하여서 아쉽게도 해당 메서드를 사용을 못했어요 🥶🥶
런타임 시점 객체 생성 비용은 생각을 못해봤는데 정보 감사합니다!
- 지하철역 교대역, 강남역 등 명시 내용 등록 - 지하철 노선 2호선, 3호선, 신분당선 등록 - 노선 초기 설정 역 등록 Closes techeer-sv#1, techeer-sv#2, techeer-sv#3
AppConfig: DI를 담당하는 클래스ExceptionHandler: 예외처리를 위한 클래스~Command: 버튼을 검증하는 역할을 하는 클래스~Message: 메시지를 모아두는 클래스로 둘려했으나 유지 보수 관점에서 코드상에서 메시지를 던져주는 것이 효율적이라 생각해 Prefix만을 선언했습니다.SubwayController: InputView, OutputView, SubwayService를 각각 호출하는 클래스InputView: 입력화면을 담당하는 클래스OutputView: 메시지 출력을 담당하는 클래스SubwayService: domain상에 협력이 필요로하는 로직을 담고 있는 클래스구현을 하는 과정에서 요구사항에 Repository를 사용하라는 부분이 고민이 많았습니다.
@Repository에 나와 있는것 처럼 결국 객체 컬랙션을 관리하는 저장소 개념으로 생각하고 구현을 하였습니다.
다른분들은 어떻게 생각을 하셨나요?
ps. 왜 기본 요구조건으로 변수며 메서드며
static을 사용한걸까요?