프론트엔드 코드 리뷰 #22
Open
프론트엔드 코드 리뷰 #22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pocojang
commented
Aug 31, 2021
There was a problem hiding this comment.
안녕하세요
리뷰어 장현석입니다.
전반적인 코드 구조보다는 코트 스타일이나 가독성 그리고 염려되는 부분 위주로 리뷰를 남겼으니 참고하셔서 수정 부탁드립니다.
- airbnb 사의
react-dates를 사용하시며 어쩔 수 없이moment를 사용하셨늗네 굳이 불편하게 데이트 포맷을 일일이 수정하는 유틸함수를 만드시는 리소스를 들일 필요가 있었을까 생각이 들었습니다. - 또한 moment에 대한 이런 저런 이슈가 많은데요. 이런 경우에는 같은 라이브러리로 비슷한 고민을 하는 개발자들을 찾아보시면 좋습니다 => Switch from moment to date-fns? react-dates/react-dates#208 의 이슈를 보면 다른 개발자들은 어떻게 해결했는지 그리고 에어비앤비 개발팀에서 어떤 방향으로 움직이고 있는지 추이를 보고 수정하시면 도움이 되실겁니다.
- 배열에 접근할때 [0...1...2] 등 암시적으로 접근하는 코드가 상당 부분 보입니다. 언젠가는 서버에서 주는 데이터가 이중 배열로 올 수도 있을텐데 최대한 명시적으로 수정하거나 lodash pick 과 같은 유틸함수를 만드셔서 코드를 더 장기적인 관점으로 변경해보셨으면 좋겠네요
- 리액트 컴포넌트 내부에서 동작되는 코드와 외부에서 동작되는 코드에 대한 고민이 필요해보입니다. 리액트 컴포넌트의 주기와 함께 움직여야하는 로직이 관리되지 않고 방치되는 경우가 있습니다.
EOL이 지켜지지 않고 있어요. 로컬에서는 정상인데 깃으로 올라가면서 오류가 있을 수 도 있긴한데요. 한번 확인 부탁드립니다.- 리액트에 이벤트로 바인딩하고 있는 컴포넌트 내부에 선언되고 있는 함수들을 굳이
Closure로 감싸고 있는데요. 성급한 판단에서 온 습관은 아닌지 어떠한 방향을 가지고 의도적으로 사용하고 있는지 점검하실 필요가 있습니다.
추가로 몇가지 의문점은 코멘트 남겼으니 반대로 저에게 피드백 부탁드리며 1차 리뷰를 마치도록 하겠습니다.
|
|
||
| // Set GraphQL Apollo server | ||
| const context = ({ req }) => { | ||
| const authorizationHeader = req.headers.authorization || ''; |
| // Set GraphQL Apollo server | ||
| const context = ({ req }) => { | ||
| const authorizationHeader = req.headers.authorization || ''; | ||
| const token = authorizationHeader.split(' ')[1]; |
There was a problem hiding this comment.
token, user을 만드는 구문은 어차피 authorization이 없으면 무효한 로직으로 보이는데요
별도의 흐름을 만들거나 별도의 함수로 분리하지 않은 이유가 있을까요?
또한 [1]으로 배열에서 아이템을 픽해오는 것은 암시적으로 느껴지는데요.
명시적으로 변경해보시는건 어떨까요?
| @@ -0,0 +1,38 @@ | |||
| // import dotenv from 'dotenv'; | |||
| DB_PRODUCTION_HOST, | ||
| } = process.env; | ||
|
|
||
| module.exports = { |
| @@ -0,0 +1,68 @@ | |||
| This project was bootstrapped with [Create React App](https://github.com/facebook/create-react-app). | |||
There was a problem hiding this comment.
프론트엔드 프로젝트만의 설명이 없고 기본적으로 생성된 문서라 아쉽네요~
| <Switch> | ||
| <Route exact path="/" component={Login} /> | ||
| <Route path="/user/register" component={RegisterUser} /> | ||
| <Route path="/main" component={SearchRoom} /> |
There was a problem hiding this comment.
�Switch 라우터를 사용하셨는데 Default 케이스도 고려해보셨는지 궁금하네요 :)
| @@ -0,0 +1,3 @@ | |||
| const MOBILE_WIDTH = 800; | |||
|
|
|||
| export const isMobile = () => window.matchMedia(`screen and (max-width: ${MOBILE_WIDTH}px)`).matches; | |||
| import * as Style from './style'; | ||
|
|
||
| export default ({ requestToServer, datePicked, deleteButtonHandle, localeLanguage }) => { | ||
| moment.locale(localeLanguage); |
There was a problem hiding this comment.
얼핏보면 매우 위험할 수 있는 코드인데 괜찮으셨을까요?
| else setFocusedInput(focusedInput); | ||
| }; | ||
|
|
||
| const saveButtonHandler = (startDate, endDate) => () => { |
There was a problem hiding this comment.
의도적으로 Closure를 사용하신걸까요?
어떠한 이득이 있으셨을지 궁금하네요 :)
| export default ({ personnelItems, requestToServer, personnelControlled, deleteButtonHandle }) => { | ||
| const saveButtonHandler = () => () => { | ||
| const personnel = personnelItems.reduce((payload, item) => { | ||
| const [stateValue, stateName] = [item[0], item[3]]; |
There was a problem hiding this comment.
코드를 읽고 이해하는데 상당 부분이 히스토리에 의존적인 로직으로 보여요
다른 사람이 볼 수 있도록 더 명시적으로 수정할 수 있을까요?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
뒤늦게 연락받고 허겁지겁 코드 리뷰를 남기게 되었습니다.
간단하게 일부의 코드만 읽어보고 매우 적은 리뷰 코멘트 남겼습니다.
그래도 마지막으로는 실제 리뷰하듯이 최종 리뷰를 남겼으니... 그 부분을 확인해주시면 좋겠습니다.
감사합니다