[1단계 - 음식점 목록] - 에프이(박철민) 미션 제출합니다.#133
Conversation
iborymagic
left a comment
There was a problem hiding this comment.
안녕하세요 에프이! 고생 정말 많으셨습니다 👏
리뷰가 다소 늦어진 점에 대해 죄송하다는 말씀 드리고 싶습니다.
타입스크립트를 아주 훌륭하게 사용해주셨는데, 타입을 하나하나 지정해줘야겠다는 강박을 버리고 조금만 더 타입스크립트를 잘 활용해보면 더 좋을 것 같습니다. 타입스크립트가 자체적으로 타입을 추론할 수 있는 많은 경우에는 구태여 타입을 지정해주지 않는 편이 불필요한 코드로 인해 가독성이 떨어지는 것을 막을 수 있습니다.
질문에 대해 답변드리면서 자세한 내용 이어갈게요.
- querySelector가 예상치 못하게 null을 return할 수 있는 (동적으로 DOM을 변경한다던가 하는)경우는 얼마든지 있으므로 항상 대비를 해주는 편이 안전합니다.
as나!를 통해 null이 아니라고 냅다 단언해버리면, 예상치 못하게 앱이 터질 수 있고, 디버깅에 난항을 겪을 수도 있으므로 조심해서 사용해야 합니다. 저는 개인적으로 optional chaining도 마찬가지로 디버깅에 곤란을 겪었던 적이 있어서 최대한 자제하는 편이에요. 질문 주신 부분에 대해서는, 저는 보통 if문을 통해 걸러주는 편입니다. - 질문이라기보다는 퀴즈에 가까운 느낌입니다 😅. 지금 상태가 어떤 시도의 결과로써 어떤 상태인지를 설명해주면 좀 더 답변하기가 쉬울 것 같습니다. 안그러면 제가 코드를 다시 한번 전체적으로 훑으면서 수많은 코드들 중 에프이가 질문하신 부분이 어떤 부분이고, 어떤 의도인지 추리해내야 하니까요.
아마 지금 생각해보기로는 컴포넌트를 구현하신 방식에 대해 질문하신 걸로 생각됩니다. (아니라면 다시 한 번 자세하게 질문 주세요) html과 스타일만 컴포넌트화 시키고 내부 로직은 컨트롤러에서 넣어주는 방식인데, 훌륭한 시도라고 생각합니다. 이렇게 구현하면 추후에 동일한 스타일과 다른 기능을 가진 버튼, 드롭다운 등을 추가하기가 편해지실거에요.
Common 디렉토리에 넣어놓으신 컴포넌트들은 그대로 해주시면 될 것 같고, 나머지 컴포넌트들은 오히려 UI 관련 로직들을 포함시켜보는 것도 좋습니다. 비즈니스 로직들을 서비스 레이어로 분리하신 것은 아주 좋으나, UI 상호작용 로직들은 컴포넌트 내부에 들어있는 것이 좀 더 응집성 있는 코드가 될 수 있을 것 같아요. 당장 1단계에서 고민하기에는 너무 늦어질 수 있으니, 2단계 진행하시면서 천천히 고민해보셔도 좋을 것 같습니다.
아래 코멘트로 몇 가지 남겨드렸으니, 확인하시고 다시 리뷰요청 주세요!
.prettierrc.cjs
Outdated
| arrowParens: 'avoid', | ||
| proseWrap: 'never', | ||
| endOfLine: 'auto', | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Prettier 2.0부터는 default 값이 'lf'로 바뀌었다는 것을 확인했습니다. 'lf'로 수정했습니다 :)
| it('모든 속성을 입력한 경우 레스토랑을 추가할 수 있다.', () => { | ||
| const newRestaurant = { | ||
| category: '기타', | ||
| name: '오한수 우육면가', | ||
| distance: 10, | ||
| description: '우육면 + 군만두', | ||
| link: 'www.naver.com', | ||
| }; | ||
|
|
||
| const result = RestaurantService.addRestaurant(newRestaurant, DUMMY); | ||
|
|
||
| expect(result).to.equal(true); | ||
| }); |
There was a problem hiding this comment.
E2E 테스트는 사용자의 관점에서 내가 의도한대로 잘 동작하는지를 테스트합니다. 실제 브라우저에서의 동작을 고려해야 하는 것이죠. 사실 요 테스트는 addRestaurant라는 함수를 테스트한 것에 가깝고, 사용자가 모든 속성을 입력하고 레스토랑을 추가했을 때 내가 화면에 어떤 결과가 나타나기를 의도했는지까지 검사해주시면 좋을 것 같아요.
There was a problem hiding this comment.
1단계는 cypress로 단위테스트를 짜는거라는 얘기를 들었습니다. 요거는 참고만 해주시고 넘어가셔도 될 것 같습니다.
There was a problem hiding this comment.
폴더 이름이 e2e라는 점을 간과했네요..!
단위 테스트만 구현한 상태이지만, cypress로 단위 테스트를 하는 것이 과연 적절할지 의문이 듭니다. jest에 비해 cypress가 상대적으로 느려서 빠르게 확인할 수 없었기 때문인데요, 단위 테스트와 E2E 테스트 모두를 진행할 때는 jest와 cypress를 함께 사용하는 것이 좋을까요?
There was a problem hiding this comment.
의문이 드시는 것이 당연합니다. cypress는 e2e 테스트 도구이기 때문이고, 아마 cypress라는 새로운 도구에 대한 적응이 필요할 테니 테스트 자체는 익숙한 단위 테스트로 짜보라는 것이 의도가 아니셨을까 싶네요.
그렇죠. 단위 테스트와 e2e 테스트는 다른 테스트이니, 각각에 맞는 테스트 도구를 사용해주면 됩니다.
| interface DropdownOption { | ||
| value: string; | ||
| content: string; | ||
| } | ||
|
|
||
| interface DropdownProps { | ||
| options: DropdownOption[]; | ||
| label?: string; | ||
| name?: string; | ||
| id?: string; | ||
| className?: string; | ||
| isRequired: boolean; | ||
| } |
There was a problem hiding this comment.
개인적으로 스크롤이 너무 늘어나지만 않는다면, 연관된 코드들은 가까이 두는 것이 읽기 편하더라구요.
There was a problem hiding this comment.
Restaurant 인터페이스와 그 내부에서 사용하는 type 간의 거리를 고려하지 않았던 것 같습니다😅 이 부분은 더 신경쓰겠습니다!
| const $ = target => { | ||
| return document.querySelector(target); | ||
| }; | ||
|
|
||
| const $$ = target => { | ||
| return document.querySelectorAll(target); | ||
| }; |
There was a problem hiding this comment.
요기서 querySelector가 null일 때를 대비해서 공통적인 처리를 해줄 수도 있겠네요.
There was a problem hiding this comment.
querySelector가 null을 반환할 경우 $ 메서드를 호출하는 곳에서 early return을 하는 것이 적절하다고 생각합니다. callee에서는 별도의 처리가 필요하지 않을 것 같은데, caller가 아닌 callee에서 처리를 해야 하는 이유가 있는지 궁금합니다!
There was a problem hiding this comment.
사실 밖에서도 어차피 early return만 해주고 있기에, 그럴거면 그냥 여기서 early return 해놓고 밖에서 편하게 쓰자 하는 의도였습니다. 하지만 실제로는 early return만으로 충분한 경우도 있지만 아닌 경우도 얼마든지 있을 수 있지요. 요 코멘트는 그냥 넘어가셔도 될 것 같습니다 😅
| this.manageFilterValue(); | ||
| } | ||
|
|
||
| showFilterDropdown() { |
There was a problem hiding this comment.
보통 show는 hide와 대비되어 많이 사용되는 편이라, renderFilterDropdown 같은 이름이 좀 더 보편적일 것 같습니다. (무조건 고치라는 뜻은 아닙니다)
There was a problem hiding this comment.
UI를 화면에 출력하는 함수들을 전부 render로 시작하는 이름으로 관리하고 있기 때문에 이 함수 또한 render를 사용하는 것이 더 좋을 것 같습니다.
추가로 Dropdown 컴포넌트는 비즈니스 로직을 처리하지 않고 UI만을 제공하고 있기 떄문에 controller가 아닌 view에서 관리하는 것으로 수정했습니다.
src/domain/RestaurantService.ts
Outdated
| addRestaurant(restaurant: Restaurant, restaurantList: Restaurant[]): boolean { | ||
| const existingRestaurant = restaurantList.find( | ||
| item => item.category === restaurant.category && item.name === restaurant.name, | ||
| ); | ||
| if (existingRestaurant) { | ||
| return false; | ||
| } | ||
| restaurantList.push(restaurant); | ||
| localStorage.setItem('restaurantList', JSON.stringify(restaurantList)); | ||
| return true; | ||
| }, | ||
|
|
||
| filterByCategory(category: FilteringCategory, restaurantList: Restaurant[]): Restaurant[] { | ||
| if (category === '전체') return restaurantList; | ||
| return restaurantList.filter(restaurant => restaurant.category === category); | ||
| }, | ||
|
|
||
| sortByProperty(property: SortingProperty, restaurantList: Restaurant[]): Restaurant[] { | ||
| return restaurantList.sort((a: Restaurant, b: Restaurant) => (a[property] > b[property] ? 1 : -1)); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
요런 함수들의 return type은 굳이 명시해주지 않아도 타입스크립트가 자동으로 추론할 수 있습니다. (IDE에서 함수명에 마우스를 갖다대보면 return type이 자동으로 추론되고 있을거에요)
사실 상황에 따라 다르긴 한데, 함수 내부 구현의 오류로 인해 외부에서 실수가 발생하는 것을 방지하기 위해서 return type을 지정해주면 편한 경우도 있습니다. 근데 개인적으로 요 정도 함수는 sort, filter 동작이 전부라서 그냥 타입스크립트가 추론하게 냅두는 것이 더 깔끔한 것 같습니다.
There was a problem hiding this comment.
마찬가지로, sortByProperty의 sort 내부의 콜백도 a, b로 변수명을 짓고 타입으로 안내를 하기보다는, 변수명 자체로 가이드를 주고 타입은 지워버리는 것이 더 권장됩니다.
There was a problem hiding this comment.
모든 parameter와 return type을 지정해야 한다는 강박이 있던 것 같습니다. 타입 추론 관련 내용을 추가적으로 공부해보겠습니다!
추가로 함수명과 filter, sort 키워드 위에 마우스를 올렸을 때 return type이 자동으로 추론되고 있는 것을 확인했습니다.
src/domain/RestaurantService.ts
Outdated
| return false; | ||
| } | ||
| restaurantList.push(restaurant); | ||
| localStorage.setItem('restaurantList', JSON.stringify(restaurantList)); |
There was a problem hiding this comment.
요런 localStorage key도 상수화해두면 실수 확률이 줄어들겠죠?
| showAddRestaurantModal() { | ||
| const addRestaurantButton = $('.gnb__button'); | ||
| addRestaurantButton.addEventListener('click', () => { | ||
| OutputView.renderAddRestaurant(this.#restaurantList); | ||
| this.manageFormEvents(); | ||
| this.manageModalEvents(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
요거는 내부 동작은 이벤트리스너를 등록해주는 동작인데, 함수명은 모달을 보여준다는 이름이네요.
There was a problem hiding this comment.
그리고 이 코드대로면 모달을 열 때마다 새로 DOM을 그리고 이벤트리스너를 등록할 것 같은데, 너무 비효율적이지 않을까요?
There was a problem hiding this comment.
이벤트 리스너를 등록하는 다른 메서드들과의 통일성을 위해 함수명을 manageAddRestaurantModal로 수정했습니다!
기존 코드는 innerHTML을 사용했기 때문에 매번 DOM을 다시 그리고 formEvent를 다시 등록하는 구조였습니다. 기존 DOM을 유지한 채 변경하기 위해 replaceChildren()과 insertAdjacentHTML()을 사용했습니다. manageModalEvents()의 경우 프로그램 실행 이후 한 번만 호출되면 되기 때문에 run()으로 위치를 옮겼습니다!
There was a problem hiding this comment.
현재 방식은 모달 container와 overlay가 html에 있는 상태이고, 모달창에 표시될 요소들을 컴포넌트화해서 container의 child를 교체해서 보여주는 방식입니다. 모달을 어떻게 구현하는 것이 좋을지 고민이 많은데요, AddRestaurant 컴포넌트 내부에 상호작용 로직을 포함시키는 것도 고려하고 있기 때문에 step2를 진행하면서 구조가 바뀔 수도 있을 것 같습니다.
| filterContainer.insertAdjacentHTML('beforeend', sortDropdown); | ||
| } | ||
|
|
||
| reload() { |
There was a problem hiding this comment.
밑에서 reload가 등장할 때마다 새로고침을 왜 해주지? 하는 생각을 했는데, 그게 아니었군요. 함수명을 조금 더 명시적으로 바꿔봐도 좋을 것 같습니다.
There was a problem hiding this comment.
reload라는 함수명이 잘못 해석될 수 있겠네요. 함수가 하는 일에 초점을 맞추어 이름을 바꿨습니다.
updateRestaurantList() {
const filteredList = RestaurantService.filterByCategory(this.#category, this.#restaurantList);
const processedList = RestaurantService.sortByProperty(this.#property, filteredList);
OutputView.renderRestaurantList(processedList);
}|
안녕하세요 피터! 에프이입니다 :) 피드백 주신 내용을 바탕으로 수정할 부분은 수정하고, 궁금한 내용들은 코멘트에 질문을 남겼습니다. *️⃣배포 주소https://chysis.github.io/javascript-lunch/dist/ 🔧수정 사항
|
iborymagic
left a comment
There was a problem hiding this comment.
안녕하세요 에프이! 리뷰 반영해주시느라 너무 고생 많았습니다. 👏👏
깔끔하게 잘 구현해주셔서, 리뷰가 잘 반영됐는지 확인하기도 편했던 것 같습니다.
일단 코멘트로 달아주신 내용을 보면 좋은 고민들을 하고 계신 것 같은데, 왜 그런 고민을 했는지가 전혀 나와있지 않아서 아쉬웠어요. 질문 주신 내용을 봐도, 사실 객체로만 놔둬도 충분하다는 근거밖에 보이지 않습니다. 클래스로 구현해야겠다는 생각을 하게 된 계기가 뭘까요? 그것부터 명확히 하고 가야할 것 같아요.
그리고 '상태를 가질 필요가 있는지'라는 질문의 의도 또한 사실 잘 와닿지 않습니다. 서비스 레이어라면 상태가 있어야지! 같은 답변을 기대하신 것이 맞을까요? 지금은 제가 질문을 이해하지 못한 상태라, 제가 이해할 수 있게끔 좀 더 다듬어서 질문 주시면 성심성의껏 답변드리도록 하겠습니다.
일단, 리뷰 드린 부분은 훌륭하게 반영해주셔서 요 PR은 머지하도록 하겠습니다.
고생 많으셨고, 다음 단계에서 얘기 마저 나눠보도록 해요!
안녕하세요 피터! 에프이입니다 🙂
이번 점심 뭐 먹지 미션 리뷰 잘 부탁드려요!
*️⃣배포 주소
https://chysis.github.io/javascript-lunch/dist/
*️⃣구조
Components
RestaurantController
RestaurantService
OutputView
RestaurantInterfaces
index.js
*️⃣기능 구현 사항
*️⃣고민한 내용
*️⃣질문
querySelector를 사용하면 결과값이 null이 될 수 있어서 매번 조건식을 추가했습니다.
!(type assertion)을 사용해도 된다는 것을 알았습니다.controller와 view를 UI 환경에서 명확히 구분하는 것이 정말 어려운 것 같습니다. 지금 상태는 기능의 결합을 고려한 결과인데요, 피터는 어떻게 생각하시는지 궁금합니다.
감사합니다 :)