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팀 송창엽] [Chapter 2-1] 클린코드와 리팩토링 #29

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

Conversation

Songchangyeop
Copy link

@Songchangyeop Songchangyeop commented Jan 9, 2025

과제 체크포인트

기본과제

  • 코드가 Prettier를 통해 일관된 포맷팅이 적용되어 있는가?
  • 적절한 줄바꿈과 주석을 사용하여 코드의 논리적 단위를 명확히 구분했는가?
  • 변수명과 함수명이 그 역할을 명확히 나타내며, 일관된 네이밍 규칙을 따르는가?
  • 매직 넘버와 문자열을 의미 있는 상수로 추출했는가?
  • 중복 코드를 제거하고 재사용 가능한 형태로 리팩토링했는가?
  • 함수가 단일 책임 원칙을 따르며, 한 가지 작업만 수행하는가?
  • 조건문과 반복문이 간결하고 명확한가? 복잡한 조건을 함수로 추출했는가?
  • 코드의 배치가 의존성과 실행 흐름에 따라 논리적으로 구성되어 있는가?
  • 연관된 코드를 의미 있는 함수나 모듈로 그룹화했는가?
  • ES6+ 문법을 활용하여 코드를 더 간결하고 명확하게 작성했는가?
  • 전역 상태와 부수 효과(side effects)를 최소화했는가?
  • 에러 처리와 예외 상황을 명확히 고려하고 처리했는가?
  • 코드 자체가 자기 문서화되어 있어, 주석 없이도 의도를 파악할 수 있는가?
  • 비즈니스 로직과 UI 로직이 적절히 분리되어 있는가?
  • 코드의 각 부분이 테스트 가능하도록 구조화되어 있는가?
  • 성능 개선을 위해 불필요한 연산이나 렌더링을 제거했는가?
  • 새로운 기능 추가나 변경이 기존 코드에 미치는 영향을 최소화했는가?
  • 리팩토링 시 기존 기능을 그대로 유지하면서 점진적으로 개선했는가?
  • 코드 리뷰를 통해 다른 개발자들의 피드백을 반영하고 개선했는가?

심화과제

  • 변경한 구조와 코드가 기존의 코드보다 가독성이 높고 이해하기 쉬운가?
  • 변경한 구조와 코드가 기존의 코드보다 기능을 수정하거나 확장하기에 용이한가?
  • 변경한 구조와 코드가 기존의 코드보다 테스트를 하기에 더 용이한가?
  • 변경한 구조와 코드가 기존의 모든 기능은 그대로 유지했는가?
  • 변경한 구조와 코드를 새로운 한번에 새로만들지 않고 점진적으로 개선했는가?

과제 셀프회고

무수히 많은 고민과 크나큰 좌절을 겪었던 한 주 였습니다
아쉬움이 너무나도 많습니다..
과제 체크포인트에 양심이 찔리는 것이 많아 체크를 못 하겠습니다 😂

잘한점

  • 자기 전까지 고민을 계속 했던 것
  • 진행한 과제중 역대급으로 많은 커밋을 남긴 것..

아쉬운점

  • 그럼에도 불구하고 베이직 과제를 구현하지 못 한점..
  • 아직도 난 클린코드의 정확한 이해를 못 하고 있다는 생각이 든다는점..

과제를 진행한 과정

  1. 코드 파악을 위한 주석 처리
  2. 관심사 별 컴포넌트 로직 분리
  3. 각 컴포넌트의 상태와 액션을 처리하기 위한 store 생성
  4. 뒤늦게 내가 짠 코드가 테스트 코드에서 DOM의 업데이트를 기다려주지 않는 다는 사실 발견..
  5. 하루밖에 남지 않았기 때문에 basic 포기.. 🥹
  6. 심화 과제 수행

과제를 진행하다 테스트 코드가 DOM의 업데이트를 기다려 주지 않는 다는 사실을 알았습니다..
테스트코드를 수정하여 setTimeout이나 waitFor을 사용하면 통과가 된다는 사실을 알았지만 그렇게 될 경우 original.js 의 테스트가 실패 하는 것을 확인하고.. 내가 익숙한 환경에서 클린코드라도 적용해보자 생각이 들어 basic을 포기하고 심화과제로 방향을 틀었습니다 T_T
다행인건.. basic에서 관심사별 분리를 해 둔 덕분에 심화과제는 빠르게 진행이 가능했습니다

과제에서 좋았던 부분

클린코드를 영상이나 글을 보며 어떤 게 클린코드다 라는 것만 인지하고 실제로 회사에서는 바쁜다는 이유로 조금 소홀히 했었는데
과제에서 정말 많은 고민을 해 본것 같습니다

과제를 하면서 새롭게 알게된 점

클린코드가 무엇인지 사실 아직 정확하게 파악은 못 했지만
안 좋은 코드가 무엇인지는 확실하게 이해했습니다..

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

  • 좋은 폴더 구조
  • 레거시 코드를 리팩토링 할 때 순서
  • 로직과 UI를 분리해야 하는 이유와 어디까지 분리를 해야하는지에 대한 기준
    • 질문에 남겨놓겠습니다.

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

로직과 UI를 분리해야 하는 이유와 어디까지 분리를 해야하는지에 대한 기준을 잘 모르겠습니다.

저는 심화과제에서 App.tsx에서 컨텍스트를 관리하는 Provider로 하위 컴포넌트를 감싸고 해당 컨텍스트 내부에는 상태와, 해당 상태를 변경하는 액션함수들을 모아 두었습니다. 그리고 컴포넌트에서 핸들러 함수를 두어 이벤트가 발생하면 컨텍스트 내부의 상태를 변경하는 액션함수를 호출하도록 처리했어요.

그런데 다른분들 코드를 살펴보니 최상위에서 모든 상태와 액션함수, 그리고 해당 액션 함수를 호출하는 핸들러까지 모두 커스텀 훅에서 관리를 하고 각 하위 컴포넌트에서는 핸들러와 상태만 props로 전달받아 사용하고 있는 구조가 많더라구요.

그래서 생각이 든게 "아 UI와 로직을 분리하라는게 상태와 핸들러를 포함한 모든 것들을 UI에서 분리를 하는건가?" 였습니다
그런데 이렇게 모든 로직을 분리하는 이유와 이점을 잘 모르겠습니다

테스트 하기 좋은 구조이기 때문인가 생각이 들었는데
제가 테스트 코드를 안 다뤄봐서 테스트코드를 작성하기에 좋은가에 대해서는 잘 모르겠네요..
각 UI 컴포넌트만 따로 가져와서 재사용 하기에 좋겠다는 생각이 들긴 했습니다
제가 작성한 컴포넌트들은 각각 상태와 핸들러등 로직을 가지고 있어 재사용 측면에서는 좋지 않을 수 있겠더라구요..

질문

질문 1. 보통 이렇게 하위의 모든 데이터를 제어하는 부모 컴포넌트를 제외한 모든 컴포넌트들의 UI와 로직을 전부 분리하는게 좋은 구조인건가요? 맞다면 이게 항상 좋은 구조일까요?

질문 2. 로직을 UI를 관리하는 부모에서 모두 관리하는 것이 유일한 방법일까요? 만약 컴포넌트의 뎁스가 깊어진다면 props drilling이 심해질 것 같습니다. 그런데 그 로직들을 각 컴포넌트에서 커스텀 훅으로 분리 후 사용하자니 재사용 측면에서는 아쉬울 것 같아서 좋은 구조가 무엇인지 헷갈립니다.

질문 3. 모든 컴포넌트들을 만들 때 재사용의 가능성이 있으니 항상 확장성 있게 만들어야 할까요?

Copy link

@wonyoung2257 wonyoung2257 left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다~!
옵져버 패턴을 사용한 데이터 관리가 매우 인상적인것 같아요
저도 코드를 보면서 옵져버 패턴으로 구현을 한번 해봐야겠어요 ㅎㅎ

Choose a reason for hiding this comment

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

오 리액트를 보는 줄 알았습니다

Comment on lines +28 to +34
export const useAddCartItem = () => useCreateCartContext(cartContext, 'useAddCartItem', 'cartProvider').addCartItem;

export const useRemoveCartItem = () =>
useCreateCartContext(cartContext, 'useRemoveCartItem', 'cartProvider').removeCartItem;

export const useClearCartItem = () =>
useCreateCartContext(cartContext, 'useClearCartItem', 'cartProvider').clearCartItem;

Choose a reason for hiding this comment

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

이런 동작은 하는 함수들을 action으로 묶어서 내보내는 건 어떻게 생각하세요?
다른 코드에서 하나씩 다 호출해서 사용하다보니 라인이 길어지져서 가독성이 조금 떨어지는 것 같더라구요
하나로 객체로 묶어서 객체분해로 사용하면 라인 수를 줄여서 더 읽기 쉬울 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

아 맞아요 사실 action 묶고 사용하는 곳에서 해당 액션 내부의 액션함수롤 가져와 사용하도록 하는게 더 간편했을 것 같네요
좋은 의견 감사합니다 👍

Comment on lines +12 to +14
const [totalPrice, setTotalPrice] = useState(0);
const [point, setPoint] = useState(0);
const [totalDiscountRate, setTotalDiscountRate] = useState(0);

Choose a reason for hiding this comment

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

포인트, 할인율, 총 가격을 state로 관리하셨군요
저는 이 친구들이 cart에 담긴 product로 계산할 수 있는 값이라 생각해서 useState를 사용하지 않아도 될 것 같다는 생각이 드네요
혹시 창엽님은 어떻게 생각하시는지 궁금하네요!

Copy link

@hdlee0619 hdlee0619 left a comment

Choose a reason for hiding this comment

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

이번 한주도 고생하셨습니다..!
커밋 수를 보고 놀랐습니다..ㅎㅎ 세세하게 나누셨구나 생각이 듭니다..!
제가 아직 많이 부족함을 느끼네용.. 좀 더 꼼꼼하게 했어야하는데,,ㅠ

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