[3팀 이가은] Chapter 2-1. 클린코드와 리팩토링#36
[3팀 이가은] Chapter 2-1. 클린코드와 리팩토링#36tooth-is-silver wants to merge 63 commits intohanghae-plus:mainfrom
Conversation
|
고생하셨습니다 가은님! |
tomatopickles404
left a comment
tomatopickles404
left a comment
There was a problem hiding this comment.
이가튼튼님 구경왔숨니다~!
이번주 스트레스 많이 받아하셨는데 완주 하셔서 다행입니당ㅎ ㅎ
"나 이가은. 해냈다."
저의 부족한 식견이지만 조금 까불다 가보겠습니다..!
다음주도 화이팅이애요... ❤️
There was a problem hiding this comment.
우와 저는 테스트코드 못짰는데 이것까지 해내셨군요 👍👍
| const priceDisplay = useMemo(() => { | ||
| if (isOnLightningSale && isSuggestedSale) { | ||
| return ( | ||
| <> | ||
| <span className="line-through text-gray-400"> | ||
| ₩{originalPrice.toLocaleString()} | ||
| </span>{" "} | ||
| <span className="text-purple-600">₩{price.toLocaleString()}</span> | ||
| </> | ||
| ); | ||
| } else if (isOnLightningSale) { | ||
| return ( | ||
| <> | ||
| <span className="line-through text-gray-400"> | ||
| ₩{originalPrice.toLocaleString()} | ||
| </span>{" "} | ||
| <span className="text-red-500">₩{price.toLocaleString()}</span> | ||
| </> | ||
| ); | ||
| } else if (isSuggestedSale) { | ||
| return ( | ||
| <> | ||
| <span className="line-through text-gray-400"> | ||
| ₩{originalPrice.toLocaleString()} | ||
| </span>{" "} | ||
| <span className="text-blue-500">₩{price.toLocaleString()}</span> | ||
| </> | ||
| ); | ||
| } else { | ||
| return `₩${price.toLocaleString()}`; | ||
| } | ||
| }, [isOnLightningSale, isSuggestedSale, originalPrice, price]); |
There was a problem hiding this comment.
jsx를 메모이제이션 하셨군요! 뭔가 안티패턴 같긴 한데.. 메모가 필요했던 이유가 있을까요?
제 생각에는 jsx에 분기처리가 필요하다면, jsx 내부에서 조건부 렌더링을 하는게 가장 적절한 방식이 아닐까 생각합니다!
메모이제이션이 필요하다면 그 상태에서 각 컴포넌트에 memo를 사용하는건 어떨까요?
{isOnLightningSale && isSuggestedSale ? (
<SuperSalePrice price={price} originalPrice={originalPrice} />
) : isOnLightningSale ? (
<LightningSalePrice price={price} originalPrice={originalPrice} />
) : isSuggestedSale ? (
<SuggestedSalePrice price={price} originalPrice={originalPrice} />
) : (
<RegularPrice price={price} />
)}이런식으로 각 조건에 해당하는 jsx를 컴포넌트로 분리해서 표현하면 깔끔할 것 같아용 (ai가 짜준건데 사실 이것 좀..)
There was a problem hiding this comment.
오~ 이런 방식으로 공용컴포넌트화 해서 나눠보겠습니다 의견 감사해요!
There was a problem hiding this comment.
분리하여 좀 더 리팩토링해보기 👍
| const [internalIsOpen, setInternalIsOpen] = useState(false); | ||
|
|
||
| const isOpen = | ||
| controlledIsOpen !== undefined ? controlledIsOpen : internalIsOpen; | ||
|
|
||
| const handleToggle = useCallback(() => { | ||
| if (onToggle) { | ||
| onToggle(); | ||
| } else { | ||
| setInternalIsOpen((prev) => !prev); | ||
| } | ||
| }, [onToggle]); | ||
|
|
||
| const handleOverlayClick = useCallback( | ||
| (e: MouseEvent) => { | ||
| if (e.target === e.currentTarget) { | ||
| handleToggle(); | ||
| } | ||
| }, | ||
| [handleToggle] | ||
| ); | ||
|
|
||
| const handleKeyDown = useCallback( | ||
| (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| handleToggle(); | ||
| } | ||
| }, | ||
| [handleToggle] | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (isOpen) { | ||
| document.body.style.overflow = "hidden"; | ||
| } else { | ||
| document.body.style.overflow = "unset"; | ||
| } | ||
|
|
||
| return () => { | ||
| document.body.style.overflow = "unset"; | ||
| }; | ||
| }, [isOpen]); |
| if (item.quantity > STOCK_THRESHOLDS.OUT_OF_STOCK) { | ||
| return `${item.name}: 재고 부족 (${item.quantity}개 남음)`; | ||
| } else { | ||
| return `${item.name}: 품절`; | ||
| } |
There was a problem hiding this comment.
얼리 리턴이 조금 더 가독성에 좋을 것 같아요 ㅎㅎ
// 방법 1: early return
if (item.quantity > STOCK_THRESHOLDS.OUT_OF_STOCK) {
return `${item.name}: 재고 부족 (${item.quantity}개 남음)`;
}
return `${item.name}: 품절`;
// 방법 2: 조건을 변수로 분리
const hasStock = item.quantity > STOCK_THRESHOLDS.OUT_OF_STOCK;
if (hasStock) {
return `${item.name}: 재고 부족 (${item.quantity}개 남음)`;
}
return `${item.name}: 품절`;아니면 여기서는 삼항 연산자가 조금 더 가독성이 좋을 것 같기도 하네용?
const stockMessage = item.quantity > STOCK_THRESHOLDS.OUT_OF_STOCK
? `${item.name}: 재고 부족 (${item.quantity}개 남음)`
: `${item.name}: 품절`;There was a problem hiding this comment.
역시 대장! 삼항연상자 괜찮네요 깔끔하게 변경 완료 🫡
| const totalStock = useMemo( | ||
| () => products.reduce((total, product) => total + product.quantity, 0), | ||
| [products] | ||
| ); | ||
|
|
||
| const isLowStock = useMemo( | ||
| () => totalStock < stockWarningThreshold, | ||
| [totalStock, stockWarningThreshold] | ||
| ); |
There was a problem hiding this comment.
이 단순한 계산들에 useMemo가 꼭 필요했을까? 라는 생각이 들어용ㅎㅎ
| const handleProductSelect = useCallback( | ||
| (e: ChangeEvent<HTMLSelectElement>) => { | ||
| onProductSelect(e.target.value); | ||
| }, | ||
| [onProductSelect] | ||
| ); |
There was a problem hiding this comment.
handleProductSelect 안에 on함수가 적절한건가?! 싶은 생각이 있습니다.
준일님의 글을 추천해요!!!! 여기에 핸들러 네이밍에 관한 언급을 읽어보시면 도움이 될 것 같습니다!
|
가은님 이번주 고생많으셨어요..! |
4 participants
과제 체크포인트
배포 url
https://tooth-is-silver.github.io/front_6th_chapter2-1/
기본과제
심화과제
과제 셀프회고
과제를 하면서 내가 제일 신경 쓴 부분은 무엇인가요?
과제를 다시 해보면 더 잘 할 수 있었겠다 아쉬운 점이 있다면 무엇인가요?
리팩토링은 너무 큰 단위 말고 내가 잘 아는 작은 단위부터 시작하는게 제일 좋다는 것을 경험했다. basic 과제 진행시 바닐라 자바스크립트의 경우에 리액트로 치면 컴포넌트와 state간의 정확한 분리가 어려워 UI를 어디까지 보고 핸들러를 분리해야할지 고민이 많았다. 기존 origin 파일로 실행된 사이트의 UI를 보면서 도메인을 이해하고 분리하려고 노력했다. 예를 들어 장바구니에 담기는 이벤트나 수량이 증가,감소하는 이벤트, 장바구니에서 상품이 제거되는 이벤트, 특정 수량 이상일 경우 출력되는 텍스트들과 할인율... 마치 UI/UX 기획서를 뜯어보는 듯한 느낌이었다.
그래서 다음에 진행한다면 작은 단위에서 진행하되 비즈니스 로직(도메인)에 따라서 진행해보고 싶다. 도메인에 맞춰서 하나씩 나누다보면 저절로 덩어리로 뭉쳐지고 자연스럽게 분리될 것으로 예상한다. 현재는 일단 핸들러와 ui를 분리하고, 그 이후에 도메인에 맞춰서 개선을 진행했는데 도메인에 맞춰서 덩어리화 한 다음에 도메인 별로 점진적으로 작은 단위부터 개선했다면 좀 더 빠르게 작업했을 것 같다.
이렇게 느낀 이유는, 상수, 핸들러, UI 등 해당 프로세스대로 분리하니 도메인 로직들이 정확히 어디에 어느 부분에서 동작하는지 파악하기가 어려웠다. 너무 더럽고 많은 코드에서 이 코드가 포인트에서 사용되는 코드가 맞는가? 라고 판단했을때 확실치 않았다.
리뷰 받고 싶은 내용이나 궁금한 것에 대한 질문 편하게 남겨주세요 :)
멘토링 때 답변 못받고 넘어간 부분인데, 코치님이 리팩토링을 진행하신다면 어떤 절차대로 진행하실지 궁금합니다!
AI를 적극적으로 활용하는건 너무 좋았는데, 열심히 쓰다보니 이게 맞나..? 해당 주차에서 내가 얻어가는게 뭐가 있을까, AI활용성 고민과 AI로의 문제해결 밖에 안남은 것 같은데 만약 복기하게 된다면 어떻게 복기하면 좋을지 고민입니다.