[1단계 - 자판기] 유세지(김용래) 미션 제출합니다.

[usageness]

안녕하세요 리뷰어님, 이번 미션의 PR을 드리게 된 유세지입니다! 😊
프로그램의 동작은 데모 페이지에서 확인하실 수 있습니다!
만나뵙게 되어 영광이고, 잘 부탁드리겠습니다!
아래부터는 이번 미션을 하면서 느낀 점과 궁금한 점입니다.

타입스크립트의 강제성

타입스크립트가 변수와 인자의 타입들을 명시적으로 기술 할 것을 강제하다보니 깊이가 있는 객체들을 다룰때도 그 내용을 확인하기 쉬웠고, 개발 중 생길 수 있는 오류를 미연에 방지할 수 있었습니다. 처음 사용해보았지만 오히려 개발자에게 친절하다는 느낌을 받았습니다.

라우터 구현

SPA의 특성을 구현하기 위해 window.location의 메서드들을 활용해보았는데, 굉장히 신기하면서도 즐거운 경험이었습니다. 지식이 부족해 기초적인 단계로만 사용해서 더 좋은 구조를 만들지 못한 점은 아쉬웠습니다. 특히 해쉬뱅(#!)을 최대한 피해보려고 웹팩 설정을 찾아보며 노력했지만 결국 실패했습니다. 혹시 해쉬뱅을 사용하지 않고도 주소를 기반으로 원하는 페이지로 바로 연결할 수 있는 방법이 있는지 궁금합니다.

컴포넌트를 로드하는 구조

라우터에서 특정한 주소를 받으면 이 주소가 보여주어야 하는 페이지를 로드하고, 로드된 페이지는 다시 컴포넌트 단위로 나뉘어진 섹션들을 로드하는 구조입니다. 이러한 구조를 선택한 이유는, 고정된 화면에서 로드해야하는 컨텐츠들의 깊이가 인덱스 - 페이지 - 컴포넌트 의 3단계로 그리 깊지 않고, 변경이나 추가적인 구현이 필요할때 컴포넌트 단위의 수정만으로 페이지를 재구성하기 쉬울 것이라 판단하였습니다. 이러한 방식이 리액트에서 props를 내려주는 구조와도 유사하다고 느꼈지만, 컴포넌트 전체의 리렌더링보단 대체나 추가등의 코드를 사용하여 나름의 최적화를 하였습니다. 리뷰어님이 생각하시기에 이런 방식으로 프로젝트를 구현하는게 적절한 방법이었는지 의견을 여쭤보고 싶습니다.

디렉토리 구조

.
|-- css
|  |-- global.css
|  `-- index.css
|-- docs
|  `-- README.md
|-- index.ts
`-- js
  |-- __test__
  |  `-- vendingMachine.test.js
  |-- components
  |  |-- AddChangeComponent.ts
  |  |-- AddProductComponent.ts
  |  |-- ChangeListComponent.ts
  |  |-- ModifyProductComponent.ts
  |  |-- ProductItemComponent.ts
  |  `-- ProductListComponent.ts
  |-- constants.ts
  |-- interfaces
  |  `-- VendingMachine.interface.ts
  |-- model
  |  |-- VendingMachine.ts
  |  `-- validator.ts
  |-- pages
  |  |-- ChangeAdd.ts
  |  `-- ProductManage.ts
  `-- routes.ts

구조도

다시 한 번 감사드리고, 좋은 하루 보내시길 바라겠습니다! 👏

[Vallista]

안녕하세요 세지님!
1단계를 함께 진행하게 되어 반갑습니다 :)

앞으로 2,3단계 해보면서 함께 성장해나가요!

레이아웃 피드백

없습니다. 깔끔하고 리렌더 없이 잘 만들어 주신 것 같아요!

전반적인 코드 피드백

• 구조도를 적어주셔서 코드를 파악하는데 쉬울줄 알았으나.. 구조도에 맞게 모듈이 잘 분리되어있는 것 같지 않습니다 ㅠ 일단, 처음 index로 진입했을때, routes 로 코드가 합병되어있지 않았으며, 이벤트를 별 다르게 주입하고 있어서 해당 부분은 도메인적 응집이 필요해보입니다.
• 관련해서 코드적인 부분은 리뷰 코맨트를 달아두었으니 확인해주시면 감사하겠습니다!

구조적인 피드백

• 컴포넌트가 현재 컴포넌트 역할을 하는지 확인해보는게 필요합니다. 컴포넌트의 정의는 무엇이고, 어떻게 개발해야 컴포넌트일까요?
• 컴포넌트에 너무 작은 일을 준게 아닐까 싶습니다. 그리고 페이지에서 너무 많은 일을 하고 있는 것 같아요!

질문 & 답변

Q1.

혹시 해쉬뱅을 사용하지 않고도 주소를 기반으로 원하는 페이지로 바로 연결할 수 있는 방법이 있는지 궁금합니다.

github pages를 이용하면, 404 에러 페이지를 기반으로 해시뱅없이 원하는 페이지로 로드시킬 수 있습니다 :)

참고: https://iamsjy17.github.io/react/2018/11/04/githubpage-SPA.html

Q2.

이러한 방식이 리액트에서 props를 내려주는 구조와도 유사하다고 느꼈지만, 컴포넌트 전체의 리렌더링보단 대체나 추가등의 코드를 사용하여 나름의 최적화를 하였습니다. 리뷰어님이 생각하시기에 이런 방식으로 프로젝트를 구현하는게 적절한 방법이었는지 의견을 여쭤보고 싶습니다.

위에서 언급했듯, 지금 component가 너무 일을 적게하고 있습니다. 페이지 단위로 너무나 많은 일을 하고 있어서 이 이들을 밑으로 내려보시는걸 추천드립니다 :)

[Vallista]

이 메타태그는 무얼 나타내나요?

[usageness]

모바일에서 보이는 크기와 배율을 설정할 수 있는 태그네요! 몰랐던 키워드인데 찾아보면서 알게됐습니다 😅

[Vallista]

40px 0 10px

[usageness]

수정했습니다 😊

[Vallista]

축약형으로 쓸 수 있지 않을까요?

[Vallista]

Roboto 가 없은 경우엔 어떻게 되나요?

[usageness]

시스템 기본 글꼴로 표시될 것 같은데, 대체 글꼴을 지정해주지 않았네요.
이 부분은 웹 폰트를 받아 표시해주는 편이 좋을 것 같아 변경했습니다!

/* font.css */
@import url(https://hangeul.pstatic.net/hangeul_static/css/nanum-gothic.css);

* {
  font-family: inherit;
}

body {
  font-family: 'NanumGothic';
}

[Vallista]

NanumGothic이 로드 안되었을때는 어떤 폰트가 로드되나요?
인터넷 환경이 느리다면, 어떤 일이 벌어질까요!?

[usageness]

기본 글꼴로 표시되다가 로드가 끝나고 나서야 폰트가 바뀔 것 같네요...
이 부분은 유저의 사용성을 해칠 수 있을 것 같아 보여요 ㅠㅠ
대안을 찾아보겠습니다!!

[Vallista]

0px vs 0

[Vallista]

어떤게 더 적합할까요?

[usageness]

px를 굳이 넣어줄 필요가 없어 보입네요...! 0으로 수정했습니다😅

[Vallista]

[500, 100, 50, 10] 은 매직넘버이므로, 상단에 전역으로 갖고있는게 좋지 않을까요? getChangeCoin 할 때마다 배열을 할당해서 메모리에 들고 있겠네요.

1. [500, 100, 50, 10] 은 과연 메모리에서 해제가 될까요?
2. 그렇다면 해제 시점은 언제일까요?

[usageness]

[500, 100, 50, 10] 은 매직넘버이므로, 상단에 전역으로 갖고있는게 좋지 않을까요?

맞습니다. 매직넘버를 제거해준다고 했었는데 놓치고 지나쳤네요 😥
상단에서 availableCoinTypeList 라는 이름으로 선언해주었습니다!

[500, 100, 50, 10] 은 과연 메모리에서 해제가 될까요?

제 생각으로는 한참 뒤에야 해제될 것 같습니다.
getChangeCoin() 이 반환값이 함수 내부에 선언된 변수 coins를 참조하고,
getChangeCoin() 을 호출한 makeChangesToCoin() 함수는 재귀적으로 실행되기 때문에
처음 입력된 money가 모두 소진되고 재귀를 시작한 첫 함수가 종료되어야 가비지가 될 것 같네요.
메모리 해제 시점 또한 첫 함수의 종료 이후일 것 같아요.

[Vallista]

코드가 복잡하고 길지 않나요? 이걸 어떻게 축약할 수 있을까요?

[usageness]

축약 방법은 도저히 모르겠네요..ㅠㅠ
다만 아래의 코드들과 동일하게 유효성을 확인하는 로직은 validator.ts으로 옮겨주었습니다.

bb987d2

[Vallista]

축약을 모를땐 도메인으로 나눠주기만 해도, 반 이상은 성공입니다 굿!

[Vallista]

throw new error를 받아주는곳이 어디인가요..? 에러나면 어디로..?

지금은 에러 핸들링 규칙이 있나요?

[usageness]

지금은 에러가 발생할 수 있는 각 함수의 호출 최상단부에서 try ~ catch 문으로 핸들링하고 있습니다.

/* AddProductComponent.ts */
try {
  vendingMachine.addProduct(newProduct);
  this.noticeStateChanged('add', newProduct);
} catch (message) {
  alert(message);
}

이렇게 관리했을때 호출하는 사람이 내부 구현을 모르면
제대로 된 핸들링을 해줄 수 없다는 문제점이 있어보이긴 하는데,
어떻게 해결을 해주어야 할지 감이 오지 않네요... 😥

커스텀 에러 핸들러를 구현해주는 방법도 사용해보았지만
이 경우 최상단까지 에러가 throw 되지 않아 오류가 발생해도
이후의 동작을 모두 막을 수가 없었습니다.

혹시 이런 경우에는 어떠한 방법이 있을까요..?

[Vallista]

catch로 나올때 커스텀 에러 핸들링을 만들어서 해주신 건 너무 좋습니다 :)

일단 에러는 일관적인 커스텀 에러로 주는게 맞고,
커스텀 에러에 이후 어떤 행동이 되어야하는지 몇 가지를 정의해두고, "어떤 에러일때 어떤 행동이 나와야한다"를 받아서 이후 조작을 해주면 좋을 것 같아요. singleton 같은 전역 객체를 만들어서 그 안에서 지금 에러 핸들링 상황이 무엇인지 저장해두고 이후 refresh를 할 지, 모달을 띄울지 뭐 그런것들을 적어주면 좋겠네요.

[Vallista]

ChangeAdd ..? 이름이 적합한가요..? 어떤걸 하려고 하는걸까요?

[usageness]

잔돈(Change) 추가(Add) 를 영어로 직역했더니 이상한 이름이 되어버렸네요...!
(동사 + 명사) 규칙을 따라 AddChange로 이름을 바꾸어주었습니다.

[Vallista]

template string을 써도 좋겠어요

[usageness]

적용했습니다!

[usageness]

안녕하세요, 발리스타님!
우선 피드백 주신 부분들을 위주로 수정해보았습니다.
아래는 변경한 내용과 변경하며 느낀 점, 궁금한 점을 정리해보았습니다.

라우터 구현 수정

이전의 라우터는 그 역할 자체는 훌륭하게 수행하고 있었지만, 라우터를 임포트 하여 사용하는 곳에서는
메서드들의 표현력도 부족하고 history state도 함께 관리해서 정작 라우터가 해야하는 일들을 나눠 갖고 있었습니다.
이러한 부분들을 라우터 내부로 옮기고, 바깥에서는 표현력 있는 메서드 만으로 라우팅을 동작할 수 있게 수정했습니다.

컴포넌트에 역할 부여

기존의 template 정도의 역할만 수행하던 컴포넌트를 내부의 구현과 로직까지 스스로 처리하도록 역할을 다시 부여해주었습니다.
이제 컴포넌트 자체만으로 다른 곳에서도 재사용이 용이해진 형태가 된 것 같습니다.
덕분에 페이지는 기존에 부여되어 있던 과중한 역할들이 사라졌고,
각각의 컴포넌트들의 상태 변화를 감지해 하위 컴포넌트에 전달하는 역할 정도만 수행하게 되었습니다.

+ 이 부분을 리팩토링하면서 느꼈는데, 각 컴포넌트에 유사하게 반복되는 구조들이 있었습니다.
예를 들어 render 메서드나, 상위 페이지(또는 컴포넌트)에 변경을 전달하는 기능, 또는 상위 페이지로부터
변경에 대한 신호를 받아 컴포넌트 스스로를 re-rendering 하는 기능들이 있었습니다.
이러한 부분들은 추후에 추상 클래스로 만들어 상속받아 구현하는 형태로 만들어도 괜찮을 것 같다는 생각이 들었네요.

에러 핸들링

지난 미션에서는 에러 핸들링을 따로 구현하여 메시지들을 관리했었는데, 이번 미션에서는 에러의 발생 지점도 다양하고
명령의 뎁스도 2~3단계를 거치다보니 기존에 알던 방법으로는 전체적인 흐름을 제어하기가 어려웠습니다.
문제가 있다는 것을 어렴풋하게 느끼고는 있지만, 어떻게 수정해야할지 감이 오지 않아 손을 대지 못했습니다.
혹시 이러한 문제는 어떻게 해결할 수 있을까요? 실무에서는 에러 핸들링을 어떤 방식으로 관리하나요?

마지막으로, 꼼꼼한 피드백 정말 감사드립니다!

[Vallista]

안녕하세요 세지님~

먼저, 코드가 도메인 단위로 정리가 되어 좋습니다!
1단계에서 피드백 드릴 요건은 다 드린 것 같고,
2단계에서 이후 기능을 개발해보시면서 유지보수가 되고,
개발될 때의 상황을 한 번 봐보시면 좋을 것 같아요!

어떤 점이 지금 구조로인해 문제가 되었고, 확장성이 부족했는지요!