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

4조 과제 제출 (유희태, 문현수, 김가은, 이창휘) #9

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

Conversation

kledyu
Copy link

@kledyu kledyu commented Aug 11, 2023

📌 프로젝트 소개

🏢 SOONYANG 프로젝트

NEXT.js, React, TypeScript, Rest API를 활용해서 만든 순양그룹 연차 당직 관리 웹사이트 입니다.

📄 배포사이트

순양그룹 연차/당직 프로그램

🖥️ Repository

Client Repo
Organization

관리자(admin) 계정

ID: [email protected]
PW: admin

프로젝트 기간

2023년 7월 24일 ~ 2023년 8월 11일

💁‍♂️ 개발팀원 및 역할

유희태 문현수 김가은 이창휘
유희태 문현수 김가은 이창휘
인증/인가 메인 페이지 구현 마이 페이지 구현 관리자 페이지 구현

📌Stack

Config

Development


Library

Enviroment


Deployment

Cowork Tools

📌구현 페이지와 주요 기능

🔐 인증/인가

  • 회원가입
  • 로그인
  • 비 로그인 상태 비밀번호 찾기
  • 이메일을 통한 비밀번호 재설정
  • 로그인 상태 비밀번호 변경
  • 비허가 접속 차단
  • 404 에러 대응

2️⃣ 메인 페이지

  • 1

3️⃣ 마이페이지 페이지

  • 개인정보 조회
  • 개인정보 수정
  • 연차, 당직신청 내역 조회

4️⃣ 관리자페이지

  • 사원 조회
  • 사원 필터별 검색
  • 사원 상세정보 조회
  • 사원 상세정보 수정
  • 사원별 승인된 연차 및 당직 조회
  • 사원 휴가 및 당직 요청 조회
  • 사원 휴가 및 당직 요청 승인 및 거절

프로젝트 미리보기

인증/인가

로그인

image image

회원가입
image
image


비밀번호 찾기
image
image


이메일 전송중
image
image


이메일 전송 완료
image
image


비밀번호 재설정
image
image


비밀번호 변경
image
image


비밀번호 변경 II
image
image


접근 권한 없이 마이페이지 접근 시

image image

접근 권한 없이 관리자 페이지 접근 시
image
image


404 에러 페이지

image image


관리자

image

사원 일정 관리 내역 확인
image


휴가 관리
image


image

휴가 승인
image


당직관리
image


사원 개인정보 수정
image


마이페이지

image

개인정보 수정
image


연차/당직 신청 리스트
image

@kledyu kledyu changed the title KDT5_TEAM4 4조 과제 제출 (김가은, 문현수, 유희태, 이창휘) Aug 11, 2023
@kledyu kledyu changed the title 4조 과제 제출 (김가은, 문현수, 유희태, 이창휘) 4조 과제 제출 (유희태, 문현수, 김가은, 이창휘) Aug 11, 2023
@kledyu kledyu self-assigned this Aug 11, 2023
Copy link

@pildrums pildrums left a comment

Choose a reason for hiding this comment

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

사실 저도 잘하는게 아니라서 리뷰하기 참 민망하지만 코드리뷰를 하면서 제가 느낀 점도 적고, 또 배울 점은 또 한수 더 배워가는 코드리뷰 시간이 되었습니다. 프로젝트 작업하시느라 고생많으셨습니다!! 앞으로 같이 더 좋은 개발자될 수 있도록 화이팅하세욥~!ㅎㅎ

export default async function memberInfo() {
try {
const response = await axios.get(
`${clientInstance.defaults.baseURL}/api/personal-info/${employeeId}`,

Choose a reason for hiding this comment

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

인스턴스를 만들어서 사용하신 부분은 상당히 잘하신 것으로 보이지만 바로 clientInstance를 불러서 사용하면 되는데 바로 headers 값하고 baseURL을 직접 조회해서 사용하신 이유가 있을까요? 이를테면
const response = await clientInstance.get('/api/personal-info/${emploeeId}')
인스턴스를 만들 때는 이런 식으로 보통 작성을 하곤 합니다만 특별한 이유가 궁금합니다. (지금 코드에서 제가 템플릿 리터럴을 적을 수가 없어서 본의아니게 따옴표 사용했습니다 이해해주세용 ㅠㅠ)

Copy link
Author

Choose a reason for hiding this comment

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

baseURL을 직접 조회해서 사용하지 않아도 되는군요. 미처 확인하지 못했었습니다. 지적해주셔서 너무나 감사드립니다..!

API 인스턴스에 대한 공부를 더 해야겠다는 생각이 듭니다! 감사합니다 :=) 참고해서 리팩토링 하겠습니다 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

마찬가지로 반복되는 header 등도 인스턴스에 넣어 공통적으로 사용할 수 있습니다~

Copy link
Author

Choose a reason for hiding this comment

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

좋은 지적 감사드립니다!!

대부분의 api 함수에 header가 반복되는데, 간과하고 넘어간 것 같습니다!
불필요한 반복이 없도록 리팩토링 하겠습니다!

) : (
<div className="flex mt-8">
<div className="">
{Object.entries(List).map(([key, value]) => (

Choose a reason for hiding this comment

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

map함수를 통해서 자동으로 렌더링하신 부분은 상당히 좋습니다만 내부에 있는 코드들을 컴포넌트로 따로 만들어서 props로 전달해도 괜찮아보이는데 직접 하드코딩을 하신 이유가 궁금합니다~!

// 추후 리팩토링을 통해 react-query를 통해 API 중복 호출을 관리할 필요
useEffect(() => {
async function getInfo() {
const response = await memberInfo();

Choose a reason for hiding this comment

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

memberInfo라는 함수는 이미 async await가 존재하는 것으로 알고 있는데 한번 더 작성하신 이유가 궁금합니다~!

<button
disabled={disabled}
onClick={onClick}
type={submit ? 'submit' : 'button'}

Choose a reason for hiding this comment

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

타입을 삼항연산자로 처리한 부분은 저도 생각 못했던 부분이네요 ㅎㅎ 잘 배워갑니다 ㅎㅎ

className={`${
secondary ? 'bg-white text-primary' : 'text-white bg-primary'
} block h-10 w-full rounded-md border-2 border-primary px-3 sm:py-2 font-bold ring-subTextAndBorder ring-offset-2 transition hover:opacity-80 text-base focus:ring-2 active:scale-95 disabled:pointer-events-none disabled:opacity-30 sm:h-12 sm:text-base`}>
{contents}

Choose a reason for hiding this comment

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

인덱스 시그니처를 통해서 모든 props 값을 받아올 수 있도록 처리해줘도 되는 부분이네요~!

Choose a reason for hiding this comment

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

Layout이라는 파일 자체는 말그대로 레이아웃을 조정하기 위한 컴포넌트인 것 같은데 스타일링이 전혀 들어가지 않은 부분이 궁금합니다

Copy link
Author

Choose a reason for hiding this comment

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

이 점 참고해서 Container와 같이 sementic을 고려한 코드 구성하도록 리팩토링 진행하겠습니다!

조언 주셔서 너무나 감사합니다 :)

};
return (
<>
{Object.entries(sideList).map(([key, link]) => (

Choose a reason for hiding this comment

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

저 리스트를 배열로 만들어서 안에 객체로 집어 넣어줬으면 어땠을까 하는 아쉬움이 있습니다. 이를테면
const sideList = [ { id: 0, url: '/member', name: '마이페이지' }, // ... ];
이런 식이 일반적입니다.

Copy link
Author

Choose a reason for hiding this comment

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

좋은 말씀해주셔서 감사합니다!!

필진님 말씀대로 배열로 만들어서 객체로 집어 넣는 방식이 오히려 코드가 길어지고, 위와 같이 사용해도 큰 문제는 없다고 생각이 드는데, 혹시 필진님이 설명해주시는 방식이 가지는 이점이 어떤 것인 지 궁금합니다!

Copy link
Member

Choose a reason for hiding this comment

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

  1. object 는 아이템들의 순서를 보장하지 않습니다.
    반면 배열과 Map은 순서를 보장할 것을 명시하고 있습니다.
    사실 object도 엔진에서 순서를 보장하게 데이터를 제시하지만, 어디까지나 엔진에서 임의로 결정한 결과입니다.
    (참고: https://betterprogramming.pub/objects-order-in-javascript-faed6b35de0e)

  2. 배열, Map과 달리 object는 iterable하지 않습니다.
    이는 object 데이터 자체가 순회, 반복 용도로 쓰이지 않는 다는 의미입니다.
    기본적으로 배열은 순서를 세우고 반복을 하기 위해 존재합니다.
    따라서 배열에 반복문을 사용하면 읽는 사람으로 하여금 자연스럽게 읽힙니다만.
    갑자기 object에 반복문을 쓰면 그 자체가 조금 이상한 행위가 될 수 있습니다.

조금 비유를 해보자면

const a = 5;
const b = a + 7;

이라는 코드는 이상하지 않습니다.
왜냐하면 숫자는 더하려고 있는 것이니까요.

const a = [true, true, true, true, true, true, true];
const b = a.length + 7;

이 코드는 굉장히 이상합니다.
처음 코드와 의도는 같지만 배열을 선언해 놓고 배열의 길이를 숫자로 사용하려고 하니까요.

즉, 데이터 타입이라는 것은 그 자체만으로 이 데이터의 사용 목적을 암시하고 있는 것입니다.

(좋은 글 입니다만 그냥 흥미로만 읽으세여: https://developerntraveler.tistory.com/135)

Copy link
Member

Choose a reason for hiding this comment

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

따라서 변수라는 것은 그 존재, 타입, 네이밍 만으로 사용 목적과 부합해야 하고 더 나아가 변수 이름만으로 그 사용 용도가 가늠이 되어야 합니다.
이것이 변수의 이름이 중요한 이유기도 합니다.

Copy link
Author

Choose a reason for hiding this comment

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

Object.entries() 메소드를 통해 객체의 속성을 배열화하여 Map으로 순회, 반복하려는 의도였는데, 이는 순수히 제가 사용하기 편한 방식으로만 사용했던 것 같습니다.

말씀해주신 Array와 Object의 설명을 읽고, 사소한 코드 하나하나 모두 사용 의도와 목적을 고려하여 구성해야겠다고 생각이 듭니다!
남겨주신 글 꼭 읽어보고 앞으로의 코드 구성에 적용하도록 노력하겠습니다!!

감사합니다!!!!!!!! :D

Copy link

@ChoEun-Sang ChoEun-Sang left a comment

Choose a reason for hiding this comment

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

안녕하세요! 재밌는 주제로 반응형 웹까지 구현하시고 정말 멋지십니다 !
코드리뷰 하면서 저도 많이 배워가게 되네요 ㅎㅎ!
정말 고생 많으셨습니다 !! 4조 최고 👍

);

return response.data;
} catch (error) {}

Choose a reason for hiding this comment

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

이 부분 catch 구문에 예외처리를 안 하신 이유가 있나요? 🤨

Choose a reason for hiding this comment

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

이거 확인해보니 누락된거같습니다 ㅠ

requestBody,
{
headers: {
Authorization: `Bearer ${accessToken}`

Choose a reason for hiding this comment

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

headers 부분도 axios config에 넣어서 사용하면 좋지 않을까요..?

Copy link
Author

Choose a reason for hiding this comment

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

날카로운 지적 감사드립니다!
이 부분도 리팩토링해서 일관성있게 axios api 호출 함수 수정하겠습니다 :)

import { Cookies } from 'react-cookie';

const cookie = new Cookies();
const accessToken = cookie.get('accessToken');

Choose a reason for hiding this comment

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

오 로컬스토리지가 아닌 쿠키에 토큰을 저장하신 이유가 궁금합니다..! 🧐

Copy link
Author

Choose a reason for hiding this comment

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

쿠키에서는 옵션을 통해 좀 더 로컬 스토리지보다 보안성을 향상 시킬 수 있다고 판단해서 쿠키에 저장 및 관리했습니다!

로그인 시 쿠키를 secure, SameSite 등의 옵션을 통해 저장하여 HTTPS 통신 외에서는 쿠키를 전달하지 못하게 하고, 크로스 사이트 요청에도 대응해보려 쿠키를 사용하게 되었습니다 !

import { Cookies } from 'react-cookie';

const cookie = new Cookies();
const employeeId = cookie.get('employeeId');

Choose a reason for hiding this comment

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

employeeId를 쿠키에 넣어서 사용하시는 이유가 궁금합니다!

Copy link
Author

@kledyu kledyu Aug 17, 2023

Choose a reason for hiding this comment

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

날카로운 질문 감사드립니다!

모든 서버와의 통신 과정에 보안은 중요하지만, 비밀번호 변경과 같은 과정은 더욱이 보안성을 신경써야한다고 생각했습니다.

엔드포인트에 employeeId를 필요로 하는 서버 통신 과정에서 누군가가 악의적으로 임의의 값을 대량으로 접근하면 개인 정보를 탈취당할 수 있는 가능성이 있기에, 이전 답변에서와 같이 보안성 있게 유지해야한다고 판단했습니다.

따라서 로그인 시 액세스토큰과 동시에 employeeId값을 쿠키에 저장하게 되었습니다!

Copy link
Member

Choose a reason for hiding this comment

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

타당한 이유입니다.
하지만 추가적인 리팩터링이 들어간다면 일단 모든 api에서 employeeId를 안쓰는 방향으로...백엔드와의 조율이 우선되어야 할 것입니다.

이미 얘기를 나누었지만, 내 accessToken을 가지고 있으면 다른 사람의 employeeId를 임의로 수정할 수 있게 되어버리니까요.(기본적으로 토큰이든 스토리지든 사용자가 변경할 수 있으니)
이건 프론트엔드 만의 작업으로 되는 건 아니지만, 추가로 적어둡니다.

Copy link

@cuconveniencestore cuconveniencestore left a comment

Choose a reason for hiding this comment

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

라이브러리를 사용하지않고 직접 캘린더나 기능들을 구현하신점이 인상깊었습니다! 또 컴포넌트화에 무척이나 신경쓰신부분에 배울점이 많았습니다! 추가적으로 테일위드 css를 사용하신 부분에 있어 장단점이 있었을까요? 아직 한번도 사용해보지 못해서 어떤점이 장단점이 있는지 궁금합니다! (제가 생각하기에 단점이라고 느껴지는 부분은 함께 진행하는 프로젝트이다 보니 서로의 코드를 볼때 가독성이 떨어지지는 않으셨나요? 어떤방식으로 서로의 코드를 공유하고 이해하셨는지 궁금합니다!) 🥹
이번 미니 프로젝트도 너무 고생많으셨습니다! 코드를 읽어보다 보니 너무나 배울점이 많네요 ㅠㅠ 정말 수고 하셨습니다!

Choose a reason for hiding this comment

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

api 폴더에서 기능별로 파일을 만들어서 관리하는 경우가 대부분이었는데 각 api 당 파일을 만들어서 관리하신 이유가 따로 있을까요?? 장점이라던지!!! 이유가 궁금합니다 🫠

Copy link

Choose a reason for hiding this comment

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

사실 한 파일에 담아서 사용해도 큰 문제는 없다고 생각이 듭니다! 하지만 개인적으로 생각해보았을 때, 아무래도 비슷한 형식의 api가 한 파일에 전부 담겨 있게 된다면 사용할 때, 헷갈릴 것이라고 생각이 들어서 연차 부분 api와 당직 부분 api를 분리해서 사용했습니다!

@@ -0,0 +1,149 @@
export const HOLIDAYS = [
'0101',

Choose a reason for hiding this comment

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

캘린더를 직접 만들어서 사용하신점이 대단합니다! 하지만 공휴일정보 데이터를 따로 받아서 사용하지 않고 직접 지정하신 이유가 따로 있으신가요?

Copy link

Choose a reason for hiding this comment

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

아 이 부분은 리팩토링 시에 괜찮은 방식이 있다면 변경해보려고 하는 부분이 었는데요... 구차하지만 좀 깊은 사연이 있습니다...
제가 원래는 휴일을 알려주는 공용 api를 사용하고자 했었는데 하필 데이터 형식이 XML이었습니다. 제가 XML은 사용해본 적이 없어서 JSON으로 변환을 해야하는 상황이었고, 변환을 시도하였으나 결국 실패하였습니다...
그래서 다음으로 모색한 방식은 추석, 설날 등 음력휴일의 정보를 양력으로 전환하는 방법이었는데요. 이 역시 시도하다가 실패를 하였습니다....(진짜 너무 어려웠어요)
결과적으로 제가 생각한 모든 방식을 실패한 시점에서 과제 제출까지의 시간이 촉박했고, 하는 수 없이 번거롭지만 향후 10년 간의 휴일 데이터(부처님오신날, 추석, 설날, 대체 공휴일)를 상수 값으로 저장해서 공휴일 정보로 사용하게 되었습니다...

Copy link
Member

@GyoHeon GyoHeon left a comment

Choose a reason for hiding this comment

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

총평

수고하셨습니다.
회사의 존재도 재미있고 위트 있습니다.

아쉬운 점

  1. 컴포넌트가 너무 거대합니다.
    컴포넌트도 하나의 함수일 뿐입니다.
    가벼워야 재사용성도 가독성도 유지보수에도 좋습니다.
  2. 재사용의 부재가 눈에 띕니다.
    사실 이건 소통의 문제일 수도 있습니다.
    내가 한 작업을 다른 사람은 두 번 하지 않도록 할 수 있었을 겁니다.
    2-1. 테일윈드 사용시 클래스 네이밍의 반복이 부족
    분명 테일윈드 사용시에도 반복 되는 클래스 네이밍이 많았을 겁니다.
    이런 부분도 인지해서 불필요한 코드의 양을 줄이는 것도 좋은 방법입니다.
  3. util 함수의 부재
    컴포넌트에서 사용되는 여러 로직들을 따로 분류하고 이를 사용하여 가독성도 높이고 로직의 불필요한 재작성을 막을 수 있습니다.

마무리...

이번 과정에서 제가 여러분께 드리는 마지막 코드리뷰고, 여러분이 받는 마지막 코드리뷰겠네요.
제가 한 모든 코드리뷰는 한 사람의 의견으로 듣되, 왜 이런 의견이 나왔는지 고심해 보시길 바랍니다.
짧은 시간 동안 제가 여러분들의 사수 역할을 조금이나마 해냈다면 좋겠네요.

마지막 프로젝트는 아니지만 그 동안 고생하셨고, 여러분이 노력하신 만큼 결과가 뒤따라오길 기원하겠습니다.
다음에 뵐때는 멘토와 수강생이 아니라 개발자와 개발자로 뵙겠습니다.

수고하셨습니다.

export default async function memberInfo() {
try {
const response = await axios.get(
`${clientInstance.defaults.baseURL}/api/personal-info/${employeeId}`,
Copy link
Member

Choose a reason for hiding this comment

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

마찬가지로 반복되는 header 등도 인스턴스에 넣어 공통적으로 사용할 수 있습니다~

import { Cookies } from 'react-cookie';

const cookie = new Cookies();
const employeeId = cookie.get('employeeId');
Copy link
Member

Choose a reason for hiding this comment

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

타당한 이유입니다.
하지만 추가적인 리팩터링이 들어간다면 일단 모든 api에서 employeeId를 안쓰는 방향으로...백엔드와의 조율이 우선되어야 할 것입니다.

이미 얘기를 나누었지만, 내 accessToken을 가지고 있으면 다른 사람의 employeeId를 임의로 수정할 수 있게 되어버리니까요.(기본적으로 토큰이든 스토리지든 사용자가 변경할 수 있으니)
이건 프론트엔드 만의 작업으로 되는 건 아니지만, 추가로 적어둡니다.

Comment on lines +6 to +8
const cookie = new Cookies();
const employeeId = cookie.get('employeeId');
const accessToken = cookie.get('accessToken');
Copy link
Member

Choose a reason for hiding this comment

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

어차피 cookie, employeeId, accessToken 전부 api 함수에서만 사용하니 밖에서 선언할 필요는 없어보입니다.
로컬 스코프에서 사용되는 변수들은 로컬에서만 선언하는 것이 좋습니다.

Copy link
Author

Choose a reason for hiding this comment

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

말씀대로 위 코드는 전역적으로 사용될 이유가 없고, 로컬 스코프 내에서만 사용된다고 생각이 듭니다!

이 부분 리팩토링 진행하고 추후 코드 작성 시에, 로컬 스코프 내에서만 사용되어도 될 코드는 전역 스코프에서 사용되지 않도록 신경쓰겠습니다!

감사합니다 :D

Comment on lines +15 to +16
<div className=" flex ">
<select className=" " value={value} onChange={handleChange}>
Copy link
Member

Choose a reason for hiding this comment

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

className에 불필요한 공백이 들어가거나 아예 className이 필요없는 상황이 보이네요.
가독성을 위해 깔끔하게 가공했으면 합니다.

Comment on lines +26 to +27
const [renderModal, setRenderModal] = useState(true); // Diaglog Modal 렌더링 조건
const [isSidebarOpen, setIsSidebarOpen] = useState<boolean>(true);
Copy link
Member

Choose a reason for hiding this comment

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

useState에 초기값을 지정해주면 type 추론으로 기본 타입이 알아서 할당됩니다.
물론 타입이 초기값에서 더 확장되는 경우가 있는 경우 타입을 지정해 줄 수 있지만, 굳이 코드의 길이를 늘리면서까지 추론이 가능한 변수에 타입을 추가로 지정해줄 필요는 없습니다.

무엇보다 중요한 것은 통일인데, useState에 무조건 초기값을 지정해주는 방식으로 통일을 하거나 or 굳이 지정해줄 필요가 없는 경우엔 지정하지 않는 방식으로 통일하는 것이 가장 좋습니다.

};
return (
<>
{Object.entries(sideList).map(([key, link]) => (
Copy link
Member

Choose a reason for hiding this comment

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

  1. object 는 아이템들의 순서를 보장하지 않습니다.
    반면 배열과 Map은 순서를 보장할 것을 명시하고 있습니다.
    사실 object도 엔진에서 순서를 보장하게 데이터를 제시하지만, 어디까지나 엔진에서 임의로 결정한 결과입니다.
    (참고: https://betterprogramming.pub/objects-order-in-javascript-faed6b35de0e)

  2. 배열, Map과 달리 object는 iterable하지 않습니다.
    이는 object 데이터 자체가 순회, 반복 용도로 쓰이지 않는 다는 의미입니다.
    기본적으로 배열은 순서를 세우고 반복을 하기 위해 존재합니다.
    따라서 배열에 반복문을 사용하면 읽는 사람으로 하여금 자연스럽게 읽힙니다만.
    갑자기 object에 반복문을 쓰면 그 자체가 조금 이상한 행위가 될 수 있습니다.

조금 비유를 해보자면

const a = 5;
const b = a + 7;

이라는 코드는 이상하지 않습니다.
왜냐하면 숫자는 더하려고 있는 것이니까요.

const a = [true, true, true, true, true, true, true];
const b = a.length + 7;

이 코드는 굉장히 이상합니다.
처음 코드와 의도는 같지만 배열을 선언해 놓고 배열의 길이를 숫자로 사용하려고 하니까요.

즉, 데이터 타입이라는 것은 그 자체만으로 이 데이터의 사용 목적을 암시하고 있는 것입니다.

(좋은 글 입니다만 그냥 흥미로만 읽으세여: https://developerntraveler.tistory.com/135)

};
return (
<>
{Object.entries(sideList).map(([key, link]) => (
Copy link
Member

Choose a reason for hiding this comment

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

따라서 변수라는 것은 그 존재, 타입, 네이밍 만으로 사용 목적과 부합해야 하고 더 나아가 변수 이름만으로 그 사용 용도가 가늠이 되어야 합니다.
이것이 변수의 이름이 중요한 이유기도 합니다.

Comment on lines +31 to +105
const Weeks = () => {
const days = []
const date = ['일','월','화','수','목','금','토']

//0부터 해당되는 인덱스의 요일을 days에 입력
for(let i=0; i<7; i++){
days.push(
<div className="grow text-xs text-center" key={i}>
{date[i]}
</div>
)
}
return(
<div className="flex pb-4">{days}</div>
)
}

const RenderDays = () => {
const monthStart = startOfMonth(currentDate)
const monthEnd = endOfMonth(currentDate)
const startDate = startOfWeek(monthStart)
const endDate = endOfWeek(monthEnd)

const rows = []
let days = []
let day = startDate;
let formattedDate = ''

while(day<=endDate){
for(let i=0; i<7; i++){
formattedDate = moment(day).format('D')
let checkedMonth = moment(day).format('MM')
let today = moment(day).format('YYYY.MM.DD') === currentDayForm
days.push(
<div
className={`h-8 grow text-xs flex justify-center items-center rounded-full leading-6 text-center
${today ? `bg-primary text-white` : null}
hover:bg-primaryHover hover:text-white`}
key={formattedDate}>
{ checkedMonth === currentMonth ?
(
<div className="h-6 w-6">
{formattedDate}
</div>
)
:
(
<div className="h-6 w-6 text-mainGray">
{formattedDate}
</div>
)
}

</div>
)
day = addDays(day, 1)
}
rows.push(
<div
className="flex"
key={formattedDate}>
{days}
</div>
)
days = [];
}

return (
<div>
<div>
{rows}
</div>
</div>
)
}
Copy link
Member

Choose a reason for hiding this comment

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

컴포넌트 안에서는 컴포넌트를 선언하면 안됩니다.

참고: https://react.dev/learn/your-first-component#nesting-and-organizing-components

Copy link
Member

Choose a reason for hiding this comment

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

이런 함수들은 컴포넌트가 아니라 util 등의 네이밍으로 따로 빼서 관리해 주는 것이 간편합니다.

Comment on lines +4 to +26
const validMessage = () => {
if (!valid && name === 'email') {
return (
<div className="text-secondary text-xs ml-1">
이메일 주소를 정확히 입력해주세요.
</div>
);
}
if (!valid && name === 'password') {
return (
<div className="text-secondary text-xs ml-1">
영문, 숫자를 조합하여 입력해주세요. (8 - 16자)
</div>
);
}
if (!valid && name === 'confirmPassword') {
return (
<div className="text-secondary text-xs ml-1">
비밀번호가 일치하지 않습니다.
</div>
);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

  1. 전부 리턴에 같은 div, className이 들어갑니다. 단순히 텍스트만 리턴해줘도 되는 상황입니다.
  2. 모든 if에 !valid가 들어갑니다. 공통된 부분은 밖으로 빼서 사용할 수 있습니다.
  3. early return을 통해서 if 문 자체를 줄일 방법도 생각해보세요.
  4. if문에 걸리지 않으면 아무것도 리턴하지 않습니다. 예외처리 용도로 기본 값을 지정해 둘 필요가 있어보입니다.

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.

7 participants