Code Review #10

lechuckroh · 2021-08-24T12:56:08Z

고생하셨습니다.

테스트 작성과 함께 DB Migration 도구를 사용해 자동화된 DB 관리를 하려고 노력하신게 좋아 보입니다.
ERD 도 잘 작성하셨네요.

전체적으로 조금 아쉬운 점은

일관된 코드 스타일이 부족해보입니다. prettier와 같은 도구를 한 번 고려해보는것도 좋을 것 같습니다.
eslint 와 같은 정적 코드 분석 도구를 사용해, 기본적인 코드 스멜을 자동으로 걸러주는 것도 추천드립니다.

코드 리뷰를 진행하면서 중복된 항목은 하나만 코멘트를 작성했습니다.

lechuckroh · 2021-08-24T12:57:18Z

review_practice/api-server/Dockerfile

+# RUN npm ci --only=production
+
+# 앱 소스 추가
+COPY . .


node_modules 디렉토리가 있는경우, 같이 복사되기 때문에 문제가 발생할 수 있습니다. .dockerignore 파일을 사용해 node_modules 를 제외하는 것이 좋아보입니다.

lechuckroh · 2021-08-24T12:58:23Z

review_practice/api-server/batch-job.sh

@@ -0,0 +1,8 @@
+# convert csv to json


shell script 작성시 첫 라인에 shebang 을 지정해 사용할 shell 을 지정해 주는 것이 좋습니다. #!/bin/sh 혹은 #!/bin/bash 등과 같이 사용할 수 있습니다.

lechuckroh · 2021-08-24T12:59:21Z

review_practice/api-server/batch-job.sh

@@ -0,0 +1,8 @@
+# convert csv to json
+cd init-data


스크립트 실행 디렉토리가 스크립트가 위치한 디렉토리와 다를 수 있기 때문에 스크립트 디렉토리로 이동 후 실행하는 것이 안전합니다.

SCRIPTPATH="$( cd "$(dirname "$0")" >/dev/null 2>&1 || exit ; pwd -P )" cd "$SCRIPTPATH/.." || exit

위와 같이 작성해서 루트 디렉토리로 이동할 수 있습니다.

lechuckroh · 2021-08-24T13:00:24Z

review_practice/api-server/config/database.js

+        host: DB_PRODUCTION_HOST,
+        dialect: 'mysql',
+    },
+};


DB_DEV_*, DB_PRODUCTION_* 를 별도로 정의하는 대신, 각 환경별로 dev.env, prod.env 와 같이 파일을 분리하고, 변수명을 DB_USER, DB_PASSWORD 와 같이 정의하는 것이 나중에 실행 환경이 더 늘어났을때 확장하는데 도움이 될 것 같습니다. 그리고 PRODUCTION 정보는 git 저장소에 저장이 안되도록 분리해서 .gitignore에 등록해두는게 좋습니다.

lechuckroh · 2021-08-24T13:01:13Z

review_practice/api-server/database/models/index.js

+
+fs.readdirSync(__dirname)
+    .filter(file => {
+        return file.indexOf('.') !== 0 && file !== basename && file.slice(-3) === '.js';


startsWith, endsWith 를 사용하는게 더 깔끔할것 같습니다.

lechuckroh · 2021-08-24T13:02:05Z

review_practice/api-server/database/models/index.js

+    if (db[modelName].associate) {
+        db[modelName].associate(db);
+    }
+});


modelName을 value를 가져오는 용도로만 사용하고 있기 때문에 Object.values() 를 사용하는게 더 좋아보입니다.
그리고 optional chaining을 사용해 db[modelName].assiciate?.(db) 와 같이 한줄로 코드를 줄일 수 있습니다.

lechuckroh · 2021-08-24T13:03:20Z

review_practice/api-server/database/models/room.js

+        const models = require('../models');
+
+        const joinCondition = { model: models.RoomOption };
+        if (searchOptions && searchOptions.date) {


optional chaining 을 사용해 if (searchOption?.date) 로 사용하면 좋을 것 같습니다

lechuckroh · 2021-08-24T13:03:59Z

review_practice/api-server/database/models/room.js

+                where: roomCondition,
+            });
+        } catch (error) {
+            throw error;


error를 캐치해서 그대로 반환하는거라면 굳이 try/catch 를 사용할 필요가 없어 보입니다.

lechuckroh · 2021-08-24T13:05:07Z

review_practice/api-server/util/csv-converter.js

+            else if (data[key] === 'FALSE') data[key] = false;
+            else if (data[key] === 'null') data[key] = null;
+            else if (!isNaN(data[key])) data[key] = Number(data[key]);
+        }


value를 가져오기 위해 매번 key를 사용해서 조회하기 보다는 Object.entries()를 사용해 한번에 key, value를 가져오는것이 좋을듯합니다.

lechuckroh · 2021-08-24T13:06:23Z

review_practice/api-server/util/jwt-manager.js

+export const isTokenValid = token => {
+    try {
+        const decoded = jwt.verify(token, TOKEN_SECRET_KEY);
+        return decoded;


변수 할당 없이 바로 반환해도 될 것 같네요.

lechuckroh · 2021-08-24T13:07:13Z

review_practice/react-app/src/components/DatePicker/index.js

+import 'react-dates/lib/css/_datepicker.css';
+import { DayPickerRangeController } from 'react-dates';
+
+import moment from 'moment';


monent.js 는 이제 죽은 프로젝트라 다른 라이브러리를 사용하기를 권장합니다.

lechuckroh · 2021-08-24T13:08:08Z

review_practice/react-app/src/components/DatePicker/index.js

+
+    const onFocusChange = focusedInput => {
+        if (focusedInput === null) setFocusedInput('startDate');
+        else setFocusedInput(focusedInput);


setFocusedInput(focusedInput || 'startDate') 로 변경할 수 있을 것 같습니다.

lechuckroh · 2021-08-24T13:09:22Z

review_practice/react-app/src/components/PersonnelController/index.js

+        <Style.PersonnelController>
+            {personnelItems.map((state, index) => (
+                <Style.PersonnelItem key={index}>
+                    <Style.ItemNameWrapper>{state[2]}</Style.ItemNameWrapper>


state[i] 대신 destructuring 해서 변수명을 할당하는 것이 코드 가독성과 유지보수성이 좋아질 것 같네요.

lechuckroh · 2021-08-24T13:12:10Z

review_practice/react-app/src/components/PriceRangeSlider/index.js

+            <Style.RangedPriceWrapper>
+                <input className="Price-Input" type="text" value={`￦ ${price[0]}`} readOnly />
+                <span className="Price-Connector">-</span>
+                <input className="Price-Input" type="text" value={`￦ ${price[1]}`} readOnly />


배열형으로 넘어오는데 변수명이 단수형인 price라서 가독성을 떨어트리고 있습니다. prices와 같은 변수명이 좋아보입니다.

lechuckroh · 2021-08-24T13:15:11Z

review_practice/react-app/src/components/PriceRangeSlider/index.js

+
+import Slider from '@material-ui/core/Slider';
+
+const PriceRangeSlider = ({ value, minPrice, maxPrice, rangeSlided, saveButtonHandle, deleteButtonHandle }) => {


saveButtonHandle 변수명이 저장버튼 핸들? 혹은 버튼핸들 저장? 으로 생각이 되기 때문에, 명확한 단어를 사용하는게 좋을 것 같습니다.

saveButtonHandler, saveHandle, onSave 등과 같은 변수명이 좀 더 이해가 쉬울 것 같습니다.

lechuckroh · 2021-08-24T13:16:34Z

review_practice/react-app/src/components/RoomItem/index.js

+    const optionFirstLine = `인원 ${maxGuest}명 · 침실 ${bedroom}개 · 침대 ${bed}개 · 욕실 ${bathroom}개`;
+    const optionSecondLine = `${freeParking ? '건물 내 무료 주차 ' : ''}${wifi ? '무선 인터넷 ' : ''}${
+        kitchen ? '주방 ' : ''
+    }${washer ? '세탁기 ' : ''}`;


이 부분은 코드 가독성이 너무 좋지 않습니다. 각 항목을 array 에 넣고 join() 메서드를 사용하는 것과 같은 방법을 사용하는 편이 가독성이 더 좋을 것 같습니다.

lechuckroh · 2021-08-24T13:17:59Z

review_practice/react-app/src/containers/RoomList/index.js

+    };
+
+    const getRoomById = roomId => {
+        return data.rooms.filter(({ room }) => room.id === roomId)[0].room;


rootId 를 찾지 못한 경우 에러 핸들링이 되어 있지 않습니다.

lechuckroh · 2021-08-24T13:19:11Z

review_practice/react-app/src/containers/RoomList/ReservationModalBody.js

+
+import { getNightValueFromDateString } from 'util/ConverDate';
+
+export default ({ room, searchOptions, closeModal }) => {


React Developer Tool 에서 디버깅을 위해서는 HOC 의 경우 displayName 을 지정하거나 함수 이름을 지정하는 것이 좋습니다.
컨벤션: 간단한 디버깅을 위한 디스플레이 네임 작성 방법

lechuckroh · 2021-08-24T13:20:21Z

review_practice/react-app/src/containers/SearchBar/ModalBody/PersonnelFilter.js

+    return (
+        <PersonnelController
+            personnelItems={personnelItems}
+            requestToServer={personnel => {


위에서 destructuring한 personnel 변수를 shadowing 하고 있습니다.

지금은 문제가 없지만, 나중에 유지보수하면서 리팩토링등으로 변수 이름이 변경되거나 할 경우 실수로 다른 값을 참조할 가능성이 높아지기 때문에 변수명이 겹치지 않도록 하는 것이 좋을것 같습니다

lechuckroh · 2021-08-24T13:20:51Z

review_practice/react-app/src/containers/SearchBar/ModalBody/PersonnelFilter.js

+            personnelControlled={([adult, children, infant]) => {
+                const guest = adult + children + infant;
+                const buttonName =
+                    guest === 0 ? '인원' : `게스트 ${guest}명${infant === 0 ? '' : `, 유아 ${infant}명`}`;


이렇게 중첩된 3항 연산자를 사용하는 경우, 코드 작성시에는 바로 이해가 되지만 나중에 유지보수를 위해서 코드를 읽게 되는 경우 코드 가독성이 떨어져서 한참을 들여다봐야하는 문제가 발생할 수 있습니다.

이런 경우 중첩된 3항 연산자를 별도의 변수에 할당을 해서 사용하게 되면 좀 더 코드 가독성이 높아집니다.

lechuckroh · 2021-08-24T13:21:41Z

review_practice/react-app/src/util/ConverDate.js

+    const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24));
+
+    return diffDays;
+};


함수명을 여기서 사용한 변수명과 동일하게 getDiffDays() 로 하는게 좀 더 일관성있지 않을까 싶습니다.

그렇게 하면 마지막에 diffDays 를 따로 변수에 할당하지 않고 바로 리턴해도 코드 가독성을 해치지 않을것 같네요

lechuckroh · 2021-08-24T13:25:13Z

review_practice/api-server/util/jwt-manager.js

+        );
+    });
+
+export const isTokenValid = token => {


is로 시작하는 함수명을 보면 boolean 타입을 반환하는 것 처럼 보이지만, 실제로는 디코딩한 값을 반환하고 있기 때문에, 함수명을 헷갈리지 않게 다른 이름으로 변경하는것이 좋을것 같습니다.

connectfoundation added 20 commits August 12, 2021 10:44

Update README.md

4f52d28

Update README.md

adbf62c

Add files via upload

172d937

Update README.md

a005751

Update README.md

5795ec0

Update README.md

fa69842

Update README.md

7c49622

Update README.md

d639dbe

Update README.md

5c292a3

Update README.md

a6e7674

Update README.md

e85ccb0

Update README.md

27d7cb0

Update README.md

b7f7d55

Update README.md

289922f

Update README.md

afdf95f

Update README.md

3228e23

Update README.md

3306cad

Update README.md

346cb9b

Update README.md

0627e2f

Update README.md

0905019

lechuckroh commented Aug 24, 2021

View reviewed changes

lechuckroh marked this pull request as ready for review August 24, 2021 13:23

lechuckroh commented Aug 24, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review #10

Code Review #10

lechuckroh commented Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021 •

edited

Loading

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021 •

edited

Loading

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021

lechuckroh Aug 24, 2021


		import Slider from '@material-ui/core/Slider';

		const PriceRangeSlider = ({ value, minPrice, maxPrice, rangeSlided, saveButtonHandle, deleteButtonHandle }) => {


		import { getNightValueFromDateString } from 'util/ConverDate';

		export default ({ room, searchOptions, closeModal }) => {

Code Review #10

Are you sure you want to change the base?

Code Review #10

Conversation

lechuckroh commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lechuckroh Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lechuckroh Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lechuckroh Aug 24, 2021 •

edited

Loading

lechuckroh Aug 24, 2021 •

edited

Loading