Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[코드리뷰용] 중간 점검차 브랜치 올려봅니다 #5

Open
wants to merge 88 commits into
base: main
Choose a base branch
from

Conversation

aristo0922
Copy link

No description provided.

@aristo0922 aristo0922 self-assigned this May 15, 2024
Comment on lines 5 to 8
public static void main(String[] args) throws Exception {
MainController main = new MainController();
main.headController();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 Exception이 발생한다면 시스템은 어떻게 되나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 원래 System.err.println 으로 문구 출력한 후 처음으로 되돌아가도록 하려고 했습니다!
근데 알아보다 보니까 이게 성능상으로 문제가 될 수 있다는 내용을 읽어서 더 고민해보고자 일단 이렇게 처리했습니다.
( https://spongeb0b.tistory.com/92 )
혹시 System.err.println 을 사용하는 것에 대해서 어떻게 생각하시는지 여쭤도 될까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.err.println 라는게 있다는 걸 지금 알았어요ㅋㅋ
요 코멘트를 남긴 이유는, Exception이 발생되었을 때 처리해주는 과정이 없는 것 같아서였어요!
throws는 상위 메서드, 그러니까 호출부에서 이 예외를 처리해줄거야~ 라는 키워드인데, main은 저희가 컨트롤할 수 있는 최상위 메서드이니까요!

제가 알기로는 main에서 throws를 하면 JVM이 예외처리를 해주는데, 요 예외 처리가 단순히 콘솔에 출력만 해주고 마는 것으로 알고 있어요. 요 프로그램은 콘솔 프로그램이니, 이 어플리케이션을 사용하는 사용자의 입장에서 생각하면 갑자기 알 수 없는 에러 메세지가 나오는 상황이 되겠죠?

try-catch를 이용해 적절하게 예외 처리 로직을 추가�하는 것은 어떤지 슬쩍 의견 남겨보아요!

Comment on lines +7 to +9
LINE_2("2호선", Arrays.asList("교대역", "강남역", "역삼역")),
LINE_3("3호선", Arrays.asList("교대역", "남부터미널역", "양재역","매봉역")),
신분당선("신분당선", Arrays.asList("강남역", "양재역", "양재시민의숲역"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요건 노선 사전 등록 정보를 enum으로 빼둔 것일까요?
초기 설정 정보를 enum으로 관리하신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음 구현이 우선일 때는 List 형식으로 데이터를 저장했었습니다!
그 상태로 구현할 때 코드 길이가 길어지는 걸 느꼈습니다.
초기화 할 때 역이 생성되고, 노선에 추가되는 부분에서요. 조건문으로 분기를 만들어서 2호선에 ~~역, 3호선에 ~~역, 신분당선은 ~~~역을 삽입한다. 는게 초기 형태였습니다.

그러다보니 초기화 부분에 변동 사항이 생기면 조건문을 찾아서 고쳐야 할 것 같았습니다. 새로운 노선이 생기고 역과 노선 관계가 생길 때마다 if 새로운 노선 then ~~ 역를 반복하는 것 처럼요.

그래서 상수 관리할 때 가장 많이 쓰이는게 enum 이니까 이걸 쓰면 되겟다!! 하는 생각으로 시작했습니다!

먼가 지금 생각해보니 해시맵 같은 거로 관리해도 괜찮을 것 같았다는 생각이 드네욯...

Copy link
Member

@EunjiShin EunjiShin May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 그렇군요!

질문을 드린 것은, �데이터 초기화는 어플리케이션 실행될 때만 한번 동작하고, 이후론 시스템 상에서 재사용될 여지가 없는 값인데 따로 enum으로 만든 분리한 이유가 무엇인지 궁금해서였어요~ enum값 조회 -> 해당 enum에 할당된 리스트를 순회하면서 저장하는 것보다, 데이터를 초기화하는 용도의 클래스를 만들면 좀 더 관리가 편하지 않을까요? 이렇게 하면 데이터 초기화 책임을 분리할 수 있다는 장점도 있어요! 지금은 초기화를 담당하는 객체와, 초기화 데이터를 담당하는 객체가 분리되어 있어 실제 데이터 초기화 과정을 한 눈에 보기 힘드니, 아예 한 곳에 모아두는 거죠.

만약 초기 데이터의 양이 엄청 많아져서 파일 입출력 등으로 대체해야 하는 요구사항이 있다고 가정할게요. 그럼 데이터 초기화 클래스에 파일 읽는 메서드만 새롭게 추가하면 되어요. 이런 식으로, 어떤 새로운 요구사항이 생겼을 때 내가 어디를 건드려야 하는지 << 를 보면서 관련있는 아이들끼리 묶어주면 좀 더 응집도가 올라갈 것 같아요!

또 Arrays.asList를 이용해 만든 리스트는 크기는 고정이지만, 내부 요소를 변경할 순 있다고 알고 있어서 의도하지 않은 값이 세팅되는 이슈도 생길 수 있을 것 같아요. Collections.unmodifiableList를 이용해 불변으로 만들면 어떨까요?

요건 개인적인 스타일이니까 그냥 이런 의견도 있구나~ 하고 참고만 해주셔요! (파일이나 DB없이 초기 데이터를 직접 넣는 경우가 일반적이지 않기도 하고)

Comment on lines +11 to +13
public boolean isNotNumber(){
throw new NumberFormatException(PUT_ONLY_VAILD_NUMBER.getMessage());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SubwayException을 상속받는 별도의 Exception 클래스를 만들지 않고 메서드로 처리한 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

입력값을 받는 부분 에러 핸들러가 한 곳에 모여있으면 좋을 것 같다는 생각에서 시작했습니다!

메서드로 처리하지 않으면 클래스를 만들게 되는데 그러면 각각의 에러마다 클래스가 생기는 건가? 싶더라구요.

그래서 내부 클래스로 생성해보는데 경험이 부족해서 오류를 많이 겪었습니다 ㅠㅠ

무엇보다 사람들에게 코드에 대해 이야기를 나누고 싶다는 생각이 강했습니다.
그리고고 피드백 반영하는 시간이 있으면 좋을 것 같아서 가장 생각하기 쉬운 형태로 구현을 먼저 해봤습니다...!

Copy link
Member

@EunjiShin EunjiShin May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 그렇군요! 에러 핸들러가 왜 한 곳에 모여있으면 좋을까<에 초점을 맞춰봐도 좋을 것 같아요.

이 어플리케이션에서 발생할 수 있는 exception을 한 눈에 보기 위함이라면, 사실 exception 패키지 하위에 각각의 예외 클래스를 위치하는 방식은 어떨까요?

에러마다 클래스가 생긴다면, 새로운 에러가 있을 때마다 새로 클래스를 만들어주어야 해서 생산성이 떨어져보일 수 있어요. 하지만 지금도, 어떤 에러가 생길때마다 새로운 메서드를 생성해야 하니 생산성 차원에서 큰 이점이 있진 않을 것 같아요 🤔

반대로, 만약 exception이 엄청나게 많아지거나, 특정 exception은 내부적으로 어떤 처리를 해주어야 한다면 (ex.중복값 이슈가 발생하면, 에러 메세지에 문제가 된 값을 띄워준다던지) SubwayException 클래스가 너무 거대해져서 오히려 원하는 코드를 찾기 힘들어지는 문제가 생기지 않을까요?

또 코드를 보는 입장에서 반환형은 boolean인데 내부에선 exception만 throw하고 있으니 의아할 것 같기도 하구요!

아령님께서 생각하신 1) 에러 핸들러는 한 곳에 모아두자 2) 에러마다 클래스를 생성하는 건 피하자 < 이 두 가지의 이유와, 클래스/메서드의 장단점을 비교해보면 좋을 것 같아요!


import static subway.controller.ManageController.br;

public class MainController implements Constants {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Application.main 메서드를 보니 여기가 제일 처음 호출되는 곳이더군요!
다만 파일들의 이름만 봤을 땐 수많은 컨트롤러 중 뭐가 제일 먼저 호출되는 것인지, MainController의 역할은 무엇인지 바로 와닿지 않는 것 같아요.

Controller은 MVC 패턴상에서 사용자의 요청을 적절한 곳으로 넘겨주고, 최종 결과를 사용자에게 리턴하는 역할을 수행해요.
MainController은 어플리케이션을 초기화하고, Controller 객체들을 생성하고, 사용자의 입력을 받아 다른 Controller으로 요청을 넘겨주는 역할을 수행하는 것으로 보여요.

이 어플리케이션에서 Controller이 수행해야 하는 역할이 무엇인지에 따라 한 번 더 책임을 분리해주어도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 처음에는 노선별로 controller 를 먼저 분할해서 만들었는데 한 곳에서 관리하는 곳이 있으면 좋을 것 같더라구요.
그래서 mainController 를 만들었고 역할이 와닿지 않는 부분에 대해서는 굉장히 공감합니다.
은연중에 느끼고 있었는데 다른분이 보기에는 역시 크게 느껴지는군요 ㅠㅠ

제일 먼저 호출하는 부분이 MainController 인 만큼
다른 controller 관련 코드는 유틸처럼 보고 이름에서 controller 로 빼면 괜찮을까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네이밍은 너모나 주관적인 의견이지만...! 저라면 아싸리 의미만 담아서 만들 것 같아요.
SubwayManagementSystem 클래스를 만들고, startexecute같은 메서드를 만드는 식으로요!

아니면 Facade 같은 걸 활용해도 좋을 듯 해요. 어차피 요 클래스가 어떤 행위를 하는 건 아니고, 실제로 동작을 할 다른 객체들을 묶어만 주는 역할이니까..! 참고만 해주셔요 :)


public MainController() {
ask = new AskView();
InitManager manager = new InitManager();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitManager 클래스로 타고 넘어가니, InitManager가 생성될 때 초기 데이터가 삽입되는 구조군요!
여기 코드만 봤을 때 내부에서 그런 동작이 이루어지고 있다는 것을 알기 힘든 구조인 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 피드백 너무 좋네요 감사합니다 어떻게 하면 좋을지 고민해보겠습니다!

Comment on lines 36 to 43
public void headController() throws Exception {
while (true) {
ask.printMain();
String node = br.readLine();
if (isEnd(node)) return;
nextStep(node);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do-while문으로 바꾸면 어떨까요? while(true)만 봤을 땐 내부 코드를 전부 읽어야만 탈출 조건을 알 수 있어요.
do-while문을 사용하면 탈출 조건이 비교적 직관적으로 명시되어 있기에 이해하기 쉬운 코드가 될 것 같아요.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextStep이란 메서드명만 봤을 때 앞으로 어떤 일이 벌어질지 알 수 없어서, 의미를 알아채기 애매한 이름인 것 같아요!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do-while문으로 바꾸면 어떨까요? while(true)만 봤을 땐 내부 코드를 전부 읽어야만 탈출 조건을 알 수 있어요. do-while문을 사용하면 탈출 조건이 비교적 직관적으로 명시되어 있기에 이해하기 쉬운 코드가 될 것 같아요.

@EunjiShin

우디님, do-while 문으로 바꾸면 탈출 조건이 직관적으로 명시된다는 것에 대해서 공감합니다!

혹시, 이 코드에서 if (isEnd(node)) return; 이 조건을 do-while 조건문에 넣으신다는 말씀이신지 궁금합니다!

그렇게 된다면 nextStep을 무조건 한번은 실행시켜야 하는 것 같아서 여쭤봅니다! 아니면 코드를 아예 바꾸면 좋겠다는 말씀이신가요!?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시, 이 코드에서 if (isEnd(node)) return; 이 조건을 do-while 조건문에 넣으신다는 말씀이신지 궁금합니다!

오 저는 이렇게 이해했습니다! do while 문 왜 생각을 못했을까요 크앙

nextStep 은 종훈님 말대로 nextStep는 종료되지 않는 이상 실행되어야 하는 프로세스에요!

do while 로 하면서 nextStep을 어떻게 수정할 수 있는지 고민해봐야겟네용 :3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 착각을 했네요😂 반대로 이해해서 의문을 가졌었네요! 이해했습니다👍

Comment on lines 45 to 52
public boolean isEnd(String node) throws IOException {
if (node.equals("Q") || node.equals("q")) {
System.out.println(" 안녕히 가세요. ");
br.close();
return true;
}
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if 블럭에서 프린트도 하고, 뭔가를 close도 하고.. 수행하는 행위가 많은 것 같아요.

  1. br이 정확히 어떤 친구인지 파악할 수 없어요. 매개변수에서도 내부 변수에서도 찾아볼 수 없어서 메서드가 짧은 편임에도 어떤 책임을 가진 메서드인지 파악하기 어려워요.
  2. 마찬가지의 맥락으로 왜 IOException이 전파되는 코드인지 파악하기 어려워요.
  3. Q와 q는 시스템 요구사항에서 주어진 메뉴이니 책임을 분리해도 좋을 것 같아요. 만약 q대신 quit를 사용하도록 요구사항이 바뀌면, q를 직접 찾아서 전부 수정해주어야 할까요? 어느 한 곳만 수정하면 전체 시스템에 뚝딱 반영되는 구조라면 훨씬 안정적일 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분을 가장 초기에 만들고 리팩토링할 때 거의 생각 못했네요...!
책임을 분리할 수 있도록 구조에 대해서 고민해보겠습니다...!

br 이 BufferedReader 라서 throw IOException을 사용했습니다...!
왜 코드를 이렇게 짰는지 모두가 이해할 수 있는 방법을 고민해보겠습니다!!

Comment on lines 63 to 78
public boolean CoreController(int command) {
if (command == STATION_COMMAND) {
return stationController.work(stationController, STATION);
}
if (command == LINE_COMMAND) {
return lineController.work(lineController, LINE);
}
if (command == SECTION_COMMAND) {
return sectionController.work(sectionController, SECTION);
}
if (command == MAP_COMMAND) {
return mapController.work();
}
throw new SubwayException();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조기 리턴을 활용해 조건 분기 복잡도를 낮춰주셨군요! 👍
지금도 충분히 깔끔하고 좋은 코드지만, 요런 점들이 지켜지면 더더 좋을 것 같아요.

  1. command가 지원하지 않는 명령이라는 것을 여기서 검증해야하는가? 입력할 때부터 검사할 수 있다면 좋지 않을까?
  2. STATION_COMMAND와 같은 상수들을 찾기 어렵다! 타고 들어가보니 Targets enum으로 맵핑되던데, 그럼 처음부터 enum을 사용하면 안되는 걸까? 왜 Targets는 시스템 메뉴에 대한 정보를 담고있는데 view인걸까?
  3. 만약 새로운 command가 추가되면 여기 메서드를 필수로 수정해주어야 하는데, 실수로 누락되어도 시스템 상에서 아무런 경고를 보내줄 수 없는 구조인 것 같다! 심지어 자연스럽게 SubwayException으로 빠져서, 사용자 입장에선 제대로 제공되는 메뉴를 선택했는데 exception이 발생하는 상황이 될 것 같다. 어떻게 개선할 수 있을까?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ CoreController라는 메서드명도 의미를 파악하기 어려운 것 같아요! 이 시스템의 맥락을 모르는 사람이 CoreController라는 이름만 들었을 때, 무슨 역할을 하는 아이라고 생각할까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와!! 감사합니다 제가 enum 사용이 아직 익숙치 않아서 이게 맞는 줄 알았어요 ㅎ
더 공부하겠습니다!!

Core 라는 단어로 핵심 알고리즘을 다루고 있는 걸 어필하고 싶었는데 쉽지 않군요... 더 괜찮은 이름을 고민해볼게요!!

Comment on lines 36 to 43
public void headController() throws Exception {
while (true) {
ask.printMain();
String node = br.readLine();
if (isEnd(node)) return;
nextStep(node);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do-while문으로 바꾸면 어떨까요? while(true)만 봤을 땐 내부 코드를 전부 읽어야만 탈출 조건을 알 수 있어요. do-while문을 사용하면 탈출 조건이 비교적 직관적으로 명시되어 있기에 이해하기 쉬운 코드가 될 것 같아요.

@EunjiShin

우디님, do-while 문으로 바꾸면 탈출 조건이 직관적으로 명시된다는 것에 대해서 공감합니다!

혹시, 이 코드에서 if (isEnd(node)) return; 이 조건을 do-while 조건문에 넣으신다는 말씀이신지 궁금합니다!

그렇게 된다면 nextStep을 무조건 한번은 실행시켜야 하는 것 같아서 여쭤봅니다! 아니면 코드를 아예 바꾸면 좋겠다는 말씀이신가요!?

Comment on lines 63 to 77
public boolean CoreController(int command) {
if (command == STATION_COMMAND) {
return stationController.work(stationController, STATION);
}
if (command == LINE_COMMAND) {
return lineController.work(lineController, LINE);
}
if (command == SECTION_COMMAND) {
return sectionController.work(sectionController, SECTION);
}
if (command == MAP_COMMAND) {
return mapController.work();
}
throw new SubwayException();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command가 중복되는 것과 if문의 개수가 늘어난다면 switch 문으로 바꿔도 괜찮을 것 같습니다!

switch 문이 if 문보다 효율적일 때가 있다고 들었었는데 jvm에서 작동을 하는 것으로 찾아봤습니다!

[ Jump Table 관련 ]
https://hee96-story.tistory.com/94

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 저도 사실 switch 문을 쓰고 싶었는데 요구사항에서 쓰면 안된다고 되어 있어서요 ㅠㅠㅠㅠㅠ
사실 그래서 실무에서도 switch 문 잘 안쓰나보다 가볍게 생각하고 넘겼는데 의외로 좋은 친구군요!

kwongwangjae added a commit to kwongwangjae/Infinite_Challenge_BE that referenced this pull request May 25, 2024
- 지하철 역 등록 및 삭제
- 노선에 등록된 역은 삭제 불가
- 중복된 지하철 역 이름이 등록 불가
- 지하철 역 이름은 2글자 이상
- 지하철 역의 목록을 조회 가능

Closes techeer-sv#4, techeer-sv#5, techeer-sv#6, techeer-sv#7, techeer-sv#8
heondong9265 added a commit to heondong9265/Infinite_Challenge_BE that referenced this pull request May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants