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

[11팀 임재도] [Chapter 2-2] 디자인 패턴과 함수형 프로그래밍 #8

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

effozen
Copy link

@effozen effozen commented Jan 13, 2025

과제 체크포인트

기본과제

  • React의 hook 이해하기

  • 함수형 프로그래밍에 대한 이해

  • Component에서 비즈니스 로직을 분리하기

  • 비즈니스 로직에서 특정 엔티티만 다루는 계산을 분리하기

  • Component에서 사용되는 Data가 아닌 로직들은 hook으로 옮겨졌나요?

  • 주어진 hook의 책임에 맞도록 코드가 분리가 되었나요?

  • 계산함수는 순수함수로 작성이 되었나요?

심화과제

  • 뷰데이터와 엔티티데이터의 분리에 대한 이해

  • 엔티티 -> 리파지토리 -> 유즈케이스 -> UI 계층에 대한 이해

  • Component에서 사용되는 Data가 아닌 로직들은 hook으로 옮겨졌나요?

  • 주어진 hook의 책임에 맞도록 코드가 분리가 되었나요?

  • 계산함수는 순수함수로 작성이 되었나요?

  • 특정 Entitiy만 다루는 함수는 분리되어 있나요?

  • 특정 Entitiy만 다루는 Component와 UI를 다루는 Component는 분리되어 있나요?

  • 데이터 흐름에 맞는 계층구조를 이루고 의존성이 맞게 작성이 되었나요?

과제 셀프회고

많은 시간을 투자했지만.. AdminPage는 제대로 리팩토링 하지 못하고 CartPage 위주로 진행한 결과가 나왔습니다. 아무래도 리팩토링은 전체적인 요소를 다루거나, 어느정도의 기준이 있어야 했는데, 잘하고 있다고 생각했는데, 어느 하나에 깊게 매몰되었던 것 같아 아쉽습니다.

간략한 요약

  1. 지난주의 아쉬움을 개선한 것
  2. 시간 문제로 AdminPage에 대해 제대로 리팩토링 하지 못한게 아쉽다.
  3. 함수형 프로그래밍이 무엇인지 알았다.
  4. FSD가 무엇인지 맛을 보았다.
  5. Zustand를 통해 전역 상태 관리 라이브러리를 처음으로 맛보았다.
  6. 팀원들과 클린코드 등에 대해 논의하면서, 보다 넓은 시각을 갖게 되었다.




과제에서 좋았던 부분 & 새롭게 알게 된 점

  1. 지난주의 아쉬움을 개선한 것

지난 주의 리팩토링 과정에서 원본 코드의 기능을 보장하면서 개선하기 보다는 일부, 원본 코드를 훼손하면서 처음부터 만들듯이 작성했던 순간이 있었습니다.

아쉬움이 많았고, 이번에는 최대한 원본 요소를 살리되, 기능의 변경이 있다면 천천히 하나씩 주석 처리하면서 점진적으로 바꿔가자! 라고 생각하게 되었습니다.

그래서, 제 코드를 보면 원본 코드의 요소가 많이 담겨져있는데.. 그게 그런 이유 입니다.

양파의 껍질을 벗기듯 껍데기부터 하나씩 분리하고자 했고, 중간에 중복되는 로직이 보이면 이를 개선하고자 했습니다.

그렇게 천천히 개선한 결과 나름의 만족스러운 과정과 결과가 나와서 기쁩니다.



  1. 동료와 함께할 때 배가 되는 성장
스크린샷 2025-01-16 오후 8 47 02

팀원들과 함께 "쏙쏙 들어오는 함수형 코딩" 책을 스터디하고 있습니다.

매일 1챕터씩 읽고 지정된 분이 발표를 하는 형식으로 진행합니다.

이를 통해서, 도통 감이 잡히지 않던 함수형 프로그래밍이 무엇인지 감을 잡을 수 있었습니다.

과제의 주제가 "디자인 패턴과 함수형 프로그래밍"이고, 테오가 평소 강조했던 "잦은 소통" 측면에서 스터디와 병행하니 이론을 배우면서도, 과제에 대한 논의를 통해 실전을 생각하고, 그 반대도 수립하는 이상적인 형태가 되었습니다

그래서 이번 주는 첫날보다, 목요일, 금요일로 갈 수록 훨씬 성장해서.. 첫날에는 보이지 않고 이해할 수 없던 영역들이 PR은 적는 지금 감을 잡고 더 시도해보고 싶어졌다는 게 매우 좋은 것 같습니다.



  1. 고정된 생각에서 벗어나다.

"객체지향이 최고야!"

"함수형 프로그래밍이 최고야!"

이런 편협된 생각에서 벗어날 수 있었습니다. 제일 좋은 것은 무난한 코드라는 것. 그 말처럼 어느 하나에 갇히는 일 없이 유연하게 사고할 수 있는 방법을 배웠다는 게 좋은 것 같습니다.

함수형 프로그래밍을 공부하면서도 저랑 팀원들이 생각했던 것은 "모든 것을 함수형 프로그래밍으로 하자!"가 아닌, 주어진 상황에서 계산과 액션, 데이터를 분리하고 그 속에서 순수함수나 데이터 불변성을 보장하고, 같은 추상화 계층을 보장하는 것. 정도를 신경썼던 것 같습니다

이유는 명확했는데, 결국은 DX 측면에서의 가독성이나 버그 가능성을 최소화하기 위함이었습니다.

이 처럼 목적에 맞는 유연한 사고를 할 수 있게 된게 좋았습니다.



  1. FSD 가 무엇인지 맛을 보다.

다음 주 주제가 FSD 로 알고 있습니다.

그럼에도 먼저 시도한 것은, 추상화나, 함수형 코딩에 있는 액션, 데이터, 계산의 분리 등. 이런 것에 대해서 조금 더 깊게 이해하기 위함이었습니다.

팀원들과 대화를 통해 FSD의 철학이 추상화 같은 요소를 학습하는 데 좋다고 생각헀습니다.

그에 따라서, 11팀 모두가 함수형 코딩, FSD에 도전하게 되었습니다.

그리고, 함수형 프로그래밍 스터디와 병행하면서 아키텍쳐 측면에서도 추상화가 뭔지 논의를 하였고.. 재밌게도 함수형 프로그래밍만 공부해서는 이해되지 않는게 FSD의 개념을 일부 끌고오니 이해가 되고, 그 반대의 경우도 생겼습니다.

결국 조금은 결이 다른 요소를 학습하고 시도하는게 시너지 효과를 내어서 FSD나 함수형 프로그래밍 둘 모두를 보다 제대로 이해할 수 있는 계기가 되었습니다.



  1. Zustand를 통해 전역 상태 관리 라이브러리를 처음으로 맛보았다.
const App = () => {
  const { products, updateProduct, addProduct } = useProducts(initialProducts);
  const { coupons, addCoupon } = useCoupons(initialCoupons);

App.tsx 코드를 보면 위와 같은 요소가 있습니다.

이는, AdminPage, CartPage에 전달되어서 props 드릴링 문제를 낳습니다.

자연스럽게 관련된 모든 컴포넌트가 렌더링이 되고, 심지어 읽기 어렵게 하는 현상이 발생합니다.


코드를 분리하는 과정에서, 이 두가지 훅이 분리를 어렵게 만들었고, 심한 경우는 데이터가 제대로 다른 모듈로 전달되지 않아서 오류가 발생하기도 했습니다.

그에 따라서 zustand를 이용해서 전역 상태로써 관리를 시도했습니다.


그리고, 다른 상태들에도 적용해보았습니다.

처음에는 전역상태에 대해서 제대로 이해하지 못해, 다음과 같은 코드를 작성하기도 했습니다.

// useCartStore.ts
export const useCartStore = create<CartState>((set, get) => ({
  cart: [],
  selectedCoupon: null,

  .... 중략 ....

  // 총 결제 금액(할인 전/후, 할인액 계산) 함수
  calculateTotal: () => {
    const { cart, selectedCoupon } = get();
    let totalBeforeDiscount = 0;
    let totalAfterDiscount = 0;

    cart.forEach((item) => {
      const { price, discounts } = item.product;
      const quantity = item.quantity;
      totalBeforeDiscount += price * quantity;

      // 상품 할인 적용: 주문 수량에 맞게 최대 할인율 적용
      const discount = discounts.reduce((maxDiscount, d) => {
        return quantity >= d.quantity && d.rate > maxDiscount ? d.rate : maxDiscount;
      }, 0);
      totalAfterDiscount += price * quantity * (1 - discount);
    });

    let totalDiscount = totalBeforeDiscount - totalAfterDiscount;

    // 쿠폰 할인 적용
    if (selectedCoupon) {
      if (selectedCoupon.discountType === 'amount') {
        totalAfterDiscount = Math.max(0, totalAfterDiscount - selectedCoupon.discountValue);
      } else {
        totalAfterDiscount *= 1 - selectedCoupon.discountValue / 100;
      }
      totalDiscount = totalBeforeDiscount - totalAfterDiscount;
    }

    return {
      totalBeforeDiscount: Math.round(totalBeforeDiscount),
      totalAfterDiscount: Math.round(totalAfterDiscount),
      totalDiscount: Math.round(totalDiscount),
    };
  },
}));

그러나, 사용을 해보면서 점점 이해가 깊어지면서, 보다 나은 코드를 작성할 수 있었습니다.

아직도 감을 잡는 단계이지만, 이번 주에도 프론트엔드 개발에 꼭 필요한 무언가를 하나 더 배웠다는 측면에서 굉장히 기쁜 것 같습니다.



과제를 진행하면서 아직 애매하게 잘 모르겠다 하는 점, 혹은 뭔가 잘 안되서 아쉬운 것들

제일 처음 서술한 내용입니다..!

많은 시간을 투자했지만.. AdminPage는 제대로 리팩토링 하지 못하고 CartPage 위주로 진행한 결과가 나왔습니다. 아무래도 리팩토링은 전체적인 요소를 다루거나, 어느정도의 기준이 있어야 했는데, 잘하고 있다고 생각했는데, 어느 하나에 깊게 매몰되었던 것 같아 아쉽습니다.

매 주 시작에는 아무것도 모른 채 과제를 이해하고, 관련 개념을 학습하기 급급한 것 같습니다.

그리고, 한 주가 끝날때 쯤이면 어느정도 학습이 이루어져.. 무언가를 하게 되는 것 같습니다.

이게 너무 아쉬운데.. 돌아오는 토요일에는 이 사이클을 조금 더 당겨서 주말에는 바짝 해서 1차적으로 끝내고, 평일에는 1차적으로 끝낸 요소를 개선하는 방향으로 진행해보고자 합니다.



리뷰 받고 싶은 내용이나 궁금한 것에 대한 질문

FSD에 대한 질문 - entities와 features의 분리에 대한 조언을 구하고 싶습니다.

스크린샷 2025-01-17 오전 8 02 52

현재 제 코드 구조입니다.

분리를 하다보니, 위와 같은 형태가 되었습니다.

calculate, store 등의 요소가 각각의 데이터에 대한 처리 로직처럼 생각되었습니다.

그래서, 뭔가 연산이자, 기능을 담당하기도 해서 features 인가? 싶다가도 entities 즉, 데이터 조작과 깊게 연관이 있는 것 같아서 entities로 묶어주다보니, 결과적으로 위와 같이 features는 비어버리고, entities에 모두 포함되는 구조가 나왔습니다.

이와 관련해서 어떻게 접근했어야 좋을지.. 그리고 현재의 상태에서 어떻게 생각하고, 접근해서 개선하면 좋을지 여쭙고 싶습니다..!

- 준만님께서 설정해준 내용을 적극 참조
- 추가 수정을 위한 전초 작업
@effozen effozen changed the title [WIP][11팀 임재도] [Chapter 2-2] 디자인 패턴과 함수형 프로그래밍 [11팀 임재도] [Chapter 2-2] 디자인 패턴과 함수형 프로그래밍 Jan 16, 2025
@effozen
Copy link
Author

effozen commented Jan 16, 2025

commit의 마지막에 chore가 많이 보이는데... 제 PC에서는 문제가 없으나, workflow에서 계속 airbnb와 eslint 버전이 충돌하는 에러가 있어서.. 시간 관계상 끝끝내 airbnb 가이드를 걷어냈습니다.

관련 요소로 버전 충돌이 원인이고, 제 pc에서 된 이유는 pnpm의 이런 버전 충돌에 대한 안정성 때문으로 간주되는데.. 주말에 좀 더 원인을 제대로 파악해보고자 합니다.

@junman95
Copy link

commit의 마지막에 chore가 많이 보이는데... 제 PC에서는 문제가 없으나, workflow에서 계속 airbnb와 eslint 버전이 충돌하는 에러가 있어서.. 시간 관계상 끝끝내 airbnb 가이드를 걷어냈습니다.

관련 요소로 버전 충돌이 원인이고, 제 pc에서 된 이유는 pnpm의 이런 버전 충돌에 대한 안정성 때문으로 간주되는데.. 주말에 좀 더 원인을 제대로 파악해보고자 합니다.

저랑 설정이 얼마나 다르신지는 모르겠는데
c8ac785

저는 해당패키지 버전 낮춰서 해결했습니다.

airbnb....

@junman95
Copy link

새로 출근하게 되셨는데도 많이하셨네요 역시... 네웹 가는사람은 다른거신가...

Copy link

@osohyun0224 osohyun0224 left a comment

Choose a reason for hiding this comment

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

안녕하세요 재도님 :) 담당 학습메이트 오소현입니다~!
이번주차 과제 리뷰 하면서 Zustand를 사용하시고, FSD구조로 분리하신게 인상 깊었습니다 !!
다음주차때 FSD 하는 것으로 알고 있는데 미리 해보시면서 리팩토링 해보신게 어떠셨을지 궁금하네요 :)
버튼 컴포넌트 분리하신 것도 좋았습니다 bb 다만 타입 체크 부분이랑 useEffect 사용하신게 궁금해서 조금 더 살펴보았어요~!
테스트 코드도 엄청 잘 작성해주셨네요 bb 시나리오도 명확해서 테스트 하려는 함수가 어떤 역할을 하고 있는지, 어떤 기댓값을 내야하는지 잘 파악할 수 있엇던 것 같습니다
부족하지만 제 리뷰가 이번주 과제를 다시한번 더 생각해보는 좋은 계기가 되었으면 해요 ㅎㅎ 이번주 과제하느라 넘고생많으셨습니다 재도님 다음주 과제도 화이팅 입니다 :)

그리고 첫 출근도 다시한번 더 축하드립니다🎉🎉 회사 병행이 쉽지 않겠지만 옆에서 잘 서포트 하겠습니다 !


// 4. 할인율 계산
// 4. 할인율 계산 (CartPage 내부에서 주문 요약 영역에 결과가 반영된다고 가정)

Choose a reason for hiding this comment

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

테스트 코드 주석에도 자세하게 추가 설명을 달아주셨군요 ㅎㅎㅎ 좋습니다 :)

Comment on lines +360 to +362
test('새로운 hook 함수르 만든 후에 테스트 코드를 작성해서 실행해보세요', () => {
expect(true).toBe(true);
});

Choose a reason for hiding this comment

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

엇 요거는 어떤 역할을 하기 위해서 들어간 테케일가용?!

Copy link
Author

Choose a reason for hiding this comment

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

사실 이 부분은 뒤에 요소들이 요구사항에 부합한다는 것을 서술하기 위한 맥락이었는데, 생각해보니 좋지 못한 코드인거 같아요. 다른 방법으로 충분히 표현이 가능했을것 같구요.. description같은....?

시간이 없어서.. 이렇게 했는데 진짜 아쉬움이 많이 남습니다..


describe('함수 테스트 > ', () => {
describe('calculateTotal > ', () => {
test('가격과 수량에 따라 올바른 총액을 계산해야 한다', () => {

Choose a reason for hiding this comment

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

재도님도 테스트 시나리오 작성하실때 ~[조건]하면, ~게 되어야한다. 이런식으로 기댓값을 적어주시는 군요 ㅎㅎㅎ 기대값을 테스트 시나리오로 작성해주면 더 파악하기 좋은 것 같아요 bb 좋습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

오 감사합니다 ㅎㅎ. 여러가지로 방법을 시도중인데, 이 방법이 좀 더 직관적으로 다가가는 느낌이네요..!

간단하게 적은 부분도 있고 그런데.. 테스트케이스를 유닛 단위에 맞추어서 유저시나리오 라고 하나요...? 그렇게 한번 적는 습관을 들여야겠습니다 ㅎㅎ..

@@ -78,11 +78,11 @@ export const AdminPage = ({ products, coupons, onProductUpdate, onProductAdd, on
};

const handleAddDiscount = (productId: string) => {
const updatedProduct = products.find(p => p.id === productId);
const updatedProduct = products.find((p) => p.id === productId);

Choose a reason for hiding this comment

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

p라는 변수명이 원래 제시된 변수명이셨군요 저도 과제할때 p가 의미를 파악하기 힘들 것 같아서 전체 다 product로 바꿔줬던거 같아요 ㅎㅎ 무엇을 참조하는지 명확히 해주는 것도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

이 부분에서 사실 좀 많이 해멨습니다.. 바꿨는데도 p가 테스트케이스를 깨트리는 경우가 많았어서... 그래도 과제의 의도를 최대한 보장하려고 그대로 살려서 구현했습니다 ㅎㅎ... 참조를 조금 더 명확히 해주는거.. 간과하고 있었는데 다시금 상기해야할 것 같네요..!  👍

Comment on lines +15 to +53
export const calculateCartTotal = (cart: CartItem[], selectedCoupon: Coupon | null) => {
let totalBeforeDiscount = 0;
let totalAfterDiscount = 0;

cart.forEach((item) => {
const { price } = item.product;
const { quantity } = item;
// 각 항목의 총액 계산
const itemTotal = calculateTotal(price, quantity);
totalBeforeDiscount += itemTotal;

// 적용 가능한 할인율 결정
const discount = item.product.discounts.reduce((maxDiscount, d) => {
return quantity >= d.quantity && d.rate > maxDiscount ? d.rate : maxDiscount;
}, 0);

// 각 항목에 할인 적용
totalAfterDiscount += itemTotal * (1 - discount);
});

// 할인 금액 계산
let totalDiscount = totalBeforeDiscount - totalAfterDiscount;

// 쿠폰 적용
if (selectedCoupon) {
if (selectedCoupon.discountType === 'amount') {
totalAfterDiscount = Math.max(0, totalAfterDiscount - selectedCoupon.discountValue);
} else {
totalAfterDiscount *= (100 - selectedCoupon.discountValue) / 100;
}
totalDiscount = totalBeforeDiscount - totalAfterDiscount;
}

return {
totalBeforeDiscount: Math.round(totalBeforeDiscount),
totalAfterDiscount: Math.round(totalAfterDiscount),
totalDiscount: Math.round(totalDiscount),
};
};

Choose a reason for hiding this comment

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

총액을 계산하는 calculateCartTotal 함수 내부에서 다양한 계산을 해주고 있군요 !
저는 이 내부에서 단일로 계산되는 함수는 다 분리했었는데 요렇게는 어떤가요? 단일 함수로 분리해두면 다른 기능에서 사용하기 좋을 것 같아서요 ㅎㅎㅎ

Suggested change
export const calculateCartTotal = (cart: CartItem[], selectedCoupon: Coupon | null) => {
let totalBeforeDiscount = 0;
let totalAfterDiscount = 0;
cart.forEach((item) => {
const { price } = item.product;
const { quantity } = item;
// 각 항목의 총액 계산
const itemTotal = calculateTotal(price, quantity);
totalBeforeDiscount += itemTotal;
// 적용 가능한 할인율 결정
const discount = item.product.discounts.reduce((maxDiscount, d) => {
return quantity >= d.quantity && d.rate > maxDiscount ? d.rate : maxDiscount;
}, 0);
// 각 항목에 할인 적용
totalAfterDiscount += itemTotal * (1 - discount);
});
// 할인 금액 계산
let totalDiscount = totalBeforeDiscount - totalAfterDiscount;
// 쿠폰 적용
if (selectedCoupon) {
if (selectedCoupon.discountType === 'amount') {
totalAfterDiscount = Math.max(0, totalAfterDiscount - selectedCoupon.discountValue);
} else {
totalAfterDiscount *= (100 - selectedCoupon.discountValue) / 100;
}
totalDiscount = totalBeforeDiscount - totalAfterDiscount;
}
return {
totalBeforeDiscount: Math.round(totalBeforeDiscount),
totalAfterDiscount: Math.round(totalAfterDiscount),
totalDiscount: Math.round(totalDiscount),
};
};
const calculateItemTotal = (item: CartItem): number => {
const { price, quantity } = item;
return calculateTotal(price, quantity);
};
const applyDiscounts = (item: CartItem, itemTotal: number): number => {
const discountRate = item.product.discounts.reduce((maxDiscount, discount) => {
return quantity >= discount.quantity && discount.rate > maxDiscount ? discount.rate : maxDiscount;
}, 0);
return itemTotal * (1 - discountRate);
};
const applyCoupon = (total: number, coupon: Coupon | null): number => {
if (!coupon) return total;
if (coupon.discountType === 'amount') {
return Math.max(0, total - coupon.discountValue);
} else {
const discountFactor = (100 - coupon.discountValue) / 100;
return total * discountFactor;
}
};
export const calculateCartTotal = (cart: CartItem[], selectedCoupon: Coupon | null) => {
let totalBeforeDiscount = cart.reduce((acc, item) => acc + calculateItemTotal(item), 0);
let totalAfterDiscount = cart.reduce((acc, item) => acc + applyDiscounts(item, calculateItemTotal(item)), 0);
let totalDiscount = totalBeforeDiscount - totalAfterDiscount;
totalAfterDiscount = applyCoupon(totalAfterDiscount, selectedCoupon);
totalDiscount = totalBeforeDiscount - totalAfterDiscount;
return {
totalBeforeDiscount: Math.round(totalBeforeDiscount),
totalAfterDiscount: Math.round(totalAfterDiscount),
totalDiscount: Math.round(totalDiscount),
};
};

저는 우선 간단히 내부에서 분리만 해뒀구요! 외부로 빼서 함수들만 가져와 사용하면 좋을 것 같습니다 :)

Comment on lines +15 to +18
// 이미 있는 getAppliedDiscount를 수정하지 않고 재사용하기 위함
export const getMaxApplicableDiscount = (item: CartItem) => {
return getAppliedDiscount(item);
};

Choose a reason for hiding this comment

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

오 약간 이 함수의 이유가 궁금한데 질문 드려도 되나용?
테스트 코드에서만 사용하고 getAppliedDiscount 함수만을 호출하는데 이유가 있을까용?


const { coupons, newCoupon, setNewCoupon, handleAddCoupon } = useAdminCoupons();

// 항상 state가 많으면 의심하게 되는 부분이 굳이 이런 자료가 필요한가이다.

Choose a reason for hiding this comment

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

고민하신 흔적이 보이는 군요 ㅎㅎㅎ 너무 고생하셨습니다 !

Comment on lines +32 to +54
<div className="mt-6 bg-white p-4 rounded shadow">
<h2 className="text-2xl font-semibold mb-2">쿠폰 적용</h2>
<select
onChange={(e) => applyCoupon(coupons[parseInt(e.target.value)])}
className="w-full p-2 border rounded mb-2"
>
<option value="">쿠폰 선택</option>
{coupons.map((coupon, index) => (
<option key={coupon.code} value={index}>
{coupon.name} -{' '}
{coupon.discountType === 'amount' ? `${coupon.discountValue}원` : `${coupon.discountValue}%`}
</option>
))}
</select>
{selectedCoupon && (
<p className="text-green-600">
적용된 쿠폰: {selectedCoupon.name}(
{selectedCoupon.discountType === 'amount'
? `${selectedCoupon.discountValue}원`
: `${selectedCoupon.discountValue}%`}{' '}
할인)
</p>
)}

Choose a reason for hiding this comment

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

나중에 이 컴포넌트 형태를 기준으로 FSD에 맞게 리팩토링해보면 좋지 않을까 생각이 듭니다!?

Comment on lines +1 to +27
import { ButtonHTMLAttributes, useState, useEffect, ReactNode } from 'react';

interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
variant?: 'primary' | 'danger';
className?: string;
onClick?: () => void;
children: ReactNode;
}

export const Button = ({ variant, className, onClick, children, ...props }: ButtonProps) => {
const [tmpClassName, setTmpClassName] = useState<string>('');

useEffect(() => {
if (variant === 'primary') {
setTmpClassName('bg-gray-300 text-gray-800 px-2 py-1 rounded mr-1 hover:bg-gray-400');
}
if (variant === 'danger') {
setTmpClassName('bg-red-500 text-white px-2 py-1 rounded hover:bg-red-600');
}
}, []);

return (
<button className={tmpClassName} onClick={onClick} {...props}>
{children}
</button>
);
};

Choose a reason for hiding this comment

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

오 버튼 공용 컴포넌트에 useEffect와 useState를 사용하셔서 스타일을 변경하시려고 한 이유가 궁금해요!
제가 생각해봤을 땐 재도님께서 useEffect를 사용해서 컴포넌트 마운트 시 초기 스타일을 설정하려는 의도이신것 같은데 맞을까용? 현재 useEffect의 의존성 배열이 비어 있는데 variant가 변경되어도 tmpClassName이 업데이트가 잘 되는지도 궁금하네요 variant에 따라 클래스 이름을 결정하게 바꾸면 좋을 것 같은데 한번 제가 제안하는 코드를 봐주세용!

Suggested change
import { ButtonHTMLAttributes, useState, useEffect, ReactNode } from 'react';
interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
variant?: 'primary' | 'danger';
className?: string;
onClick?: () => void;
children: ReactNode;
}
export const Button = ({ variant, className, onClick, children, ...props }: ButtonProps) => {
const [tmpClassName, setTmpClassName] = useState<string>('');
useEffect(() => {
if (variant === 'primary') {
setTmpClassName('bg-gray-300 text-gray-800 px-2 py-1 rounded mr-1 hover:bg-gray-400');
}
if (variant === 'danger') {
setTmpClassName('bg-red-500 text-white px-2 py-1 rounded hover:bg-red-600');
}
}, []);
return (
<button className={tmpClassName} onClick={onClick} {...props}>
{children}
</button>
);
};
import { ButtonHTMLAttributes, ReactNode } from 'react';
interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
variant?: 'primary' | 'danger';
className?: string;
onClick?: () => void;
children: ReactNode;
}
export const Button = ({ variant, className, onClick, children, ...props }: ButtonProps) => {
const getClassName = (variant: string | undefined) => {
switch (variant) {
case 'primary':
return 'bg-gray-300 text-gray-800 px-2 py-1 rounded mr-1 hover:bg-gray-400';
case 'danger':
return 'bg-red-500 text-white px-2 py-1 rounded hover:bg-red-600';
default:
return '';
}
};
const tmpClassName = `${getClassName(variant)} ${className || ''}`;
return (
<button className={tmpClassName} onClick={onClick} {...props}>
{children}
</button>
);
};

Choose a reason for hiding this comment

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

이건진짜 깔끔하네요

Switch case 에서 한단계 나아가서 키밸류 형태의 맵 자료형이나 객체도 관리하기 좋아보입니다.
switch문은 함수내부에서만 사용가능하니까 분리가 . 좀어렵죠

@osohyun0224
Copy link

📦src
 ┣ 📂refactoring
 ┃ ┣ 📂app
 ┃ ┃ ┗ 📜App.tsx
 ┃ ┣ 📂entities
 ┃ ┃ ┣ 📂cart
 ┃ ┃ ┃ ┣ 📂model
 ┃ ┃ ┃ ┃ ┣ 📜caculate.ts
 ┃ ┃ ┃ ┃ ┣ 📜cartStore.tsx
 ┃ ┃ ┃ ┃ ┣ 📜discount.ts
 ┃ ┃ ┃ ┃ ┣ 📜index.ts
 ┃ ┃ ┃ ┃ ┗ 📜update.ts
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┣ 📂coupon
 ┃ ┃ ┃ ┣ 📂model
 ┃ ┃ ┃ ┃ ┣ 📜couponStore.tsx
 ┃ ┃ ┃ ┃ ┣ 📜index.ts
 ┃ ┃ ┃ ┃ ┗ 📜initialCoupons.ts
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┗ 📂product
 ┃ ┃ ┃ ┣ 📂model
 ┃ ┃ ┃ ┃ ┣ 📜index.ts
 ┃ ┃ ┃ ┃ ┣ 📜initialProducts.ts
 ┃ ┃ ┃ ┃ ┗ 📜productsStore.tsx
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┣ 📂pages
 ┃ ┃ ┣ 📂AdminPage
 ┃ ┃ ┃ ┣ 📂ui
 ┃ ┃ ┃ ┃ ┣ 📜AdminPage.tsx
 ┃ ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┃ ┣ 📂utils
 ┃ ┃ ┃ ┃ ┣ 📜index.ts
 ┃ ┃ ┃ ┃ ┗ 📜useAdminCoupon.tsx
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┗ 📂CartPage
 ┃ ┃ ┃ ┣ 📂ui
 ┃ ┃ ┃ ┃ ┣ 📜CartPage.tsx
 ┃ ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┣ 📂shared
 ┃ ┃ ┣ 📂constants
 ┃ ┃ ┃ ┣ 📜errorMessage.ts
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┣ 📂libs
 ┃ ┃ ┃ ┣ 📜calculate.ts
 ┃ ┃ ┃ ┣ 📜index.ts
 ┃ ┃ ┃ ┗ 📜item.ts
 ┃ ┃ ┣ 📂types
 ┃ ┃ ┃ ┣ 📜index.ts
 ┃ ┃ ┃ ┗ 📜types.ts
 ┃ ┃ ┗ 📂ui
 ┃ ┃ ┃ ┣ 📜Button.tsx
 ┃ ┃ ┃ ┣ 📜Header.tsx
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┣ 📂widgets
 ┃ ┃ ┣ 📂CartItem
 ┃ ┃ ┃ ┣ 📂ui
 ┃ ┃ ┃ ┃ ┣ 📜CartItem.tsx
 ┃ ┃ ┃ ┃ ┣ 📜GridContainer.tsx
 ┃ ┃ ┃ ┃ ┣ 📜GridItem.tsx
 ┃ ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┗ 📂SelectedItem
 ┃ ┃ ┃ ┣ 📂ui
 ┃ ┃ ┃ ┃ ┣ 📜SelectedItem.tsx
 ┃ ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┃ ┃ ┗ 📜index.ts
 ┃ ┗ 📜main.tsx

폴더 구조 코드로도 남겨봅니다 :) 옆에 주석으로 해당 역할들 작성해주시면 더 좋을 것 같아서 남겨요!

Copy link

@wonjung-jang wonjung-jang left a comment

Choose a reason for hiding this comment

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

재도님! 일단 인턴 합격하신 거 다시 한 번 축하드립니다!
첫 출근하기 전에 일할 거 미리 공부도 하시고, 출근하고 나서 정신없고 바쁘셨을 텐데 과제도 열심히 하셔서 멋있었어요!👍

setCoupons(initialCoupons);
}, []);

// 어드민일때랑 아닐 때 상태 분류

Choose a reason for hiding this comment

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

이런 주석은 이미 변수명으로 설명되지 않을까 싶습니답

Comment on lines +81 to +115
calculateTotal: () => {
const { cart, selectedCoupon } = get();
let totalBeforeDiscount = 0;
let totalAfterDiscount = 0;

cart.forEach((item) => {
const { price, discounts } = item.product;
const quantity = item.quantity;
totalBeforeDiscount += price * quantity;

// 상품 할인 적용: 주문 수량에 맞게 최대 할인율 적용
const discount = discounts.reduce((maxDiscount, d) => {
return quantity >= d.quantity && d.rate > maxDiscount ? d.rate : maxDiscount;
}, 0);
totalAfterDiscount += price * quantity * (1 - discount);
});

let totalDiscount = totalBeforeDiscount - totalAfterDiscount;

// 쿠폰 할인 적용
if (selectedCoupon) {
if (selectedCoupon.discountType === 'amount') {
totalAfterDiscount = Math.max(0, totalAfterDiscount - selectedCoupon.discountValue);
} else {
totalAfterDiscount *= 1 - selectedCoupon.discountValue / 100;
}
totalDiscount = totalBeforeDiscount - totalAfterDiscount;
}

return {
totalBeforeDiscount: Math.round(totalBeforeDiscount),
totalAfterDiscount: Math.round(totalAfterDiscount),
totalDiscount: Math.round(totalDiscount),
};
},

Choose a reason for hiding this comment

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

같은 폴더에 있는 caculate.ts 파일과 중복되는 로직인 것 같아요!
하나의 함수로 작성한 다음에 재활용하는 게 좋을 것 같습니다.

import { CartItem } from '@/shared/types';

export const updateCartItemQuantity = (cart: CartItem[], productId: string, newQuantity: number): CartItem[] => {
// cart에서 아이템을 찾는 로직

Choose a reason for hiding this comment

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

주석이 혼란을 유발하네요.
주석을 수정하거나 이미 함수명으로 충분해서 주석을 없애도 좋을 것 같습니다.

Comment on lines +12 to +15
useEffect(() => {
setProducts(initialProducts);
setCoupons(initialCoupons);
}, []);

Choose a reason for hiding this comment

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

리뷰때도 말씀드렸지만 텍스트로 남기기위해 다시한번 언급하겠습니다.. ( 알람이 간다면 죄송합니다 )

useEffect 를 최소화하고 initialValue 를 훅의 인자값으로 넣는게 더 좋아보입니다.!!

https://ko.react.dev/learn/you-might-not-need-an-effect

Choose a reason for hiding this comment

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

파일명에서 중복이 보이는것 같은데 도메인을 나타내는 slice 영역의 파일 네이밍을 통일하는게 어떨까요?

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.

5 participants