[클린코드 리액트 2기 양세영] 페이먼츠 미션 Step 1 #55
Conversation
There was a problem hiding this comment.
안녕하세요 세영님! 리뷰어 정현수입니다 전반적으로 잘 구현해주셨어요!
질문 남겨주신 1번에서 5번까지의 질문중에 1번 2번 3번은 제가 리뷰에 조금 달아놓은 것 같아요!
전반적으로는 괜찮은데 context에 대한 사용법만 조금 더 익히시면 완벽한 코드가 될 것 같네요!
가독성 좋은 함수의 네이밍이 너무 어렵습니다. 리뷰어님의 함수 이름 지을 때 팁 같은게 있을까요?!
4번에 대한 답도 사실 리뷰에 달려져 있긴한데 조금 더 설명을 해볼게요.
가독성 좋은 코드라는 것은 말 그대로 "읽기 좋은 코드" 인데요 사실 개발자 신에서 사람들이 글을 잘 쓰는 사람은 개발도 잘 한다 라는 말이 있어요. 이건 왜 그럴까요? 글을 잘 쓴다는 것은 상대방의 처지, 수준을 이해하고 상대방이 이해하기 쉽게 그리고 상대방의 입장에서 잘 생각해서 글을 썼기 때문에 해당 글을 읽는 사람들은 술술 잘 읽히게 되는 것 이거든요. 저는 이것과 같은 맥락이라고 생각해요. 세영님이 정말 이 코드에 대한 맥락이 없는 사람이라고 생각하면 돼요. 사실 세영님도 이 프로젝트를 하시면서 많은 생각들을 하셨을텐데 그런 생각들이 모든 코드에 녹아져있지 않아요. 제 코드도 물론이구요. 사실 그렇게 되면 최대한 변수명과 함수명과 컴포넌트명과 같은 이름에 집중을 해서 잘 지어줘야 해요. 잘 짓는 것은 제가 앞서 얘기했던 것 처럼 "상대방의 입장"이 되어보시면 됩니다. onChangeInput 이라는 함수를 만들었다면 "Input이 변할 때.." 라는 뜻의 이름이 되는데 이 함수는 when만 설명하고 what을 전혀 설명하고 있지 않아요. "그래서 뭘 하는데?" 라고 독자는 생각할 수 있겠죠. 그럼 what을 이름에 녹여주시면 돼요!
다른 코드들도 다 같은 맥락인데요, 어떤 유틸함수가 있으면 해당 유틸함수가 무엇을 하는지 집중하시면 돼요. 그리고 그렇게 이름을 짓다보면 해당 함수가 하는 일이 너무 많을 땐 이름 짓기가 어려울텐데 그럴 땐 또 함수를 나누면 되는거에요.
재사용가능한 컴포넌트로 분리하기위해 고민하면서 미션을 진행했는데.. 하고보니 단순하게 UI만 분리됐을 뿐 효과적 재사용할 수 있도록 분리하지 못했다는 생각이 듭니다. 특히 input에는 연결되는 상태와 로직이 많은데 이렇게 분리하는게 맞는가?란 의구심이 듭니다.
지금 저는 크게 나쁘지는 않은 것 같아요. 각 Input들이 특성이 다르고 모양도 조금씩 다르고 로직들도 조금씩 달라서 나눠진 것 자체는 크게 나쁘지 않은 것 같지만 뭔가 합성 컴포넌트를 이용한다면 코드를 줄일 수 있을 것 같은 느낌이 드네요! 카카오 블로그에서 작성한 합성 컴포넌트에 대한 글을 참고해보시면 어떤 느낌인지 대충 느낌이 오실거에요. 사실 현재 지금 만들어진 컴포넌트들이 대부분 비슷한 형식이거든요. "Container로 감싸고, Title이 있고, common에서 가져온 Input이 있다" 정도인 것 같네요. 반복되는 것은 언제나 줄일 수 있답니다. 제가 리뷰에서 설명드린 것을 참고해보시고, 카카오 글도 한 번 참고해보시고 생각해보셨으면 좋겠어요! 만약 정말 답이 안나온다면 저와 같이 얘기해보시죠! 이게 꼭 답이 아닐수도 있답니다. 세영님이 생각해보시고 아니다 싶으시면 굳이 적용시키지 않으셔도 돼요. 하지만 저는 지금 적당히 나뉘어져 있는 것 같기도하고, 프로젝트 특성상 Input이 많고 컴포넌트가 많아지는 건 어쩔 수 없다고 생각하고 있긴하지만 조금 더 발전가능성이 있을 것 같긴해요.
제가 조금 어렵게 설명드린 것 같아서 죄송하네요.. 세영님이 보시고 너무 어렵다 싶으시면 그냥 저한테 다시 말씀해주세요ㅎㅎ 제가 조금 더 쉽게 설명 드릴 수 있다면 설명드리겠습니다! 또 계속 질문주세요!
.eslintrc.json
Outdated
| { | ||
| "env": { | ||
| "browser": true, | ||
| "es2021": true | ||
| }, | ||
| "extends": [ | ||
| "eslint:recommended", | ||
| "plugin:react/recommended", | ||
| "plugin:@typescript-eslint/recommended", | ||
| "plugin:prettier/recommended" | ||
|
|
||
| ], | ||
| "overrides": [ | ||
| ], | ||
| "parser": "@typescript-eslint/parser", | ||
| "parserOptions": { | ||
| "ecmaVersion": "latest", | ||
| "sourceType": "module" | ||
| }, | ||
| "plugins": [ | ||
| "react", | ||
| "@typescript-eslint" | ||
|
|
||
| ], | ||
| "rules": { | ||
| "prettier/prettier": ["error", { "endOfLine": "auto" }] | ||
| } | ||
| } |
There was a problem hiding this comment.
https://www.npmjs.com/package/eslint-plugin-json-format
eslint-plugin-json-format eslint 플러그인을 사용하면 json 파일도 포맷팅 처리해줄 수 있어요!
There was a problem hiding this comment.
앗 json 파일들을 제가 신경을 못쓰고 있었네요! eslint에 추가해서 json도 포맷팅했습니다 감사합니다!
.storybook/main.js
Outdated
| module.exports = { | ||
| "stories": [ | ||
| "../src/**/*.stories.mdx", | ||
| "../src/**/*.stories.@(js|jsx|ts|tsx)", | ||
| ], | ||
| "addons": [ | ||
| "@storybook/addon-links", | ||
| "@storybook/addon-essentials", | ||
| "@storybook/addon-interactions", | ||
| "@storybook/preset-create-react-app" | ||
| ], | ||
| "framework": "@storybook/react", | ||
| "core": { | ||
| "builder": "@storybook/builder-webpack5" | ||
| } | ||
| } |
There was a problem hiding this comment.
전체적으로 no newline at end of line 마크가 많은 것 같아요.
prettier를 프로젝트 전체로 한 번 돌리면 해당 마크가 없어질거에요!
여러 버전의 유닉스 OS간의 공통된 API 및 인터페이스를 정의해놓은 POSIX 명세 중 text file 부분을 보면 유닉스 시스템은 text file들을 서로 구분할때 EOL을 사용하기 때문에 파일을 만들때마다 파일의 끝에 EOL을 넣어주는 것이 안전하다.
라고 하네요! 사실 저도 잘은 모르지만 깃헙에서 표시해주고 있는 이유도 위와 같은 이유일 것 같네요.
package.json에서 lint script를 만들어서 프로젝트 전체 prettier, eslint를 적용시키는 스크립트를 생성해놔도 괜찮을 것 같네요~
There was a problem hiding this comment.
EOL 개념에 대해서 모르고 있었는데 파일 구분시 사용되는거라 끝에 넣어주는게 안전한거였군요!
그런데 전체 eslint를 돌렸는데 .storybook 파일까지 수정이 안되는 이슈가 있어서 직접 경로를 지정해서 포맷팅시켜줬는데.. script에 추가로 더 설정을 해줘야되는 부분일까요!?
"lint": "eslint .",
package.json
Outdated
| "@testing-library/jest-dom": "^5.16.5", | ||
| "@testing-library/react": "^13.4.0", | ||
| "@testing-library/user-event": "^13.5.0", | ||
| "@types/jest": "^27.5.2", | ||
| "@types/node": "^16.18.12", | ||
| "@types/react": "^18.0.28", | ||
| "@types/react-dom": "^18.0.11", | ||
| "eslint-config-prettier": "^8.6.0", | ||
| "prettier": "^2.8.4", |
There was a problem hiding this comment.
현재 dependencies와 devDependencies에 들어가야 하는 라이브러리들이 혼재되어 있는 것 같아요!
type과 관련된 패키지 또는 테스팅과 관련된 패키지들은 실제 유저에게 배포될 번들에는 굳이 포함되지 않아도 될 것 같네요!
There was a problem hiding this comment.
넵 모두 devDependencies 로 옮겼습니당!
src/App.tsx
Outdated
| function App() { | ||
| return ( | ||
| <BrowserRouter> | ||
| <div className="App"> |
There was a problem hiding this comment.
이 div는 꼭 필요한 tag 일까용?
요기서 처리해주기 보다는 Layout 컴포넌트에서 스타일 처리를 해줘도 괜찮겠다는 생각이네용
관리포인트가 너무 다양한 곳에 흩어져 있는 것 같아서요!
세영님이 생각하시기에 굳이? 라는 생각이 들면 하지 않으셔도 괜찮습니다~
There was a problem hiding this comment.
파일명이 App.tsx이고 <div className="App"> 태그로 감싸고 있어서 이 존재에 대해 당연히 있어야할 태그라고 생각했던거같아요ㅎㅎㅎ 근데 굳이 필요없는 태그네요
src/utils/form.ts
Outdated
| export const handleDigit = (digit: string, targetName: string) => { | ||
| const value = digit.replace(/\D+/g, ''); | ||
| const length = value.length; | ||
| const nextId = Number(targetName.split('digit')[1]) + 1; |
There was a problem hiding this comment.
이런 복잡한 코드는 사실 주석을 달아주면 괜찮은 경우도 있어요.
코드로 최대한 해결하면 좋지만 어쩔 수 없이 복잡한 연산이 들어가는 코드들은 주석으로 설명을 해주면 협업할 때 큰 도움이 된답니다!
src/utils/form.ts
Outdated
| return value; | ||
| }; | ||
|
|
||
| export const handleExpire = (digit: string) => { |
There was a problem hiding this comment.
handleExpire의 이름이 적당한지 생각해보시면 좋겠어요!
해당 함수에서 지금 하는 역할이 좀 많은 것 같은데 이것도 유틸함수들로 최대한 잘게 잘게 나눌 수 있을 것 같아요!
src/utils/form.ts
Outdated
| @@ -0,0 +1,47 @@ | |||
| const SLASH = '/'; | |||
|
|
|||
| export const handleDigit = (digit: string, targetName: string) => { | |||
There was a problem hiding this comment.
input으로 들어오는 문자를 확인해서 빈값으로 처리하는 함수인데 적절한 이름이 아닌거같아요ㅠ 유틸에 있는 함수이름 모두 수정했습니다!
src/utils/form.ts
Outdated
| if (length < 3) result = value; | ||
| else if (length < 5) { |
There was a problem hiding this comment.
3과 5는 제가 무슨 뜻인지 모르겠어요! 매직 스트링을 최대한 줄이면 좋을 것 같아요!
src/utils/form.ts
Outdated
| export const handlePassword = (digit: string, targetName: string) => { | ||
| const value = digit.replace(/\D+/g, ''); | ||
| const length = value.length; | ||
| const nextId = Number(targetName.split('ps')[1]) + 1; |
There was a problem hiding this comment.
ps와 같은 스트링은 UI에서도 쓰이는 것 같은데 이런 경우에는 상수로 빼서 UI에서도 상수를 바라보고 해당 유틸함수에서도 상수를 바라보는 형식이 제일 좋아요! 변경사항에 유연하게 대처할 수 있고, 상수로 의미를 한 번에 알아볼 수 있어요!
There was a problem hiding this comment.
헉 이거 "password"로 바꾼다고 돌아다니면서 수정했는데 코멘트 주신거처럼 상수로 빼버리면 훨씬 간단하게 처리가 가능한 부분이었네요!
|
현수님 리뷰 꼼꼼하게 읽으면서 다시 생각해보고 공부했는데 정말 도움이 많이 되었습니다 감사합니다! 코드를 다른 사람들이 읽기 쉬운 글을 쓰듯 작성하면 좋고 또 이해하기 어려운 코드들이 무엇인지도 함께 알려주셔서 방향을 제대로 잡을 수 있었어요! 질문드리고 싶은 내용위 댓글로 질문을 드렸는데 댓글들이 많아서 찾기 어려우실거같아 보기 쉽게 여기 다시 정리해서 적어요!
|
There was a problem hiding this comment.
세영님 잘 반영해주셨어요~ 고생하셨네요!
이번 스텝 1 미션은 우선 머지하고 스텝 2로 넘어가면 될 것 같아요!
제가 이번에 말씀드릴건 간단해요!
코드 리뷰 (셀프 리뷰) 남기기 할 때 single comment로 남기시는 것 보다 자신의 코드를 코드 리뷰하는 것 처럼 깃허브에서 셀프 리뷰를 남길 수 있어요! 요기 블로그를 참고 하셔서 셀프리뷰를 남기시면 좋겠어요!
이유는 싱글 코멘트를 남기게 되면 해당 풀리퀘스트에 관계된 모든 사람들이 이메일이나 알림을 해놨다면 싱글 코멘트를 하나를 남길 때 마다 알림이 가서 조금 노이지 할 수 있어요ㅎ 그래서 보통은 협업하거나 할 때 start review로 셀프리뷰를 시작하시고 (다른 사람 코드 리뷰 할 때도 똑같아요) 그리고 하나씩 코멘트를 달고 한꺼번에 Finish your review로 끝내면 알림이 딱 한 번만 가거든요. 그래서 컴팩트하게 리뷰를 할 수 있답니다!
CardContext.tsx 파일에 context api와 onChange 함수들을 합치기 위해 useReducer을 활용해서 다시 리팩토링했는데 현수님께서 의견주신 방향과 일치하는 코드가 맞을까요!?(CardContext.tsx)
넵. 잘 작성하셨어요. 근데 기억해야 할 것이 하나 있어요.
저도 벨로퍼트님의 문서를 보고 context api + reducer를 접했는데요 이렇게만 사용을 하다보면 헷갈리는 부분이 뭐냐면 context 와 reducer를 하나로 생각한다. 인데요. 이거는 좀 위험한 생각이에요.
지금 작성해주신 CardContext.tsx 에는 context의 개념도 쓰였고, reducer의 개념도 쓰였어요.
다시 말씀드리면 context만 써도 되고, reducer만 써도 되고, 둘 다 같이 써도 돼요! 근데 세영님은 둘 다 같이 쓰기로 선택을 하신거죠! 그래서 둘의 차이점을 잘 알고 사용해야 나중에 가서 적재적소에 그리고 따로 사용할 때 헷갈리지 않아요. (제가 겪은거라서 정말 잘 알고있거든요 ㅎㅎ.. 어느부분에서 헷갈리는지)
context는 해석하면 "문맥", "흐름" 뭐 대충 이런 느낌인데요 이걸 리액트적으로 해석을 해보면 "어떤 영역에 문맥을 제공하는 친구" 라고 생각할 수 있는데요. 그래서 context.provider는 "어떤 영역에 문맥을 제공하는 친구를 제공해주는 친구" 라고 생각할 수 있어요. (어렵나요?.. 하하)
그래서 핵심은 "단순 전역 상태를 관리하는 친구" 라기보다는 우리만의 문맥안에 흐르는 상태들을 정의하고 어떤 공간에서 그 상태들이 어디서든 접근할 수 있게끔 하자와 같은 느낌으로 기억해주시면 될 것 같아요.
그리고 reducer(사용해주신 useReducer는 reducer를 사용하는 훅이죠)의 핵심개념은 뭘까요?
reducer는 state와 action을 변수로 받는데요, 짧게 요약하자면 "어떤 행동(액션)을 받으면 행동에 해당하는 로직을 실행시켜 상태값을 바꾼다" 에요. 제가 리뷰때도 말씀드렸지만 저게 어떤 장점이 있을까요? 다른 사람이 코드를 볼 때 조금 더 직관적으로 코드를, 그리고 흐름을 이해할 수 있어요.
만약 상태가 여러 개인데 그걸 전부 useState로 선언을 하고 어떤 함수안에서 상태가 3-4개 변한다고 가정을 해볼게요. 그런 그 어떤 함수를 작성할 때 useState로 작성을 했다면 그 함수를 다른 사람이 봤을 때 이해하기가 되게 어려울거에요.
const Something = () => {
setA("");
setB("");
// 어떤 로직 수행..
setC("");
// 어떤 로직 수행..
}그럼 다른 사람이 해당 함수를 해석할 때 시간이 많이 걸리지만 useReducer를 사용한다면
const Something = () => {
dispatch({ action: "SOMETHING" });
};으로 간단하게 추상화를 시키고, 그 안에 상태들이 어떻게 변하는지에 대한 것은 reducer에 전적으로 다 맡기는거에요. 그럼 관심사를 분리를 시킬 수 있겠죠. 상태가 변하는 것에 대한 로직은 전부 reducer에 맡기고, 외부에서 사용할 때는 추상화된 언어(액션)(으)로 소통한다. 이렇게 되겠죠!
그래서 제가 말씀드리고 싶은 핵심은 두 개는 항상 같이 쓰는 것이 아니라 적재적소에 다르게 쓸 수 있다. 예요!
card 상태값들 중 분명 number여야 할 type들(digit, cvc, passwords)을 string으로 지정을 했는데요! 초깃값을 '' 빈값으로 처리하기 위해 string으로 선택했던거였고 헷갈리는 요소가 될 수 있기 때문에 number로 모두 바꾸고 싶습니다. 그런데 초깃값때문에 어쩔 수 없이 아래와 같이 리팩토링을 했는데 string이 추가로 존재하는게 어색해서요ㅠㅠ 혹시 더 좋은 타입이 있을까요!?(CardContext.tsx)
사실 근데 string으로 있는게 어색하지 않은 것 일 수도 있어요. number는 count와 같은 개수를 셀 때 쓰면 정말 유용하게 잘 셀 수도 있겠죠. 근데 지금 cvc와 같은 변수는 사실 "string인데, number가 여러 개 연속된 string" 일수도 있어요. 그래서 초기값을 설정하기가 되게 애매한거죠. number로 지정을 하면 초기값을 0으로 해야하는지 아니면 -1로 해야하는지 기준도 애매하구요. 그래서 만약 number로 정말 하고 싶으시다면 그냥 number | undefined 로 타입을 지정하고 undefined로 되어있으면 예외처리를 해주는식으로 "명시적"으로 타입 지정을 해줄 수도 있어요.
이건 사실 어떻게 하든 세영님의 선택이에요. 정답은 없거든요!
생각 잘 해보시고 하시면 될 부분같아용~
우선 Approve 하고, 다음 미션에서 제가 말씀드린 것 적용해보시고 잘 생각해보세요~
| passwords: { password1: string; password2: string }; | ||
| }; | ||
|
|
||
| type SampleDispatch = Dispatch<Action>; |
There was a problem hiding this comment.
SampleDispatch 으로 되어있는 것 보니까 벨로퍼트님 글을 참고하셨군요ㅋㅋㅋㅋㅋ
안녕하세요 리뷰어님! 이번 미션도 잘 부탁드립니다!
구현 링크
스토리 링크
요구사항
궁금한 부분
카드추가, 카드추가완료에서 카드 UI의 데이터를 유지시키기 위해서 context api를 이용했는데요. 그리고 같은 context provider 안에서 complete라는 상태를 통해 카드추가에서 완료상태로 넘어가도록 구현했습니다. 처음엔 라우터로 페이지를 분리해야하나 고민했는데 이렇게 구현하는게 안정적인 방법인지 궁금합니다!(PaymentCardRegister.tsx)
카드의 form 데이터를 쭉 나열해서 세팅했는데 좀 더 깔끔하게 정리할 수 있는 방법이 있을까요?(PaymentCardRegister.tsx, :6)
onChange 핸들러들의 중복된 코드들이 많은데 이것들을 깔끔하게 정리하고 싶습니다. useReducer를 통해 묶어보려고 했는데 익숙하지 않아서 코드를 어떻게 해야할지 감이 안 오네요ㅠㅠ 어떤 방법으로 코드를 정리하면 좋을까요?(CardForm.tsx)
가독성 좋은 함수의 네이밍이 너무 어렵습니다. 리뷰어님의 함수 이름 지을 때 팁 같은게 있을까요?!
재사용가능한 컴포넌트로 분리하기위해 고민하면서 미션을 진행했는데.. 하고보니 단순하게 UI만 분리됐을 뿐 효과적 재사용할 수 있도록 분리하지 못했다는 생각이 듭니다. 특히 input에는 연결되는 상태와 로직이 많은데 이렇게 분리하는게 맞는가?란 의구심이 듭니다.