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

[2단계 - 음식점 목록] 가브리엘(윤주현) 미션 제출합니다. #58

Merged
merged 182 commits into from
Mar 12, 2023

Conversation

gabrielyoon7
Copy link
Member

안녕하세요?

말씀 나눴던 것 처럼 아직 완성이 되지 않았지만 개인 사정으로 일단 PR을 먼저 보내드립니다.
현재 진행상황은 다음과 같습니다.

  1. 기능 요구사항은 완성됐습니다. (사실 타입 스크립트쪽은 리팩토링이 더 많이 필요한 것 같습니다.)
  2. 디자인은 아직 마무리가 되지 않았습니다.
  3. 지난 step1에 추가로 달아놓으셨던 피드백을 방금 읽어서 다시 반영해야 할 것 같습니다 ,,, 🥲🥲

step1로부터 변경 사항

리드미 최신화

리드미에 작성된 문서를 최신화했습니다.
주요 파일들이 하는 역할을 위주로 설명하도록 개선했습니다.
트리구조로 작성해놨습니다.

Proxy 구조 변경

step1 피드백에서 Controller가 없는 형태로 하는 것을 조언하셔서
RestaurantList에서 직접 Proxy를 선언하지 않도록 모듈화 하였는데요,
피드백 반영 전후를 비교해봤을 때 모든 컴포넌트들이 RestaurantList(의 상태)를 바라봐야 하는 문제는 여전했습니다.

따라서 이를 개선하기 위해 restaurants라는 데이터 관리용 객체를 구현하여 RestaurantList이 해야할 일과 부담을 줄여주는 방향으로 전면 개편하였습니다.

// restaurants.ts
import { renderRestaurantList } from "../components/RestaurantList/handleRestaurantList";
import IRestaurant from "../type/IRestaurant";

const initState = {
  restaurants: [] as IRestaurant[],
  filter: "all",
  sort: "name",
  menuTab: "tab-all",
};

const restaurants = {
  state: initState,
  create() {
    this.state = new Proxy(initState, {
      set: (obj, prop, value) => {
        // type-guard (최적화 필요)
        if (
          prop === "restaurants" ||
          prop === "filter" ||
          prop === "sort" ||
          prop === "menuTab"
        ) {
          obj[prop] = value;
        }
        renderRestaurantList();
        return true;
      },
    });
  },
};

Proxy 객체 접근 로직 개선

위에 언급된 코드를 누구나 접근하여 수정을 시도할 수 있었는데요, 구현하다보니 한가지 문제가 발생했습니다.
state라는 객체를 덮어 씌우는 행위는 setter함수가 동작을 하였으나, 특정 프로퍼티를 수정하는 행위는 검출하지 못했습니다.
예를 들면 즐겨찾기 등록을 위해 어떤 음식점의 프로퍼티 1개를 수정하면 반영이 되지 않는 문제가 있었습니다.
따라서 모든 업데이트 로직은 다음 코드를 거치게 수정하였습니다.

// restaurant.ts
export const updateRestaurants = (newRestaurants: IRestaurant[]) => {
  restaurants.state.restaurants = [...newRestaurants]; // Proxy 객체 업데이트
  Storage.saveRestaurants(restaurants.state.restaurants); // 로컬 스토리지에도 최신 상황을 반영
};

어떠한 경우에도 Proxy 객체를 수정하려는 경우 스프레드 연산자를 통해 setter 함수를 호출하였습니다.

UUID 적용

아이디의 고유 값을 UUID로 관리하고 있습니다.

개선이 필요한 사항

CSS 미구현 사항

각종 디자인이 아직 미완성 상태입니다.
이 부분은 이전에 말씀드렸던 것 처럼 step2 피드백 1차 이후에 반영할 수 있도록 하겠습니다.

리팩토링

타입스크립트에서 any가 사용된 곳은 없지만, 리팩토링이 필요한 부분이 몇 군데 있다고 생각합니다.
이 부분도 추후에 반영하겠습니다.

@woowahan-cron woowahan-cron self-assigned this Mar 10, 2023
@woowahan-cron woowahan-cron self-requested a review March 10, 2023 14:48
Copy link

@woowahan-cron woowahan-cron left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 가브리엘!!!

Step1에서 피드백 드린 내용은 웬만하면 중복을 제거하고 드리려고 하지만, 제가 일일이 모든 것을 체크하면서 봐 드리기는 어려워서 중복되더라도 양해 부탁드릴게요.

나라의 부름을 받고 예비군을 다녀오셔야 하는 입장이니 미션에서 정말 중요한 부분만 챙기실 수 있도록 도움을 드리려고 해요. 그래서 이번에는 살짝 순한맛 리뷰로 변경하여 꼭 필요한 부분만 피드백을 드리려고 합니다. 미비한 부분도 완성해서 다음 피드백 요청에서는 최대한 완성해 볼까요?

예비군 일정으로 타격이 상당히 커서 미션을 다 수행하지 못할 수도 있어요. 하지만, 코드를 보니 가브리엘은 아키텍처부터 패턴까지 상당히 고심한 부분이 곳곳에서 보이고 있어서 몇 가지만 보완한다면 이번 미션은 그냥 넘어가도 괜찮지 않을까 합니다.

그렇지만, 가브리엘이 어떠한 기준에서 코드를 작성했는지 궁금한 부분은 깜짝 퀴즈나 깜짝 묻고답하기를 곳곳에 심어 놓았으니 가브리엘만의 코드 작성 기준을 스스로 정리해 보면 좋겠어요.

아래에는 종합적으로 진행해 보았으면 하는 부분을 남겨 봅니다.

1. 웹 컴포넌트에서 단위별로 스타일 나누기

지금은 src/css/style.css 파일에 모두 몰빵되어 있는데 이 부분을 스타일로 나눠보는 것은 어떨까요?

2. 상수화 부족

상수의 경우 재활용이 목적이 아닌 "의미 부여"가 목적이다

아직 상수화하지 못한 부분이 많이 있습니다.
유지보수를 쉽게 할 수 있도록 상수화를 해보면 어떨까요?
스타일에서 쓰이는 클래스라거나, 필터의 정렬 기준이라거나 의미를 부여할 곳은 많아보이는걸요!

3/12 일요일 20시까지만 진행하시고 바로 피드백 요청하세요!

그때까지 모든 것이 완결되지 않아도 되고 피드백 모든 부분을 수정하지는 않아도 좋습니다.
가브리엘이 필요하고 중요하다고 생각하는 부분 위주로 피드백을 반영해 보면 좋겠습니다.
(예비군 때문이니까)

image

Comment on lines 7 to 8
const newMenu = event.target.value;
restaurants.state.menuTab = newMenu;

Choose a reason for hiding this comment

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

newMenu 값은 8번째 줄 이후로 참조되지 않는다면 굳이 선언하여 사용할 필요가 있을까요?

Suggested change
const newMenu = event.target.value;
restaurants.state.menuTab = newMenu;
restaurants.state.menuTab = event.target.value;

Copy link
Member Author

Choose a reason for hiding this comment

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

새로 선택된 탭이라는 것을 명시하기 위해 newMenu를 선언했었는데, menuTab에 들어가는 것이 당연히 새로운 탭이라고 생각해보면 불필요한 조치였던 것 같습니다.

}
restaurantList?.addEventListener("click", (event) => {
event.stopPropagation();
const id = (event.target as HTMLElement).closest("restaurant-item")?.id;

Choose a reason for hiding this comment

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

✨ 깜짝 퀴즈

    const id = (event.target as HTMLElement).closest("restaurant-item")?.id;

위의 코드에서 ?의 의미가 무엇인지 설명해 주세요.

Copy link
Member Author

Choose a reason for hiding this comment

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

🙋‍♂️ 깜짝 대답

?.는 옵셔널 연산자로, 좌측이 undefined거나 null인 경우, 참조를 방지하는 기능을 합니다.
예를 들면 restaurant-item을 찾을 수 있을지 없을지 확신하기 어려울 때 사용할 수 있습니다.

Comment on lines 15 to 27
set: (obj, prop, value) => {
// type-guard
if (prop === "restaurants" || prop === "filter" || prop === "sort") {
// type-guard (최적화 필요)
if (
prop === "restaurants" ||
prop === "filter" ||
prop === "sort" ||
prop === "menuTab"
) {
obj[prop] = value;
}
renderRestaurantList();
return true;
},

Choose a reason for hiding this comment

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

리팩터링을 위해 힌트를 드립니다.

뭐, 사실 정답을 찾아가는 과정은 그리 어렵지 않을 것이라고 생각합니다.
다만, 아래에 제안해 드린 코드가 어째서 무엇 때문에 오류없이 동작하는지 설명하는 것이 어려울 것이라고 생각해요.
그럼 코드 힌트 나갑니다!

1. 다음의 인터페이스를 프로젝트에 추가하여 해당 파일인 src/domain/restaurants.ts에 불러오기

interface IState {
  restaurants: IRestaurant[];
  filter: string;
  sort: string;
  menuTab: string;
}

2. set 영역을 아래와 같이 수정

Suggested change
set: (obj, prop, value) => {
// type-guard
if (prop === "restaurants" || prop === "filter" || prop === "sort") {
// type-guard (최적화 필요)
if (
prop === "restaurants" ||
prop === "filter" ||
prop === "sort" ||
prop === "menuTab"
) {
obj[prop] = value;
}
renderRestaurantList();
return true;
},
set: (obj, prop: keyof IState, value) => {
// type-guard (최적화 필요)
if (prop in obj) {
obj[prop] = value;
renderRestaurantList();
return true;
}
return false;
},

3. 설명하기

✨ 깜짝 퀴즈

가. 코드는 적절하게 작성되었나요? 그렇다면 또는 그렇지 않다면 이유는?
나. 타입스크립트에서 keyof가 뭘 하는 녀석이죠?
다. if (prop in obj) 에서 in의 의미는 무엇인가요?
라. prop: keyof IState의 의미가 무엇이고 이렇게 기입한 경우 동작하는 이유가 무엇인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

🙋‍♂️ 깜짝 대답

가. 적절한 것 같습니다. 일단 리스트의 상태를 인터페이스로 작성해놔서 이전 처럼 키 값(prop)을 일일이 작성하지 않아도 되는 장점이 있습니다.
나. keyof는 어떤 객체의 모든 속성을 유니온 타입으로 돌려줍니다. 즉, prop이 가질 수 있는 타입을 제한합니다.
다. obj는 곧 초기 상태인 state 객체 (IListState)입니다. prop은 IListState의 키 중 하나입니다. 따라서 해당 키 값이 해당 객체의 키 중 해당하는 것(in)이 있는지 검사하는 것입니다.
라. 에서 답변드린것 처럼 prop의 형을 IListState의 키 값을 유니온으로 관리하는 것 입니다. 따라서 prop이 반드시 키 값 중 하나여야 합니다.

@@ -21,11 +28,13 @@ export const distanceOptions = () => {
export const createNewRestaurant = (event: SubmitEvent) => {
const formData = new FormData(event.target as HTMLFormElement);
const newRestaurant: IRestaurant = {
id: uuidv4(),

Choose a reason for hiding this comment

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

🎤 깜짝 묻고답하기

  1. uuid는 무엇인가요?
  2. uuidv4는 어떤 원리로 랜덤한 값을 생성하나요?

알아야 한다거나 심도 있게 공부할 필요까지는 없지만, 그래도 사용해 보았으니 uuid의 존재를 모르는 크루에게 설명한다고 가정하고 정리해 볼까요? 정리하고 PR에 댓글을 남기자마자 까먹어도 좋습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

🙋‍♂️ 깜짝 대답

  1. uuid는 식별자로 사용하기 위한 데이터를 생성해주는 표준 기술입니다.
  2. uuidv4는 이 중에서도 랜덤한 번호를 생성해주는 기술로 122개의 난수 비트 와 시간, mac주소 등등 여러가지의 알고리즘을 조합해서 난수를 생성합니다.

요약하면 거의 모든 시스템에서 중복될 가능성이 매우 적은 난수를 생성할 수 있다고 가정하는 식별자 생성 기술입니다.

Comment on lines 32 to 36
category: formData.get("category") as TCategory,
name: formData.get("name") as string,
distance: Number(formData.get("distance")),
description: formData.get("description") as string,
link: formData.get("link") as string,

Choose a reason for hiding this comment

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

🎤 깜짝 묻고답하기

  1. 이곳에 타입 단언은 왜 사용하셨나요?
  2. 타입 단언이 모든 상황에서 유용할까요? 이유는요?

또.. 여기다가 타입 단언 왜 사용했냐고 질문을 던졌으니
"어라~ 크론이 여기에 리뷰를 했네.. 잘못한 거야.. 고쳐야겠어" 하면서
"수정했습니다~" 코멘트 달지 마시고..ㅠㅠ
올바르게 판단하셔서 본인이 나름대로의 기준을 세워서 작성한 것이라면 그대로 두시기 바랄게요

학습하는 관점에서 깜짝 질문이나 묻고답하기를 드리는 경우
가브리엘이 본인의 기준대로 작성하신 것인지 혹은 합당한 근거나 이유가 있어 작성한 것인지 등을 파악해보기 위한 용도입니다.
제가 파악한다기보다는 가브리엘이 코드를 작성하면서 본인만의 기준이나 일관성을 정립해 나가는 과정이라고 할 수 있겠죠!

Copy link
Member Author

Choose a reason for hiding this comment

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

🙋‍♂️ 깜짝 대답

제가 생각했을 때 이 곳에서 타입 단언(as)을 사용한 이유는 FormData애서 get으로 받아온 값이 string인지 아닌지 확신할 수 없다고 생각해서였습니다.

실제로 타입스크립트가 해당 값이 명백하게 string이라고 판단하지 못하는 상황이므로, 해당 값의 타입이 string인지 타입 가드와 같은 내로잉을 통해서 타입을 지정할 수 있다고 생각합니다.
하지만 타입 가드 검사에서 통과되지 못하는 경우에는 에러를 호출하거나 반드시 string인 어떤 특정한 값(예를 들면 ""과 같은 공백)으로 반환하여 레스토랑 객체를 만들거나 오류를 회피할 수 있습니다.

다만, 지금처럼 프로퍼티가 너무 많은 경우에는 일일이 타입 가드를 적용해주는 것이 오히려 가독성을 해치기 때문에 타입 단언으로 as를 사용하는 것이 낫다고 생각했습니다.

@@ -1,9 +1,11 @@
import { TCategory } from "./TCategory";

export default interface IRestaurant {
id: string;
category: TCategory | string;

Choose a reason for hiding this comment

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

TCategory | string은 곧 string이지 않나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다. step1 때 문제를 겪어서 임시로 저렇게 처리했는데, 지금은 제거해도 오류가 안나는군요...!

}

attributeChangedCallback(name: string, oldValue: string, newValue: string) {
this.favorite = JSON.parse(newValue);

Choose a reason for hiding this comment

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

newValue는 오류가 나지 않을 것이란 보장을 받을 수 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

보장 받기 어렵습니다. 반영하겠습니다.

this.addEventListener("click", (event) => {
event.stopPropagation();
updateFavorite(id);
const buttons = document.querySelectorAll(

Choose a reason for hiding this comment

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

✨ 깜짝 퀴즈

querySelectorquerySelectorAll의 차이점이 무엇인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

🙋‍♂️ 깜짝 대답

querySelector은 조건에 따라 탐색한 엘리먼트 중 가장 먼저 등장한 엘리먼트 1개를 반환합니다.
반면에 querySelectorAll은 조건에 따라 탐색한 엘리먼트 모두를 배열로 반환합니다.

`.favorite-button-${this.restaurantId}`
);
buttons.forEach((button) => {
button.setAttribute("favorite", `${!this.favorite}`);

Choose a reason for hiding this comment

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

웹 컴포넌트로 작성되는 부분이기는 하지만 favorite는 버튼이 가지는 기본 속성은 아닐 텐데요!
데이터 속성을 사용해 보는 것은 어떤가요? (사용하지 않아도 무방합니다. 취향 차이..)

Copy link
Member Author

Choose a reason for hiding this comment

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

보내주신 자료를 토대로 버튼의 favorite 속성을 데이터 속성으로 표기해봤는데, 엘리먼트의 dataset을 통해 접근을 할 수 있다는 것이 유용한 것 같습니다. 예를 들면 제가 임의로 어트리뷰트 명을 짓게되면, 해당 네이밍이 표준인지 비표준인지 헷갈릴 수 있게 되는 문제가 있는데, 데이터 속성은 이런 문제들을 일부 방지해주는 장점이 있는 것 같습니다.

다만 이번 미션에서는 기존에 적용해놨던 어트리뷰트들이 어디서 쓰이는지 파악하기 어려워진 것 같아서 즐겨찾기 기능에만 적용해보겠습니다.

Comment on lines +35 to +36
class="favorite-button-${restaurant.id}"
restaurant-id="${restaurant.id}"

Choose a reason for hiding this comment

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

여기도 커스텀 속성을 사용해 보는 건 어떨까요? (취향에 맞고 공감한다면)

Copy link
Member Author

Choose a reason for hiding this comment

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

적용해봤습니다!

Copy link

@woowahan-cron woowahan-cron left a comment

Choose a reason for hiding this comment

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

코드 리뷰를 마치며...

cypress를 위한 테스트 코드를 제외하면 나머지는 경험할 만큼 충분히 경험했다고 생각해요!
프로젝트란 살아 있는 생물과 같아서 계속해서 피드백이라는 자양분으로 성장해야 하는 것과 같다고 생각해요.
하지만, 우리는 학습을 위해 코드를 작성하는 것이잖아요?
이번 미션에서도 마찬가지로 피드백을 드리자면 끝도 없을 것이라서 이쯤에서 종료하려고 합니다!
머지가 통과되더라도 훈련이 끝난 이후에 테스트 코드를 작성해보시면 좋겠습니다~

고생하셨습니다.

image

@@ -0,0 +1,14 @@

describe('test', () => {
it('레스토랑 추가', () => {

Choose a reason for hiding this comment

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

오호.. 그런데 지금은 테스트 코드가 1개밖에 되어 있지 않아요
좀 더 다양한 테스트 케이스를 생각해 보는 건 어떨까요?

Choose a reason for hiding this comment

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

추가로 테스트를 위한 코드도 코드이므로 어떻게 리팩터링해서 가독성을 끌어올리면 좋을지도 같이 고민해주세요!

Copy link
Member Author

Choose a reason for hiding this comment

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

네. 알겠습니다.

Comment on lines +21 to +22
<category-select-box></category-select-box>
<sorting-select-box></sorting-select-box>

Choose a reason for hiding this comment

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

여닫는 태그 사이에 아무것도 존재하지 않으면 <tagName />과 같이 간결하게 사용할 수도 있답니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

step1에서 <tagName />을 사용하는 경우 오류가 발생하는 경우가 있어서 <tagName></tagName >으로 표기했습니다. insertAdjacentHtml으로 모달 내부를 그려주는 과정에서 잘못 된 위치에서 태그가 닫히는 등 여러 문제가 있었습니다 ㅠㅠ 지금은 insertAdjacentHtml을 제거하여 해당 문제가 없긴 하지만 당시에 그 원인이 <tagName />표기법 때문인지를 확신하기 어려워서 <tagName></tagName >로 교체하고 innerHtml을 쓰는 것으로 문제를 해결하였습니다.

gabrielyoon7@394586e

gabrielyoon7@d73eb64

@woowahan-cron woowahan-cron merged commit f8a0894 into woowacourse:gabrielyoon7 Mar 12, 2023
@gabrielyoon7
Copy link
Member Author

이번 미션에서 정말 많은 피드백(과 깜짝 질문)을 받은 만큼, 많이 학습하게 된 계기가 된 것 같습니다. 특히 타입스크립트 초보자인 입장에서 반드시 알아야 할 기술과 관련된 질문을 많이 남겨주신 것이 굉장히 많은 도움이 됐습니다.

감사합니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants