프론트엔드 코드 리뷰 #11
프론트엔드 코드 리뷰 #11
Conversation
There was a problem hiding this comment.
고생하셨습니다.
전반적으로 이해가 잘 되는 코드를 작성해 주셔서 큰 무리 없이 어떤 작업을 하시려고 했는지 이해할 수 있었습니다. 테스트 코드를 짜신 부분과 스타일 영역을 별도로 분리한 부분도 바람직한 것 같아요.
다만 컴포넌트에서 Sementic HTML tag를 조금 목적에 맞지 않게 썼던 부분들이 있었고, 삼항연산자 등의 코드가 일부 가독성이 떨어지는 부분이 있는 것 같아서 코멘트를 드렸습니다. 그리고 전반적으로 중복되는(또는 중복이 될 가능성이 있는) 컴포넌트 코드가 많아 보였는데 이런 부분은 재사용할 수 있게 리팩토링을 해 주셔도 좋을 것 같다는 의견입니다.
작업 하시느라 고생 많으셨습니다.
|
|
||
| module.exports = { | ||
| development: { | ||
| username: DB_DEV_USER, |
There was a problem hiding this comment.
중복되는 코드는 가능하면 줄이는 것이 좋습니다. 왜냐하면 지금은 코드가 많지 않아서 문제가 없다고 느낄 수도 있지만, 이후 복잡도가 증가하면 코드를 수정할 때 유지보수의 측면에서 버그가 발생할 수 있는 가능성이 높아지기 때문입니다. DRY(Don't Repeat Yourself) 원칙은 이후로도 항상 기억하고 코드를 작성하는 것이 바람직할 것 같습니다.
| } | ||
| }); | ||
|
|
||
| db.sequelize = sequelize; |
There was a problem hiding this comment.
db 객체의 필드값이 이렇게 비슷하면 이후에 헷갈리기 쉬울 것 같습니다. 변수 명을 조금 더 명확하게 구분을 해 주는 것도 좋을 것 같아요.
| name: '개인실', | ||
| }, | ||
| { | ||
| name: '호텔 객 실', |
| @@ -0,0 +1,13 @@ | |||
| NODE_ENV= | |||
There was a problem hiding this comment.
.env 파일은 실제로 사용할 때는 민감한 정보들이 많이 있을 수 있으니 .dev.env 형태로 github에서 보이지 않게 해주는 것도 보안상 좋을 것 같아요.
|
|
||
| fs.readdirSync(__dirname) | ||
| .filter(file => { | ||
| return file.indexOf('.') !== 0 && file !== basename && file.slice(-3) === '.js'; |
There was a problem hiding this comment.
한 줄이라도 만약 한 눈에 보기에 조금 복잡하다는 느낌이 든다면 별도의 함수 형태로 외부에 빼는 것도 좋을 것 같아요. 이후에 조건이 더 붙는다면 그렇게 하는 것을 권장합니다. 다른 개발자가 코드를 이해하는데 훨씬 더 도움이 될 것입니다.
| import 'react-dates/lib/css/_datepicker.css'; | ||
| import { DayPickerRangeController } from 'react-dates'; | ||
|
|
||
| import moment from 'moment'; |
There was a problem hiding this comment.
moment 라이브러리는 현재 개발이 더이상 진행이 되지 않는다고 공식문서에 나와 있습니다. 가능하면 다른 대안들을 찾아보는 것도 좋을 것 같아요. 공식문서
| expect(modal).toHaveStyle('padding: 2rem;'); | ||
| }); | ||
|
|
||
| it('배경이 흰색이다.', () => { |
There was a problem hiding this comment.
CSS를 테스트 코드로 짜는 것이 잘못된 것은 아니지만, 꼭 필요한 테스트인지 다시 한 번 고민해 볼 필요가 있을 것 같습니다. 가능하면 비즈니스 로직이나 중복해서 쓰이는 유틸 함수 등을 테스트 코드로 먼저 짜보는 것도 권장드립니다.
| ))} | ||
| <Style.PersonnelControllerButtonWrapper> | ||
| <a onClick={deleteButtonHandle}>삭제</a> | ||
| <a className="Save-Button" onClick={saveButtonHandler()}> |
There was a problem hiding this comment.
a 태그를 사용하셨는데, 이 부분은 버튼이기에 button 태그가 조금 더 적합할 것 같아요. HTML sementic tag에 대해서 모질라 등 공신력있는 페이지의 글들을 읽어보는 것을 추천합니다.
| const { optionFirstLine, optionSecondLine } = convertOptionDataToStrings(info.maxGuest, option); | ||
| return ( | ||
| <Style> | ||
| <div className="Room-image"></div> |
There was a problem hiding this comment.
조금더 확장성을 고려해 본다면, 각각의 컴포넌트를 이렇게 직접 다 만들어 주는 것보다는, 재사용할 수 있는 컴포넌트를 따로 만들어서 가져다가 쓰는 식으로 구조를 리팩터링하고, 필요한 데이터만 해당 컴포넌트에 보내서 1급 객체의 형태로 만들어 보는 방향을 권장합니다.
|
|
||
| // State | ||
| const priceState = useState([ | ||
| price && price.startPrice ? price.startPrice : MIN_PRICE, |
There was a problem hiding this comment.
이 코드는 아래와 같이 작성을 하는 건 어떨까요?
const priceState = useState([ price?.startPrice || MIN_PRICE, price?.endPrice || MAX_PRICE ]);
No description provided.