Skip to content

[1단계 - 음식점 목록] - 밍고(성민제) 미션 제출합니다.#207

Merged
JeongBin0227 merged 42 commits intowoowacourse:minsungjefrom
MinSungJe:step1
Mar 10, 2025
Merged

[1단계 - 음식점 목록] - 밍고(성민제) 미션 제출합니다.#207
JeongBin0227 merged 42 commits intowoowacourse:minsungjefrom
MinSungJe:step1

Conversation

@MinSungJe
Copy link
Copy Markdown

@MinSungJe MinSungJe commented Mar 7, 2025

케빈 안녕하세요 밍고입니다! 점심 뭐 먹지 미션동안 잘 부탁드립니다. 😎

학습 목표

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

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

제출 전 체크 리스트

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

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

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

이번 구현동안 어플리케이션을 컴포넌트로 보는 연습에 집중하여 미션에 임했습니다!

페어와 같이 최종적으로 구현된 컴포넌트 단위는 다음 그림과 같습니다.

image

저희가 생각하며 분리한 컴포넌트의 기준은 재사용에 초점을 맞췄습니다.
즉 유사한 형태로 여러 번 사용되는 형태(LunchInfoCard, InputForm 등)를 컴포넌트로 분리해 구현했습니다.
또한 만든 컴포넌트 형태로 묶어 다른 파일로 빼는 경우(그림에서 파란색)도 분리했습니다.

저희의 진행 방식은

  1. UI 적용
  2. E2E 테스트 작성
  3. 컴포넌트 구현 및 동적 렌더링 구현
  4. 리팩터링

으로, E2E를 먼저 작성해 리팩터링 과정에 기존의 기능이 망가지지 않았는지 확인하며 구현해나갔습니다.

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

  • E2E 테스트의 범위를 어디까지 진행해야 하는가?

저희가 Cypress를 처음 사용하니 확실하게 써보자는 마음으로 **요소가 화면에 보이는가?**와 같은 모든 경우를 E2E 테스트에 담았습니다. 하지만 앞으로 있을 미션에서도 너무 꼼꼼하게 적다 보면 괜히 시간을 버리는 것이 아닌가하는 우려가 있습니다. 케빈의 경우에는 어디까지 Cypress를 이용한 E2E 테스트를 진행할 지 궁금증이 생겨 여쭤보고 싶습니다.

  • 데이터의 상태는 어떻게 관리하는가?

b739431 저희는 음식점 정보를 관리하기 위해 state.js를 분리했습니다. 구현이 간단하고 여러 컴포넌트에서 state 속 음식점 정보를 참조하기 편하도록 하기 위함입니다. 하지만 상태가 변경됐을 때 화면에 바로 반영이 되지 않고, 따로 리렌더링을 해줘야 하는 문제가 있습니다. 이처럼 여러 컴포넌트에서 사용하는 상태를 관리하기 위해 전역으로 빼는 방법 이외에 다른 방법들도 많을 것 같은데, 케빈은 어떤 식으로 구현할 것 같은 지 여쭤보고 싶습니다.


✅ 리뷰어 체크 포인트

1단계

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

2단계

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

shuyeon added 30 commits March 4, 2025 13:37
@JeongBin0227 JeongBin0227 self-assigned this Mar 8, 2025
@JeongBin0227
Copy link
Copy Markdown

E2E 테스트의 범위를 어디까지 진행해야 하는가?
저희가 Cypress를 처음 사용하니 확실하게 써보자는 마음으로 **요소가 화면에 보이는가?**와 같은 모든 경우를 E2E 테스트에 담았습니다. 하지만 앞으로 있을 미션에서도 너무 꼼꼼하게 적다 보면 괜히 시간을 버리는 것이 아닌가하는 우려가 있습니다. 케빈의 경우에는 어디까지 Cypress를 이용한 E2E 테스트를 진행할 지 궁금증이 생겨 여쭤보고 싶습니다.

-> 사실 요기에 정답이 없기 떄문에.. 제 경험을 말씀드리면 전 e2e 테스트는 이 기능이 사용자의 관점에서 정상적으로 동작하는가? 를 중심으로 핵심 기능들을 테스트 해주시면 된다고 생각해요. 근데 그 과정에서 핵심 사용 플로우에 초점을 맞추되, 불필요한 UI 렌더링 검증은 지양하는 것이 좋다고 생각합니다. 여기까지 들어가면 너무 깊어 질 수 있고, 검증 목적에 벗어날수 있다고 생각해요

그냥 밍고가 만든 서비스를 개발 다하고 테스트 하시잖아요? 아마 보통 해피 케이스 위주로...? 그걸 e2e 테스트로 풀어내면 좋을거 같아요
참고로 현업에서는 QE (퀄리티 엔지니어링) 이라는 직군이 따로 있어서 그분들이 만들어주신 TC 로 e2e 테스트 작성합니다

@JeongBin0227
Copy link
Copy Markdown

JeongBin0227 commented Mar 8, 2025

데이터의 상태는 어떻게 관리하는가?
b739431 저희는 음식점 정보를 관리하기 위해 state.js를 분리했습니다. 구현이 간단하고 여러 컴포넌트에서 state 속 음식점 정보를 참조하기 편하도록 하기 위함입니다. 하지만 상태가 변경됐을 때 화면에 바로 반영이 되지 않고, 따로 리렌더링을 해줘야 하는 문제가 있습니다. 이처럼 여러 컴포넌트에서 사용하는 상태를 관리하기 위해 전역으로 빼는 방법 이외에 다른 방법들도 많을 것 같은데, 케빈은 어떤 식으로 구현할 것 같은 지 여쭤보고 싶습니다.

-> state.js를 활용하는 방식은 전역 상태를 처럼 사용하면서 쉽게 참조가능하고, 여러 컴포넌트에서 동일한 데이터를 사용할 수 있는 장점이 있다고 생각합니다. 하지만 말씀하신대로 상태가 변경될 때 화면이 자동으로 리렌더링되지 않고, state.js를 직접 import하여 참조하면, 컴포넌트 간 결합도가 높아진다는 큰 단점이 있다고 생각합니다

만약에 제가 이런 미션을 했더라면 stata.js 를 사용하지 않고, DOM 자체를 데이터 저장소처럼 활용하는 방법도 고려해봤을거 같아요. data-attribute를 활용하는거죠! 이러면 데이터 조작이 좀 복잡해질수 있지만, 새로운 데이터 추가 시 UI를 다시 그릴 필요 없어서 좋을거 같아요

해당 미션에서는 권하지 않는 방법일거 같은데, 저라면 이런 상황에서 EventEmitter를 활용한 상태 관리를 할 거 같습니다.
state js 코드를 예시로 본다면,

class StateManager {
  constructor() {
    this.state = {
      restaurantList: [],
    };
    this.listeners = new Set();
  }

  subscribe(listener) {
    this.listeners.add(listener);
  }

  notify() {
    this.listeners.forEach((listener) => listener(this.state));
  }

  addRestaurant(restaurant) {
    this.state.restaurantList.push(restaurant);
    this.notify();
  }
}

export const stateManager = new StateManager();

이런식으로 EventEmitter를 활용하면 상태가 변경될 때 자동으로 UI가 업데이트되도록 할 수 있을거 같아요

@JeongBin0227
Copy link
Copy Markdown

다음번에는 배포한 링크를 기입해주시면 좋을거같아요~

@JeongBin0227
Copy link
Copy Markdown

학습목표에 관련해서..

  • 컴포넌트 단위로 모듈화하여 개발 하는건 잘하신거 같아요 👍 다만 이제 이 컴포넌트의 결합도를 기준으로 재사용성이나 상태변화, 렌더링등을 고민해보면 좋을거 같아요
  • 요소가 보이는가? 는 단위 테스트로 충분한거 같고, 사용자 플로우 중심으로 테스트를 재 고민해보면 좋을거 같아요!

Copy link
Copy Markdown

@JeongBin0227 JeongBin0227 left a comment

Choose a reason for hiding this comment

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

안녕하세요 밍고! 만나서 반갑습니다 잘 부탁드려요~

전반적으로 잘 해주셨다고 생각합니다. 👍 코드 가독성이 좋았고, 컴포넌트를 상세하게 잘 분리해주셔서 좋았어요. 코드에 대한 피드백과 학습목표, 질문에 대한건 코멘트로 남겨두었으니 확인해보시고 만약에 이해가 안되는 코멘트가 있다면 편하게 질문주세요~
아직 배포가 안된거 같아 화면단으로 확인은 못했는데, 해당부분은 배포해주시면 다시 확인해볼게요!

음식점 목록 미션하느라 고생하셨습니다 💯


test(`${DESCRIPTION_LENGTH_MAX}자를 넘는 가게 이름은 받을 수 없다.`, () => {
const wrongDescription =
"안녕하세요이제는300자를넘겨야하네요조금은엉성한코드일수있겠지만아무쪼록잘부탁드립니다.300자를넘기기위해선어떤이야기를해야할까요?오늘점심은제가우육면을먹었어요사실다적었었는데중간에라이브쉐어가꺼져서300자를적어놓고82자밖에남지않았어요너무억울해서조금만복붙하겠습니다안녕하세요이제는300자를넘겨야하네요조금은엉성한코드일수있겠지만아무쪼록잘부탁드립니다.300자를넘기기위해선어떤이야기를해야할까요?오늘점심은제가우육면을먹었어요사실다적었었는데중간에라이브쉐어가꺼져서300자를적어놓고82자밖에남지않았어요너무억울해서조금만복붙하겠습니다";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

재밌어서 계속 봤네용 ㅋㅋㅋ 그치만 DESCRIPTION_LENGTH_MAX 는 200 자인데 오타가 난걸까요??

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

그리고 만약에 최대 글자수가 변경된다면 해당 문구도 변경되어야겠죠?? 그렇게 계속 유지보수를 두는것보다
const LONG_DESCRIPTION = "a".repeat(DESCRIPTION_LENGTH_MAX + 1);
이렇게 해도 좋을거 같아요

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.

케빈의 말씀대로 처음엔 300자였다가 200자로 줄어들었는데 이를 반영하지 못했습니다😢 정말로 유지보수에 어려움이 있는 코드네요

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.

9cdd153 동적 최대글자 반영 완료했습니다!

}).toThrow(ERROR_MESSAGE.NAME_LENGTH_MAX);
});

test(`${DESCRIPTION_LENGTH_MAX}자를 넘는 가게 이름은 받을 수 없다.`, () => {
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.

20237eb 테스트 명세 반영완료했습니다!

src/main.js Outdated
Comment on lines +14 to +16
addEventListener("keydown", (e) => {
if (e.key === "Escape") Modal.close();
});
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.

큰 고려없이 빠른 구현을 위해 주었습니다😢 모달이 열렸을 때만 이벤트가 주어지도록 보완해야겠네요..!

Copy link
Copy Markdown
Author

@MinSungJe MinSungJe Mar 9, 2025

Choose a reason for hiding this comment

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

a05627d 모달이 열릴때만 이벤트리스너를 부여했습니다. 또 사이드 이펙트를 고려하여 닫힐 때 해당 이벤트리스너를 삭제하도록 구현했습니다.

Comment on lines +24 to +34
export function renderRestaurantList() {
DOM.$restaurantList.replaceChildren();
state.restaurantList.forEach(
({ src, name, distance, description, label }) => {
render(
LunchInfoCard.create({ src, name, distance, description, label }),
DOM.$restaurantList
);
}
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이건 개인마다 차이가 있을 수 있다고 생각하는데.. 저는 이 코드가 눈에 잘 안들어오더라구요
그래서 이게 무슨코드인지 보면서 이해했는데, 이렇 방식도 좋지만

  const restaurantElements = state.restaurantList.map(({ src, name, distance, description, label }) =>
    LunchInfoCard.create({ src, name, distance, description, label })
  );

  restaurantElements.forEach((element) => {
    render(element, DOM.$restaurantList);
  });

이렇게 배열을 map으로 변환 후 .forEach에서 appendChild 하는 방식은 어떨까요?

Copy link
Copy Markdown
Author

@MinSungJe MinSungJe Mar 8, 2025

Choose a reason for hiding this comment

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

25bde2e 확실히 만들어서 띄운다는 과정이 보여서 좋네요!

src/main.js Outdated
});
renderRestaurantList();

DOM.$main.append(Modal.create(AddLunchModalForm.create()));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이렇게 직접 DOM.$main.append()에 추가되는 것도 좋지만

function initModal() {
  const modalContent = AddLunchModalForm.create();
  const modalElement = Modal.create(modalContent);
  DOM.$main.append(modalElement);
}

initModal();

이렇게 하는게 나중에 유지보수에도 도움이 될거 같아요~

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.

13ea78b 반영했습니다!
🤔 개인적인 궁금증이 있는데, 유지보수에 도움이 된다는게 Content와 Element를 만드는 과정을 보일 수 있기 때문인가요??

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

네! 그 의도로 말한게 맞아요! 유지보수에 도움이 된다는 이유는 Content와 Element를 생성하는 과정을 명확하게 분리할 수 있기 때문이라고 생각했어요

단순히 DOM.$main.append(Modal.create(AddLunchModalForm.create()))처럼 한 줄로 작성하면,

모달의 컨텐츠가 어떻게 생성되는지 명확하지 않고, 모달 생성 로직과 추가 로직이 한 줄에서 이루어지기 때문에 재사용성이 떨어질 가능성이 있어요.
하지만 initModal() 함수를 만들어 모달 컨텐츠를 만들고, 이를 Modal.create()를 통해 모달 요소로 변환한 후, DOM에 추가하는 흐름을 명확히 해두면, 각 단계의 역할이 분명해져서 가독성이 향상되고, 모달의 컨텐츠를 변경해야 할 경우에도 initModal() 내부만 수정하면 되므로 유지보수가 쉬워진다고 생각합니다. 따라서 나중에 다른 모달을 추가해야 할 경우, initModal()과 비슷한 패턴을 따라가면 일관성을 유지할 수 있다고 생각해요.

즉, 유지보수에 도움이 된다는 것은 코드를 이해하기 쉽고, 변경이 용이한 구조로 만든다는 의미이고, initModal()을 분리하는 것이 이러한 목표에 부합한다고 볼 수 있어요

Comment on lines +21 to +44
event.preventDefault();
const formData = new FormData(event.target);
const { category, description, distance, link, name } =
Object.fromEntries(formData.entries());

try {
Validator.name(name);
if (link !== "") Validator.link(link);
if (description !== "") Validator.description(description);

state.restaurantList.push({
src: CATEGORY_ICON[category],
name: name,
distance,
description,
label: category,
});

renderRestaurantList();
Modal.close();
} catch (e) {
alert(e.message);
}
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이렇게 코드를 작성하는게 절대 안좋다는건 아니지만, 계속적으로 코드가 확장되면서 분명히 나중에 유지보수할때 어려움을 겪을거라 확신합니다.. 이렇게 handleSubmit(), validateFormInputs(), addRestaurant()로 역할이 나누어 작성해보면 어떨까요?? 예시 코드를 짧게 짜면

function handleSubmit(event) {
  event.preventDefault();
  const formData = new FormData(event.target);
  const { category, description, distance, link, name } = Object.fromEntries(formData.entries());

  try {
    validateFormInputs({ category, description, distance, link, name });
    addRestaurant({ category, description, distance, link, name });
    Modal.close();
  } catch (e) {
    alert(e.message);
  }
}

function validateFormInputs({ name, link, description }) {
  Validator.name(name);
  if (link !== "") Validator.link(link);
  if (description !== "") Validator.description(description);
}

function addRestaurant({ category, description, distance, link, name }) {
  state.restaurantList.push({
    src: CATEGORY_ICON[category],
    name,
    distance,
    description,
    label: category,
  });
  renderRestaurantList();
}

이렇게 될거 같아요 이런 코드는 어떠신가요?

Copy link
Copy Markdown
Author

@MinSungJe MinSungJe Mar 8, 2025

Choose a reason for hiding this comment

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

e7f5f0d 확실히 긴 기능을 3개로 나누어서 관리하면 유지보수에 이점이 있어보이네요!🙂

},
};

export default AddLunchModalForm;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이 파일이 엄청 길어졌는데 대부분 중요한 코드가 있기 보단 카테고리에 들어가는 하드 코딩된 값들때문이라고 생각하는데, 해당코드를 constants 로 빼면 어떨까요?

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.

6491954 반영했습니다! 덕분에 코드의 길이를 줄일 수 있었네요.

},
};

export default Modal;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

궁금증) 이런 구조라면 Modal 이 중복해서 여러개 생길 위험은 없을까요?

Copy link
Copy Markdown
Author

@MinSungJe MinSungJe Mar 9, 2025

Choose a reason for hiding this comment

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

이미 생성되어 있는 Modal에 modal--open, modal--close 클래스를 주는 거라, Modal이 하나만 있을 경우는 상관없겠지만, 만약 Modal class를 가진 요소가 여러개라면 같이 조작될 위험이 있겠네요😢

Copy link
Copy Markdown
Author

@MinSungJe MinSungJe Mar 9, 2025

Choose a reason for hiding this comment

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

fbd0346 모달별로 다른 id를 주어 하나의 모달에만 적용될 수 있게 반영했습니다!

@MinSungJe
Copy link
Copy Markdown
Author

다음번에는 배포한 링크를 기입해주시면 좋을거같아요~

https://minsungje.github.io/javascript-lunch/
헉 반영을 깜빡했네요 ㅜ PR 본문에도 반영하고, 코멘트에도 링크를 달아둡니다😢

LunchInfoCard를 만들고 렌더링 하는 과정을 분리
submit 연결, 입력값 검증, 음식점 리스트 추가 함수 기능 분리
2개의 서로 다른 Modal을 구분하기 위해 고유 id를 부여하도록 구현
이를 통해 2개의 모달이 동시에 열리거나 닫히는 현상을 방지 가능
Modal 중 AddLunchModal 관련 파일은 폴더로 묶어 정리
Copy link
Copy Markdown

@JeongBin0227 JeongBin0227 left a comment

Choose a reason for hiding this comment

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

안녕하세요 밍고!

코멘트가 다소 많을수 있다고 생각했는데 잘 반영해주셔서 감사합니다~
또 배포해주신 주소로 확인했는데 잘 동작하는거 같아요 👍

1단계는 여기서 머지하도 될거같아요~ 2단계에서 만나요 💯

@JeongBin0227 JeongBin0227 merged commit 2d89a19 into woowacourse:minsungje Mar 10, 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.

3 participants