Skip to content

[2단계 - 자주 가는 음식점] 헤일리(유소정) 미션 제출합니다.#233

Merged
yujo11 merged 55 commits intowoowacourse:kaori-killerfrom
kaori-killer:step2
Mar 24, 2025
Merged

[2단계 - 자주 가는 음식점] 헤일리(유소정) 미션 제출합니다.#233
yujo11 merged 55 commits intowoowacourse:kaori-killerfrom
kaori-killer:step2

Conversation

@kaori-killer
Copy link
Copy Markdown

@kaori-killer kaori-killer commented Mar 15, 2025

안녕하세요, 유조~!
주말 잘 보내고 계신가요? 😄

이번 미션에서는 STEP 1에서 만든 컴포넌트를 재활용하면서,
컴포넌트를 효율적으로 설계하는 것이 얼마나 중요한지 새삼 느꼈어요!
다만, 상태 관리와 E2E 테스트는 여전히 쉽지 않아서 고민이 많았네요. 😭

이번 미션도 잘 부탁드려요! 🙌✨

학습 목표

이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.

  • UI와 도메인 영역을 분리할 수 있는 설계를 고민해보고, 목적에 맞게 객체와 함수를 활용
  • TDD 방식으로 개발하며, 단위 테스트 기반으로 점진적인 리팩터링

제출 전 체크 리스트

  • 기능 요구 사항을 모두 구현했고, 정상적으로 동작하는지 확인했나요?
  • 기본적인 프로그래밍 요구 사항을 준수하고 있는지 확인했나요?
  • 테스트 코드는 모두 정상적으로 실행되나요?
  • (해당하는 경우) 배포한 데모 페이지에 정상적으로 접근할 수 있나요?

리뷰 요청 & 논의하고 싶은 내용

1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점

📌 컴포넌트 리렌더링 최적화

  • 기존 방식의 문제점
    • 처음에는 특정 조건에서 리렌더링이 필요할 때, 아래와 같이 body 전체를 초기화하는 방식을 사용했어요.
export default function reset() {
  document.querySelector("body").innerHTML = "";
}

하지만 사실 전체 화면을 다시 그릴 필요 없이 음식점 목록만 갱신하면 되는 상황이었어요.
그런데 Restaurant.js를 보면 **헤더(Header)와 음식점 리스트(RestaurantListContainer)**가 하나로 묶여 있어서 리스트만 따로 갱신하기가 쉽지 않았어요.

  • 개선한 방법
    이 문제를 해결하기 위해, 특정 조건을 받아서 음식점 목록 부분만 업데이트하는 방식으로 개선했어요.
export default function Restaurant({ isReRender }) {
  ...
  if (isReRender) {
    reRenderRestaurantListContainer($body, $restaurantListContainer);
  }
  ...
}

📌 타입스크립트 & E2E 테스트

아직 타입스크립트와 E2E 테스트가 익숙하지 않아서 부족한 점이 많을 것 같아요..! 😭
특히, E2E 테스트를 UI 단위 테스트가 아니라 사용자 시나리오 중심으로 작성하는 방법이 감이 잘 오지 않네요.

타입스크립트도 도메인 영역에 최대한 적용하려고 노력했는데,
이 방식이 적절한지 확신이 없어요. 🤔

혹시 코드 보시고 작은 피드백이라도 주시면 바로 반영하겠습니다! 💪

2) 이번 리뷰를 통해 논의하고 싶은 부분

🎯 UI와 Domain을 효과적으로 분리하는 방법

현재 상태를 컴포넌트와 완전히 분리하여 별도의 파일에서 전역으로 관리하고 있어요. 하지만 이렇게 하면 관심사가 ‘분리’된 것이 아니라 오히려 ‘파편화’된 느낌이 들더라고요. 상태가 필요할 때마다 여기저기에서 가져다 쓰면서 관리가 어려워지고 있어요. 🤯

그러다 보니 "그럼 상태를 컴포넌트 내부에서만 관리하도록 클래스로 만들면 어떨까?" 라는 고민도 했어요. 하지만 그렇게 하면 UI와 Domain이 분리되지 않는 문제가 발생하더라고요. 😢

현재 가장 큰 고민은:

  • UI와 상태 관리(Domain)를 어떻게 적절히 분리할지
  • 클래스 컴포넌트를 언제 사용하면 좋을지
  • Domain 코드가 너무 복잡해지고 있는 문제를 어떻게 해결할지

이 부분에 대해 조언을 주시면 정말 감사하겠습니다! 🙌

🎯 공통 코드 정리 - 유틸리티 함수 분리

자주 사용하는 함수들을 유틸리티 함수로 정리해 더 효율적으로 관리할 수 있도록 했어요!
특히, 재사용 가능한 컴포넌트의 유틸리티 함수와 일반적인 유틸리티 함수를 명확히 구분하기 위해 📂 src/components/util 과 📂 src/utils로 나누어 관리했습니다.

하지만, 이 구분이 완벽하게 적용된 건지 고민이 되네요. 🤔
혹시 컴포넌트 관련 함수가 일반 유틸 폴더에 들어가 있거나, 반대로 일반 유틸 함수가 컴포넌트 폴더에 포함된 건 아닐까? 하는 의문이 들어요.

이렇게 나누는 게 적절할지, 혹은 더 좋은 방식이 있을지 고민 중입니다. 유조의 의견이 궁금해요!


✅ 리뷰어 체크 포인트

1단계

  • UI를 컴포넌트 단위로 분리했는가?
  • 재사용 가능한 컴포넌트를 고려했는가?
  • 주요 기능 시나리오에 대한 E2E 테스트를 작성했는가?

2단계

  • 오류 상황(예: 잘못된 입력)에 대해서도 테스트 케이스를 정의했는가?
  • 타입 정의를 적절히 사용하여 컴파일 오류 없이 안정적으로 동작하는가?
  • 로컬/배포 환경에서 테스트가 정상적으로 동작하는지 확인했는가?

@kaori-killer kaori-killer marked this pull request as ready for review March 15, 2025 12:23
@kaori-killer kaori-killer changed the base branch from main to kaori-killer March 15, 2025 12:24
@yujo11 yujo11 self-requested a review March 15, 2025 13:26
@yujo11 yujo11 self-assigned this Mar 15, 2025
Copy link
Copy Markdown

@yujo11 yujo11 left a comment

Choose a reason for hiding this comment

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

안녕하세요 헤일리 ~ 다시 만나서 반갑습니다 ㅎㅎ

📌 컴포넌트 리렌더링 최적화

이 부분은 지난 step 리뷰하면서 개선되었으면 좋겠다고 생각한 부분인데 스스로 해결방법을 찾아서 해결해주셨군요. 멋집니다 헤일리 👍 👍 👍

🎯 UI와 Domain을 효과적으로 분리하는 방법

UI와 상태 관리(Domain)를 어떻게 적절히 분리할지

도메인 로직과 UI 컴포넌트가 분리되어 있지만 헤일리가 말씀해주신 것과 같은 문제들(관심사 파편화, 전역 상태 관리)과 같은 문제로 전체 코드를 파악하기가 조금 어렵게 느껴졌어요. 이런 문제들은 요구사항이 추가되고 도메인이 복잡해질수록 점점 더 커져 유지보수하기 어려운 형태로 변하게 될 가능성들이 있다고 느껴졌습니다. 여기에 추가로 다음 질문으로 주신 유틸리티 함수 부분에서도 유틸 함수이지만 도메인에 의존하거나 UI와 관련되어 있는 유틸 함수들이 많아 보였어요.

UI와 도메인 로직을 분리하고 싶어 하시는 것 같은데요. 그렇게 분리하는 경우에는 이벤트 버스 패턴 같은 것을 통해서 분리를 하는 방법도 가능할 것 같습니다.그러나 현재 단계에서 이 정도의 복잡도가 필요하다고는 생각되지 않는데요. 대신에 컴포넌트 패턴을 통해서 각각의 컴포넌트에서 필요한 상태들을 각각의 컴포넌트가 관리하도록 하는 것도 또 하나의 방법이 될 수 있을 것 같아요. 두 방법 외에도 다른 방법들도 있을텐데요. 어떤 방법을 사용하시던 간에 전체적인 아키텍처를 바꿔야 하는 작업이 되는 만큼 시간이 오래 걸리고 구현 과정에서 여러 어려움을 겪을 수 있는데요. 한 번에 진행이 어려우시다면 언제든지 리팩터링 과정에서 중간중간 PR을 생성해주시거나 아니면 따로 여쭤봐 주셔도 좋을 것 같아요.

클래스 컴포넌트를 언제 사용하면 좋을지

이건 어떤 정답이 있는 부분은 아니라고 생각을 하는데요. 지금 헤일리가 구현해주신 것과 같은 도메인과 관련된 모델을 정의하거나 제가 위에서 말씀드린 컴포넌트 패턴으로 UI 컴포넌트를 구현하는 경우에 기본적인 Component의 형태를 class로 정의하고 extends 하면서 사용하면 캡슐화가 가능하고, class를 통해 특정 메서드의 구현을 강제하는 등의 역할을 할 수 있어 class가 유리할거 같아요.

그럼 언제 함수를 사용하는 게 더 나을지도 생각을 해보면 좋을 것 같은데요. 함수의 경우에는 어떤 상태를 알고 있을 필요가 없는 경우, 간단한 이벤트를 핸들링 하는 경우. 아니면 단순히 한번 호출되어서 어떤 UI를 그리는 경우에는 클래스로 굳이 구현을 할 필요가 없겠죠.

Domain 코드가 너무 복잡해지고 있는 문제를 어떻게 해결할지

이 부분은 위에 UI와 상태관리를 어떻게 적절히 분리할지에 관한 질문에 답변을 드린 내용도 참고하시면 좋을 것 같은데요. 추가로 현재 도메인이라고 불리고 있는 클래스들에서 데이터 구조를 정의하고 유효성 검사를 진행하고 그와 관련된 서비스 로직들도 처리하고 로컬스토리지(저장소)에 접근을 하는 부분들도 모두 들어있는데요. 이런 부분들을 각각의 역할로 보고 하나의 클래스에서 관리되는 게 아니라 레이어를 나눠서 관리해준다면 복잡도가 내려갈 수 있을거 같아요

🎯 공통 코드 정리 - 유틸리티 함수 분리

이 부분은 코드 리뷰 과정에서 코멘트를 남겼는데요. 해당 코멘트들을 참고해주시면 좋을거 같아요! 이 부분은 위 질문에 남겼던 답변들대로 리팩터링이 진행된다면 자연스럽게 어느 정도 해소 되는 부분도 존재할거 같네요!


리뷰를 남기다보니 코멘트가 많아지고 답변이 조금 길어졌는데요. 확인해 보시고 잘 이해가 되지 않는 부분이나 추가적으로 궁금한 부분들이 있다면 언제든지 편하게 말씀해 주세요. step2 진행하느라 고생 많으셨습니다 헤일리 ~~ 👍 👍 👍

import ERROR_MESSAGE from "../../src/constants/errorMessage";

describe("음식점 추가 E2E 테스트", () => {
describe("유저가 음식점을 추가하고, 추가한 음식점을 목록에서 확인할 수 있어야 한다.", () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 👍 👍

Comment on lines +1 to +13
describe("음식점 관리 기능 테스트", () => {
beforeEach(() => {
cy.visit("http://localhost:5173");
cy.viewport(1280, 720);
});

describe("음식점 목록이 정상적으로 렌더링 되는 경우", () => {
it("음식점 목록을 확인할 수 있다.", () => {
cy.get(".restaurant").should("have.length.at.least", 1);
});
});

describe("음식점 상세 정보 모달을 여는 경우", () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

context를 잘 나눠 주셨네요 👍 👍 👍 💯 💯

$fragment.appendChild(RestaurantItemDetailModalButtonContainer({restaurantId}));

return $fragment;
} No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

코드 포맷터가 잘 동작하고 있는지 한번 점검해보셔도 좋을거 같네요

Copy link
Copy Markdown
Author

@kaori-killer kaori-killer Mar 17, 2025

Choose a reason for hiding this comment

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

VSC에 Prettier 설정이 안되어 있었네요 😅
수정했습니다!

c54b7d2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TypeScript를 사용한 파일과 그렇지 않은 파일들이 있는데 사용 기준이 있었나요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

요구사항 중에 도메인 영역을 TypeScript으로 변환한다. (UI 영역은 선택)라고 되어 있었는데요!
TypeScript가 익숙하지 않아서 도메인 영역만 바꿨습니다. (상태, 유효성 검사)

this.validateDistance(distance);
this.validateName(name);

this.id = id || crypto.randomUUID();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

여긴 default parameter를 사용하지 않은 이유가 있을까요?

Copy link
Copy Markdown
Author

@kaori-killer kaori-killer Mar 17, 2025

Choose a reason for hiding this comment

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

기존에 등록된 음식점이라면 기존 id를 그대로 유지해야 하고,
새로운 음식점이라면 id가 없으므로 새롭게 생성해야 합니다!

그래서 이를 처리하기 위해 default parameter를 사용하지 않고,
다음과 같이 || 연산자를 활용했습니다 😄

더 개선할 방법이 있을까요?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

지금도 충분해보입니다 ㅎㅎ 👍

export default function createTabButton({className, text, isWishTab}) {
const $tab = createElement({
tag: "button",
classNames: ["restaurant-tab", className],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

여기도 위와 같은 맥락으로 도메인에 대한 내용을 이 함수가 알고 있어 util로 불리는게 어색하게 느껴지네요

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

새로운 element를 만드는 코드로 보이는데 이 친구는 Component로 만들지 않은 이유가 있을까요?

Copy link
Copy Markdown
Author

@kaori-killer kaori-killer Mar 23, 2025

Choose a reason for hiding this comment

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

컴포넌트인데 네이밍을 잘못 지으면서 유틸로 착각했네요 😅
이 부분은 TabButton 컴포넌트로 수정하고 componets/util로 넣었어요.

추가적인 질문이 있습니다!
이 컴포넌트가 도메인에 대해 알고 있다는 것은 해당 컴포넌트가 도메인(상태)를 인자로 받기 때문인가요? 그럼 반복해서 사용하는 컴포넌트지만 유틸로 분리하기 어려울까요?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

네네 맞습니다 ㅎㅎ

그럼 반복해서 사용하는 컴포넌트지만 유틸로 분리하기 어려울까요?

반복해서 사용하는 로직 자체는 추상화가 가능하지만 util 함수로 불리는 것이 조금 어색하게 느껴졌습니다

const category = selectedFilterValue.getSelectedFilterCategoryValue();
const sorting = selectedFilterValue.getSelectedFilterSortingValue();
return dataList.filter((data) =>
category === "전체" || data.category === category
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

상수로 만들어서 관리해보셔도 좋을거 같네요

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

수정했습니다!

0f7c843

Comment on lines +26 to +30
dataList.sort((a, b) => {
if (a.distance > b.distance) return 1;
if (a.distance < b.distance) return -1;
return 0;
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

아래처럼 구현해도 같은 결과가 나올거 같네용

Suggested change
dataList.sort((a, b) => {
if (a.distance > b.distance) return 1;
if (a.distance < b.distance) return -1;
return 0;
});
dataList.sort((a, b) => a.distance - b.distance);

}

function filterByStar(dataList) {
return dataList.filter((data) => data.isWish === true) || [];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

|| [] 은 어떤 상황을 대비하기 위해 들어간 코드일까요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

앗! 아무 값도 일치하지 않으면 []를 반환하기 때문에 해당 구문이 필요가 없네요!
수정했습니다 :)

});

describe("음식점 즐겨찾기(★) 기능을 테스트하는 경우", () => {
it("음식점 목록에서 즐겨찾기 버튼 클릭 시 추가 및 삭제 가능", () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

추가/삭제가 별도의 테스트로 작성되어 있어도 좋을거 같네요

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

반영했습니다!

5c4a72b

@kaori-killer
Copy link
Copy Markdown
Author

안녕하세요 유조~ 요청이 늦었는데 기다려주셔서 감사합니다!

아래 두 작업은 시간이 오래 걸릴 듯 싶어서 이것들을 제외하고 요청드립니다 !!

  • 함수형 컴포넌트를 클래스로 바꿔보는 작업
  • 전역 상태를 없애는 작업

Copy link
Copy Markdown

@yujo11 yujo11 left a comment

Choose a reason for hiding this comment

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

코멘트를 꼼꼼하게 확인하고 반영해주셨군요 고생 많으셨습니다 헤일리 !!

헤일리가 남겨주신 질문들에 대한 답변들도 남겼으니 같이 확인해보시면 좋을거 같아요!

전역 상태를 제거하는 작업이나 Class로 전환하는 작업은 다음 미션에서 신경 써서 진행해보시면 좋을거 같습니다!

이 코드베이스에서 위 작업들을 해보고 싶고, 추가로 리뷰 받고 싶으시다면 언제든지 편하게 말씀해주세요~

고생 많으셨습니다 헤일리 ~~ 👍 👍 👍

},
];

export default restaurants; No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

여기는 여전히 EOL이 발생하네요 .ts 파일들에서도 코드 포맷터가 잘 동작하는지 확인해봐도 좋겠네요!

import RestaurantData from "./RestaurantData.ts";
import restaurants from "../constants/restaurants.js";

const LOCAL_STORAGE_KEY = "dataList";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 👍 👍

}

createData(data: Restaurant): RestaurantData {
#createData(data: RestaurantDTO): RestaurantData {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 👍 👍

updateSelectedFilterValue(id: string, value: string | boolean): void {
if (id === "category-filter") {
this.#category = value as string;
if (id === "category-filter" && typeof value === "string") {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 👍 👍

@yujo11 yujo11 merged commit 95a83db into woowacourse:kaori-killer Mar 24, 2025
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.

2 participants