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

[3주차 기본/심화/공유 과제] React 연습 #6

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

Conversation

daahyunk
Copy link
Member

@daahyunk daahyunk commented Nov 5, 2024

✨ 구현 기능 명세

💡 기본 과제

  • Context API, 전역상태 라이브러리 사용 X (ThemeProvider 제외)
  1. 헤더
  • 게임/랭킹 2개의 메뉴 선택 가능
  • 게임 선택 시 헤더 우측에 레벨 선택 Select와 타이머 표시
  • 게임 선택 시 게임 판 출력
  • 랭킹 선택 시 헤더 우측엔 아무것도 나오지 않음
  • 랭킹 선택 시 랭킹 보드 출력
  1. 게임
  • (기본) 한 종류의 레벨만 구현
  • 숫자는 항상 랜덤으로 표시됨. (초기 표시 숫자들도, 이후 열리는 숫자들도 모두 랜덤)
  • 처음에 표시되는 숫자는 클릭해야 하는 숫자의 앞에 절반임. 만약 level 1이라 118까지 클릭해야한다면, 처음에는 19까지의 숫자가 랜덤으로 보여짐
  • 게임판 위쪽에 다음으로 클릭해야할 숫자를 표시
  • 1을 누르는 순간 게임이 시작되며 헤더 우측의 타이머가 동작. 타이머는 소수점 2번째 자리까지 측정.
  • 마지막 숫자 클릭시 게임 종료
  • 게임 종료 시, 타이머를 멈추고 alert 창을 띄워주며 걸린 시간을 표시
  • 게임 종료 시, 현재 시각, 게임의 레벨, 플레이 시간 3개의 정보를 localStorage에 저장 (랭킹에서 사용)
  • 종료 창에서 확인 누르면 다시 시작할 수 있는 상태로 게임 초기화
  • 게임 중 level 변경 시 다시 시작할 수 있는 상태로 게임 초기화
  1. 랭킹
  • localStorage에서 데이터 불러오기
  • 플레이 시간 오름차순으로 보여야 함 (빨리 깬 기록이 위쪽)
  • 우측 상단의 초기화 버튼 누르면 대시보드 초기화 (localStorage도 초기화)

🔥 심화 과제

  1. 게임
  • Level 선택 가능
    Level 1: 3 x 3, Level 2: 4 x 4, Level 3: 5 x 5
  • 숫자 클릭할 때 클릭되는 것 같은 효과 (예시: 깜빡거림)
  • 게임 종료 alert 대신, React의 createPortal을 사용하여 Modal 구현
    createPortal
  1. 랭킹
  • Level 내림차순 & 시간 오름차순 정렬(정렬 기준이 2개). 높은 Level이 위쪽으로, 같은 레벨 중에선 플레이 시간이 짧은게 위쪽으로 정렬

공유과제

제목: React Dev Tools? 이것 뭐예요~??

링크 첨부 : https://wave-web.tistory.com/85


❗️ 내가 새로 알게 된 점

1️⃣ Array.from() 메소드

숫자 배열을 쉽게 만들 방법이 필요했는데 감이 잡히지 않아서 찾아본 후에야 Array.from() 메소드를 알게 되었다. 이 메소드는 유사 배열이나 이터러블 객체로부터 새로운 배열을 만들어주는 유용한 메소드라고 함...!

이번에 Array.from()을 사용한 이유는 특정 범위의 숫자 배열을 간단하게 생성할 수 있기 때문이었다. 보통은 [1, 2, 3, ..., N]처럼 연속된 숫자가 들어있는 배열이 필요할 때 for 문을 사용해 직접 푸시하거나 map을 써서 번거롭게 만들곤 했는데 Array.from()을 쓰니까 코드가 깔끔해졌다는 느낌을 받았다.

🛠️ Array.from()은 어떻게 쓰는 거지?

Array.from()은 보통 두 가지 인수를 받음

  1. 배열의 길이를 정의하고,
  2. 각 인덱스에 어떤 값을 넣을지를 정의하는 콜백을 사용

ex) Array.from({ length: N }, (_, i) => i + 1) 이렇게 작성하면 1부터 N까지의 숫자 배열을 쉽게 만들 수 있다. (_, i) => i + 1 이 부분이 각 인덱스에 i + 1 값을 넣어주는 콜백 함수다. 여기서 _는 배열 요소가 필요 없을 때 쓰는 자리고 i는 현재 인덱스 값을 가리킴,,,!

2️⃣ 피셔-예이츠 셔플 알고리즘

이전에 셔플을 구현했어야 했던 경험이 있는데 그땐 sortMath.random() - 0.5를 이용해 셔플을 구현했었다. Math.random()은 0에서 1 사이의 난수를 반환하므로 Math.random() - 0.5는 양수나 음수를 랜덤으로 뱉게 되는데 sort는 리턴값이 음수면 a와 b 자리를 바꾸고 양수라면 바꾸지 않는 메소드이니 그걸 사용해서 자리를 랜덤으로 바꾸는... 방법이었다. 그렇기에 이번에도 그렇게 하면 되겠다 싶어 그렇게 구현을 했었는데, 문득 이게 괜찮은 방법인가? 더 최적의 방법이 있을 것 같은데... 싶어 바로 찾아봤다. 그랬더니

https://velog.io/@keeper1826/sort%EB%A5%BC-%EC%9D%B4%EC%9A%A9%ED%95%9C-%EC%85%94%ED%94%8C%EC%9D%98-%EB%AC%B8%EC%A0%9C%EC%A0%90-javaScript

이런 아티클이 있었다. 이 아티클에서 피셔-예이츠 알고리즘을 알게 되어 바로 적용했다.

🐟 피셔 예이츠 알고리즘이란?

카드를 섞는 과정이랑 비슷하다고 생각하면 쉽다. 우리가 카드 더미를 들고 있다면, 가장 뒤에서부터 한 장씩 뽑아 앞쪽의 랜덤한 위치에 넣으며 섞는 방식이다.

<알고리즘의 단계>

  1. 배열의 마지막 요소부터 시작해서 처음 요소까지 거꾸로 이동하며 진행
  2. 현재 요소와 앞쪽의 있는 랜덤한 요소를 서로 교환
  3. 이렇게 하면 배열의 모든 요소가 랜덤하게 섞이게 됨

이 방식을 사용하면? 배열의 각 요소가 동일한 확률로 섞일 수 있게 보장!

-> 근데 이걸 내 코드에 어떻게 적용하지...?
알고리즘 단계를 차근차근 코드로 풀어냈다...ㅎㅎ

// 피셔-예이츠 알고리즘을 이용해 배열을 무작위로 섞음
const fisherYatesShuffle = (array) => {
    // i는 현재 요소의 위치, 배열의 마지막 요소부터 시작해 앞으로 이동
    // array.length - 1은 배열의 마지막 인덱스를 뜻함
    // i-- 로 1씩 감소시킴, 이러면 앞쪽으로 한 칸씩 이동하게 됨
    for (let i = array.length - 1; i > 0; i--) {
      // 현재 위치 i와 그 앞의 인덱스들 중 하나를 무작위로 선택해서 j에 저장하는 것
      // i + 1을 곱하는 이유는 i까지의 인덱스 중 하나를 선택하기 위해서
      // Math.floor()는 소수점을 버리고 아래쪽으로 가장 가까운 정수로 만듦
      const j = Math.floor(Math.random() * (i + 1));
      // array[i]와 array[j]를 교환
      [array[i], array[j]] = [array[j], array[i]];
    }
    return array;
  };

  export default fisherYatesShuffle;

3️⃣ 소수점 처리 방법

시간을 측정하는 타이머 기능을 구현하면서, 소수점 자릿수 처리가 필요했다. 소수점 두 자리까지 표시를 해야 하는데 어떻게 해야할지 몰라서 찾아보니 toFixed() 메소드를 사용하면 간단하게 소수점 아래 자릿수를 제한할 수 있었다. toFixed(2) 이렇게 하면 소수점 두 자리까지만 표시해주기 때문에 예를 들어 0.12345 같은 값도 0.12로 깔끔하게 보여줄 수 있다. 👍 근데 여기서 중요한 건 toFixed()가 문자열을 반환한다는 것,,, 처음에 바로 숫자 연산에 사용하려다가 에러가 나서 알게 됐다. parseFloat()를 통해 다시 숫자로 바꿔주어 사용했다.

4️⃣ createPortal로 모달 구현

이번 과제를 통해 처음 들어보는 기능이었다... createPortal은 평소 컴포넌트가 렌더되는 DOM 계층과 다른 위치에 컴포넌트를 렌더링할 수 있도록 해주는 기능이다. 평소엔 모든 컴포넌트가 부모 요소를 따라 렌더되지만 모달처럼 전체 화면을 덮거나 앱의 최상단에서 보여야 하는 UI는 계층을 벗어나야 할 때가 많다. 이럴 때 createPortal이 매우 유용하다고 함!

5️⃣ styled-components에서 $ 접두사

나중에 등장하는 숫자 배열은 색을 다르게 하기 위해 사용했다! 이 기능을 구현하면서 $ 접두사의 역할에 대해 개념을 다시 정리한 것 같아 정리해보려고 한다.

$ 접두사의 역할
styled-components는 기본적으로 모든 prop을 DOM에 전달하려고 한다. 그런데 prop 중에는 스타일 컴포넌트 내에서만 사용하고 실제 DOM에는 전달하지 않고 싶은 경우가 있다. 이번 코드를 예로 들자면 isNew와 같은 prop은 스타일링을 위해 필요하지만 DOM 요소에 전달되면 불필요한 속성이기 때문에 경고가 발생한다.

이럴 때 styled-components에서 $ 접두사를 붙인 prop을 사용하면 이 prop은 스타일 컴포넌트 내에서만 사용되고 DOM에 전달되지 않는다.
-> $isNew와 $isVisible과 같은 prop은 DOM 요소에 전달되지 않으면서 스타일링에만 영향을 미치게 된다

6️⃣ 스프레드 연산자와 얕은 복사

이번 과제를 하면서 자바스크립트의 스프레드 연산자와 얕은 복사 개념을 다시 알게 되었다. (물론 깊은 복사, 얕은 복사... 세미나 때 배웠지만 복습을 98번 정도 해야겠다...) 처음엔 단순히 배열을 복사하는 줄만 알았는데, 이 과정에 중요한 개념들이 포함되어 있어서 정리하게 됐다.

🤔 스프레드 연산자?
스프레드 연산자(...)는 배열이나 객체의 요소를 개별적으로 분리하여 새로운 배열이나 객체로 확장해주는 기능이다. const newArray = [...originalArray]처럼 사용하면, originalArray의 요소들이 새로운 배열 newArray로 복사되는 것이다. 여기서 중요한 점은, 이 복사가 얕은 복사로 이루어진다는 것!

-> 얕은 복사 vs 깊은 복사
배열이나 객체를 복사할 때, 얕은 복사는 참조값만을 복사해 새로운 배열이나 객체를 만드는데, 이 과정에서 중첩된 객체나 배열의 참조는 여전히 원본을 가리킨다는 차이점이 있다. 즉, 최상위 요소는 새로운 복사본이지만 내부 중첩된 데이터는 원본을 공유하게 된다. 이와 달리 깊은 복사는 중첩된 구조까지 모두 새로운 메모리 공간에 복사하는 방식이다. 이번 코드에서는 중첩된 구조가 필요하지 않아 스프레드 연산자를 이용한 얕은 복사를 사용했다.

7️⃣ 함수형 업데이트와 불변성 유지

리액트에서 함수형 업데이트와 불변성 유지가 중요하다는 걸 어렴풋이는 알고 있었는데 이번 과제를 하면서 개념을 확실히 알았다. (확실히는 아닐수도)

함수형 업데이트
리액트의 상태 업데이트는 비동기로 작동하기 때문에 현재 상태를 기반으로 새 상태를 업데이트할 때는 함수형 업데이트 방식을 사용하는 것이 안전하다고 한다,,, 함수형 업데이트를 사용하면 setState에 이전 상태를 매개변수로 받아와 그 값을 기반으로 새로운 상태를 설정할 수 있다. 이번엔 setIsNew에 함수형 업데이트를 적용해 prevIsNew 값을 안전하게 참조하여 업데이트할 수 있었다.

불변성 유지
리액트에서는 상태를 직접 수정하지 않고 불변성을 유지하며 새로운 상태를 만들어 업데이트하는 것이 중요하다는 것도 깨달았다,,!! 배열에서 특정 인덱스의 값을 변경할 때, 배열을 직접 수정하는 대신 스프레드 연산자를 통해 복사본을 만들고 그 배열에서 필요한 값을 업데이트하여 반환했다. -> 리액트는 상태 변경을 감지하고 컴포넌트를 효율적으로 다시 렌더링할 수 있음.


❓ 구현 과정에서의 어려웠던/고민했던 부분

진심 너무 어려워서 헤매면서 했더니 뭔가 누가 봐도 이건 최적의 방법이 아니다 ^_^ 가 느껴지는,,,, 최대한 컴포넌트랑 로직을 분리하려고 고민을 많이 했고 이번에는 useEffect를 아예 사용하지 않으려고 노력했다.... ㅠㅠ 그리고 사실 새로 알게된 점 = 어려웠던, 고민했던 점이다...!!

💾 localStorage / 데이터 관리

게임이 끝날 때마다 기록을 로컬 스토리지에 저장해야 했는데 새로운 기록을 추가하면서 기존 기록을 덮어쓰지 않는 방법을 고민했다. 기존 기록을 유지하면서 새로운 데이터를 추가하려면 먼저 기존 기록을 불러온 후 새로운 기록을 배열에 추가하고, 최종적으로 업데이트된 배열을 다시 저장하는 방식이 필요했다.

-> 로컬 스토리지에서 기존 기록을 불러와 JSON 형식으로 변환하고 새로운 기록을 배열에 추가한 뒤 정렬된 배열을 다시 로컬 스토리지에 저장했다.

🥲 소요 시간

  • 3days

🖼️ 구현 결과물

2024-11-07.3.31.01.mov

Copy link

@constantly-dev constantly-dev left a comment

Choose a reason for hiding this comment

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

과제 수고많으셨습니다! 코드 보면서 utils도 그렇고 custom hook도 정말 잘 분리가 되어있어서 코드 보기가 너무 편했던 것 같아요. 보면서 저의 코드를 반성하게 되는 시간이었습니다..!
중간에 약간의 의견을 내는 것을 제외하면 전체적으로 완성도가 많이 있었던 코드였던 것 같아요! 대신 함수의 분리나 상태 관리 코드에 대해서 조금 더 최적화적인 코드는 무엇일지 같이 생각해보면 좋을 것 같습니다 :)

그리고 useEffect에 대해서 고민하신 점이 정말 인상 깊었습니다. 단순히 지양하지 말자가 아니라 왜 쓰면 안될지, 그런 점에서 어떻게 useEffect를 대체할 지 고민하신 부분이 보여서 정말 열심히 하신 것 같다는 생각이 듭니다..!!!

PR에 로컬스토리지 관련된 내용은 JS 불변성 원칙에 대해서 작성해주신 것 같아서 관련 공부도 더 같이 해보면 좋을 것 같아요~ 과제 수고하셨습니다 ⭐

Comment on lines +33 to +46
setNumbers((prev) =>
prev.map((num, idx) => {
if (num === number) {
// 새로 추가된 숫자 위치 표시
setIsNew((prevIsNew) => {
const newIsNew = [...prevIsNew]; // 현재 isNew 배열을 새로운 배열로 복사
newIsNew[idx] = true; // 클릭한 셀의 인덱스에 해당하는 위치를 true로 설정
return newIsNew; // 업데이트된 배열을 반환하여 isNew 상태를 갱신
});
return nextNumber || null; // 다음 숫자가 없으면 null로 대체
}
return num;
})
);

Choose a reason for hiding this comment

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

p3 : 해당 부분 로직이 return 문이 3번 중첩되어있어서 가독성 측면 그리고 새로운 isNew라는 상태에 대한 역할을 더 확실히 한다는 이유로 분리를 조금 더 하면 좋을 것 같아요!

const updateIsNew = (index) => {
  setIsNew((prevIsNew) => {
    const newIsNew = [...prevIsNew];
    newIsNew[index] = true;
    return newIsNew;
  });
};

따로 이렇게 분리하고

 if (num === number) {
        updateIsNew(idx); 
        return nextNumber || null; 
      }

내부에서 이렇게 써도 되지 않을까 생각합니다!

Copy link
Member

Choose a reason for hiding this comment

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

p3: 저도 해당 의견에 동의합니다. setIsNew 부분을 따로 함수로 분리하여 사용하는 것이 가독성도 그렇고 로직을 이해하는데 훨씬 도움이 될거같아요.

Comment on lines +8 to +11
const generateShuffledNumbers = (start, count) => {
const numbers = Array.from({ length: count }, (_, i) => i + start);
return fisherYatesShuffle(numbers);
};

Choose a reason for hiding this comment

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

P2 : generateShuffledNumbers 함수에서 매개변수를 start, count로 따로 받아와도 괜찮지만 이렇게 되면 generateShuffledNumbers를 호출하는 곳에서 매번 initialNumbersRangeindex 0과 1을 나눠서 두 개를 다 줘야 해요!

그래서 만약

const generateShuffledNumbers = ( [start, count] ) => {
    const numbers = Array.from({ length: count }, (_, i) => i + start);
    return fisherYatesShuffle(numbers);
  };

이렇게 받는 곳에서 배열 구조 분해 할당으로 받게 되면,

const initial = generateShuffledNumbers(initialNumbersRange);
const remaining = generateShuffledNumbers(remainingNumbersRange).sort(
    (a, b) => a - b);

이렇게 사용하는 곳에서 두 개의 값을 따로 안 줘도 가능할 것 같아요!

Comment on lines +14 to +18
const gameData = {
timestamp: new Date().toISOString(), // 현재 시각
level: level, // 현재 레벨
playTime: formattedTime // 플레이 시간
};

Choose a reason for hiding this comment

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

p2 : 이미 알고 계셨을 수도 있지만 객체에서 key와 value 값이 같다면 하나만 작성해도 되니 해당 코드를

 const gameData = {
      timestamp: new Date().toISOString(), // 현재 시각
      level, // 이 부분!!
      playTime: formattedTime // 플레이 시간
};

이렇게 줄여도 괜찮을 것 같아요. 물론 같지 않은 부분은 그대로!

Comment on lines +9 to +15
return savedRankings.sort((a, b) => {
// 레벨 기준 내림차순 정렬
if (b.level !== a.level) {
return b.level - a.level;
}
// 레벨이 같으면 playTime 기준 오름차순 정렬
return parseFloat(a.playTime) - parseFloat(b.playTime);

Choose a reason for hiding this comment

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

p3 : 함수 네이밍 그대로 loadRankings라는 의미라면 load라는 역할에 더 충실하면 더 좋을 것 같아요! 그렇다면 정렬해주는 함수를 sortRankings로 따로 분리해서 loadRankings에서 이걸 그대로 사용하는 것도 좋은 방법일 것 같습니다! 지금도 코드 좋지만 분리하면 더 깔끔해질 것 같아요!

Comment on lines +9 to +25
const startTimer = () => {
if (!isRunning) { // 타이머가 멈춰 있을 때만 실행되게
setIsRunning(true); // 실행 상태로 변경
// timerRef.current를 통해 setInterval이 반환하는 ID 값을 timerRef에 저장
timerRef.current = setInterval(() => {
setTime((prevTime) => prevTime + 0.01); // 이전 값에 0.01을 더해 업데이트
}, 10); // 10ms마다 한 번 실행
}
};

const stopTimer = () => {
if (isRunning) {
clearInterval(timerRef.current); // 타이머 멈추기
timerRef.current = null; //타이머 ID 초기화시킴
setIsRunning(false); // 실행 상태를 멈춤으로 변경
}
};

Choose a reason for hiding this comment

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

p3 : 이렇게 하면 useEffect를 쓰지 않고 timer에 대한 로직을 이벤트만으로 조작할 수 있겠군요..!!! 저는 무작정 isActiveTimer로 타이머가 실행 가능할지 state로 체크한 다음 useEffect내에서 timer를 실행하도록 했는데 분리도 깔끔하고 좋은 방법인 것 같습니다!

Copy link
Member

Choose a reason for hiding this comment

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

p3: 저도 useRef를 이용하여 타이머 로직을 구현하였는데 useEffect를 최대한 쓰지 않으려는 노력이 보기 좋습니다. 진혁님 코드리뷰에도 달았는데 해당 내용 한번 읽어보면 좋을것 같아서 남깁니다.

useEffect와 useRef를 사용하는 것은 어떤 차이점이 있나?

간단한 타이머나 상태 변화 감지가 주요 목적일 때는 useEffect가 더 간결하고 리액트의 라이프사이클과 잘 맞아떨어지기 때문에 useEffect가 더 적합하다고 해요. 예를 들어, 특정 상태가 변경될 때마다 타이머를 시작하거나 중단하고 싶을 때는 useEffect를 사용한다고 합니다.

복잡한 타이머 제어나 성능이 중요한 경우(ex: 타이머를 반복적으로 시작/정지하거나, 다른 상태 변화와 무관하게 타이머만 따로 관리하고 싶을 때) timerRef를 사용하는 것이 좋다고 해요. 이렇게 하면 렌더링과 관계없이 타이머를 관리할 수 있어 성능이 중요한 애플리케이션에서 유리하겠죠.

복잡한 타이머 관리나 빈번한 렌더링이 부담스러울 경우에는 timerRef를 사용하고, 그렇지 않다면 간단하게 useEffect로 구현하는 것이 일반적으로 더 효율적이라고 하네요!

Comment on lines +33 to +38
{rankings.map((record, index) => (
<tr key={index}>
<td>{new Date(record.timestamp).toLocaleString()}</td>
<td>Level {record.level}</td>
<td>{record.playTime} 초</td>
</tr>

Choose a reason for hiding this comment

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

p3 : map() 에서도 구조 분해 할당을 쓸 수 있습니다! 아래처럼! 이건 당연히 필수가 아니고 각자마다 편한 방법으로 사용하면 좋을 것 같아서 추가적으로 작성해봤습니다 :)

 {rankings.map(({ timestamp, level, playTime }, index) => (
          <tr rankings.map(({ timestamp, level, playTime }, index) => (
          <tr key={index}>
            <td>{new Date(timestamp).toLocaleString()}</td>
            <td>Level {level}</td>
            <td>{playTime}</td>
          </tr>
        ))}

Comment on lines +45 to +53
RankingTable.propTypes = {
rankings: PropTypes.arrayOf(
PropTypes.shape({
timestamp: PropTypes.string.isRequired,
level: PropTypes.number.isRequired,
playTime: PropTypes.string.isRequired,
})
).isRequired,
};

Choose a reason for hiding this comment

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

p3 : 헉 propTypes까지.. 저는 TS로 안해서 불편한 오류를 그냥 eslint를 지워서 진행했는데, JS 환경에서도 타입 지정을 해서 더욱 데이터를 파악하기 쉽도록 개발을 하셨네요! 👍


const GameBoardContainer = styled.div`
display: grid;
grid-template-columns: repeat(${(props) => props.gridSize}, 1fr);

Choose a reason for hiding this comment

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

p3: theme 사용하실 때 잘하신 것 같은데 이 부분도 구조 분해 할당으로 props 키워드 없이 작성할 수 있겠네요!

Comment on lines +35 to +41
{numbers.map((number, index) => (
<Cell
key={index}
$isNew={isNew[index]}
$isVisible={number !== null}
onClick={() => number !== null && onCellClick(number)}
>

Choose a reason for hiding this comment

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

p3: 중복되는 number !== null 로직을 따로 빼서 변수화 해두면 가독성도 좋고 중복도 방지할 수 있을 것 같습니다!

      {numbers.map((number, index) => {
        const isVisible = number !== null; // 불필요한 조건 중복 방지
        return (
          <Cell
            key={index}
            $isNew={isNew[index]}
            $isVisible={isVisible} // 여기
            onClick={() => isVisible && onCellClick(number)} // 여기
          >

Comment on lines +29 to +41
const handleCellClick = (number) => {
if (number === nextNumber) {
if (number === 1) {
startTimer();
}
if (number === initialRange[1] + remainingRange[1]) { // 레벨별 마지막 숫자에 맞춰 종료하기
stopTimer();
endGame(time);
return;
}
updateNumbers(number);
}
};

Choose a reason for hiding this comment

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

p2 : numbernextNumber랑 같은지 비교해서 같으면 안에 로직을 실행하도록 해도 되지만, 반대로 아예 다르면 return을 하는 early return 패턴을 쓰면 더 코드가 깔끔해질 것 같습니다! 이렇게 early return을 하면

가독성 측면에서 조건에 맞지 않는 경우 바로 함수 실행을 중단(return)하기 때문에, 나머지 로직 이해하는 데 훨씬 좋을 것 같습니다. 그리고 조건에 안 맞으면 반환 되니까 함수 내부 나머지 코드가 실행되지 않아서 성능 상의 이점도 있을 것 같아요!

const handleCellClick = (number) => {
  if (number !== nextNumber) return;

  if (number === 1) startTimer();
  
  if (number === getLastNumber()) {
    stopTimer();
    endGame(time);
    return;
  }

  updateNumbers(number);
};

const getLastNumber = () => initialRange[1] + remainingRange[1];
// 이건 가독성을 위해 분리해도 괜찮을 것 같다는! (선택입니다!!!)

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
Member

@KIMGEONHWI KIMGEONHWI left a comment

Choose a reason for hiding this comment

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

커스텀 훅도 적극 사용하고, 컴포넌트 분리도 납득이 가게 잘한것 같아요! 특히, 피셔 예이츠 알고리즘은 처음들어보는데 pr에 작성해주신 내용을 통해서 배열의 각 요소가 동일한 확률로 섞일 수 있게 보장해줄 수 있는 것으로 보아 의심없이 랜덤 메소드를 사용한 저를 반성하게 되었습니다. 한가지 피드백을 하자면, 현재 useShuffledNumbers커스텀훅에 너무 많은 로직이 섞여있는 듯한 느낌이듭니다. 조금더 명확한 역할로 분리해보면 좋은 경험이 될 것 같아요!
3주차 과제도 고생 많았어요😄

Comment on lines +17 to +21
const RightSection = styled.div`
display: flex;
gap: 1rem;
align-items: center;
`;
Copy link
Member

Choose a reason for hiding this comment

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

p1: 현재 게임이 시작되어서 타이머가 실행되면 타이머 숫자가 바뀜에 따라 너비가 달라지기 때문에 RightSection영역 내부 요소인 LevelSelectTimer 부분이 계속해서 좌우로 흔들려서?보이는 사용자 경험 측면에서 좋지 않은 문제가 발생하고 있는데요. 다음과 같이 RightSection에 넉넉한 width값을 지정해주면 해당 이슈 해결할 수 있을거 같아요!

Suggested change
const RightSection = styled.div`
display: flex;
gap: 1rem;
align-items: center;
`;
const RightSection = styled.div`
display: flex;
width: 10rem;
gap: 1rem;
align-items: center;
`;

Comment on lines +94 to +100
Header.propTypes = {
selectedMenu: PropTypes.string.isRequired,
setSelectedMenu: PropTypes.func.isRequired,
time: PropTypes.number.isRequired,
isGameStarted: PropTypes.bool.isRequired,
onLevelChange: PropTypes.func.isRequired,
};
Copy link
Member

Choose a reason for hiding this comment

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

p3: eslint를 오류 때문에 다음과 같이 PropTypes을 지정해주어서 진행하여 타입 안전성을 높인 부분은 좋은 방향이오나, 타입을 일일이 지정해줘야한다면 차라리 타입스크립트를 사용하는 것이 좋지 않을까?라는 생각이 듭니다. 사실 그렇게되면 자바스크립트를 사용하는 이유중 하나인 타입 지정 없이 빠르게 개발할 수 있고 유연성이라는 장점이 많이 퇴색된다고 느껴 저는 해당 오류를 일으키는 eslint조건을 지워서 진행하였습니다. 제 의견이니 참고만 해주세요!

Copy link
Member

Choose a reason for hiding this comment

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

p5: theme 정의해서 사용한거 아주 좋습니다👍

Comment on lines +18 to +23
// levelSettings를 GamePage 컴포넌트 내부에 둘지, 분리할지 고민이에요
const levelSettings = {
1: { initialRange: [1, 9], remainingRange: [10, 9], gridSize: 3 },
2: { initialRange: [1, 16], remainingRange: [17, 16], gridSize: 4 },
3: { initialRange: [1, 25], remainingRange: [26, 25], gridSize: 5 },
};
Copy link
Member

Choose a reason for hiding this comment

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

p3: levelSettings객체를 GamePage 컴포넌트 내부에 두는 것도 일리가 있지만, 저라면 분리해서 사용할 것 같아요! 근거는 다음과 같습니다.

  1. GamePage 컴포넌트에서 상태와 로직만 유지하고, 세팅 관련 부분은 따로 분리하면 코드 가독성을 높일 수 있을거 같아요.
  2. 추후에 levelSettings 내부 조건들을 수정해야되는 경우 분리하여서 사용하였을 때 관리가 더 쉬울 것 같습니다.
  3. 가장 중요하게 생각하는 부분 중 하나인 관심사 분리 측면에서 levelSettings를 분리하면 컴포넌트의 역할을 더욱 명확히 할 수 있을거 같아요!

Comment on lines +9 to +25
const startTimer = () => {
if (!isRunning) { // 타이머가 멈춰 있을 때만 실행되게
setIsRunning(true); // 실행 상태로 변경
// timerRef.current를 통해 setInterval이 반환하는 ID 값을 timerRef에 저장
timerRef.current = setInterval(() => {
setTime((prevTime) => prevTime + 0.01); // 이전 값에 0.01을 더해 업데이트
}, 10); // 10ms마다 한 번 실행
}
};

const stopTimer = () => {
if (isRunning) {
clearInterval(timerRef.current); // 타이머 멈추기
timerRef.current = null; //타이머 ID 초기화시킴
setIsRunning(false); // 실행 상태를 멈춤으로 변경
}
};
Copy link
Member

Choose a reason for hiding this comment

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

p3: 저도 useRef를 이용하여 타이머 로직을 구현하였는데 useEffect를 최대한 쓰지 않으려는 노력이 보기 좋습니다. 진혁님 코드리뷰에도 달았는데 해당 내용 한번 읽어보면 좋을것 같아서 남깁니다.

useEffect와 useRef를 사용하는 것은 어떤 차이점이 있나?

간단한 타이머나 상태 변화 감지가 주요 목적일 때는 useEffect가 더 간결하고 리액트의 라이프사이클과 잘 맞아떨어지기 때문에 useEffect가 더 적합하다고 해요. 예를 들어, 특정 상태가 변경될 때마다 타이머를 시작하거나 중단하고 싶을 때는 useEffect를 사용한다고 합니다.

복잡한 타이머 제어나 성능이 중요한 경우(ex: 타이머를 반복적으로 시작/정지하거나, 다른 상태 변화와 무관하게 타이머만 따로 관리하고 싶을 때) timerRef를 사용하는 것이 좋다고 해요. 이렇게 하면 렌더링과 관계없이 타이머를 관리할 수 있어 성능이 중요한 애플리케이션에서 유리하겠죠.

복잡한 타이머 관리나 빈번한 렌더링이 부담스러울 경우에는 timerRef를 사용하고, 그렇지 않다면 간단하게 useEffect로 구현하는 것이 일반적으로 더 효율적이라고 하네요!

Comment on lines +29 to +41
const handleCellClick = (number) => {
if (number === nextNumber) {
if (number === 1) {
startTimer();
}
if (number === initialRange[1] + remainingRange[1]) { // 레벨별 마지막 숫자에 맞춰 종료하기
stopTimer();
endGame(time);
return;
}
updateNumbers(number);
}
};
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 +33 to +46
setNumbers((prev) =>
prev.map((num, idx) => {
if (num === number) {
// 새로 추가된 숫자 위치 표시
setIsNew((prevIsNew) => {
const newIsNew = [...prevIsNew]; // 현재 isNew 배열을 새로운 배열로 복사
newIsNew[idx] = true; // 클릭한 셀의 인덱스에 해당하는 위치를 true로 설정
return newIsNew; // 업데이트된 배열을 반환하여 isNew 상태를 갱신
});
return nextNumber || null; // 다음 숫자가 없으면 null로 대체
}
return num;
})
);
Copy link
Member

Choose a reason for hiding this comment

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

p3: 저도 해당 의견에 동의합니다. setIsNew 부분을 따로 함수로 분리하여 사용하는 것이 가독성도 그렇고 로직을 이해하는데 훨씬 도움이 될거같아요.

Comment on lines +54 to +60
const resetNumbers = () => {
const initial = generateShuffledNumbers(initialNumbersRange[0], initialNumbersRange[1]);
const remaining = generateShuffledNumbers(remainingNumbersRange[0], remainingNumbersRange[1]).sort((a, b) => a - b);
setNumbers(initial);
setRemainingNumbers(remaining);
setIsNew(Array(initial.length).fill(false)); // 초기화 시 isNew 배열도 초기화
};
Copy link
Member

Choose a reason for hiding this comment

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

p3: resetNumbers 함수에서 초기 숫자와 남은 숫자 배열을 설정하는 부분을 별도의 함수로 분리하면 가독성과 관리 측면에서 더 좋을거 같아요. 다음과 같이, initializeNumbers와 같은 함수를 만들어 사용할 수 있을거 같아요.

Suggested change
const resetNumbers = () => {
const initial = generateShuffledNumbers(initialNumbersRange[0], initialNumbersRange[1]);
const remaining = generateShuffledNumbers(remainingNumbersRange[0], remainingNumbersRange[1]).sort((a, b) => a - b);
setNumbers(initial);
setRemainingNumbers(remaining);
setIsNew(Array(initial.length).fill(false)); // 초기화 시 isNew 배열도 초기화
};
const initializeNumbers = () => ({
initial: generateShuffledNumbers(initialNumbersRange[0], initialNumbersRange[1]),
remaining: generateShuffledNumbers(remainingNumbersRange[0], remainingNumbersRange[1]).sort((a, b) => a - b),
});
const resetNumbers = () => {
const { initial, remaining } = initializeNumbers();
setNumbers(initial);
setRemainingNumbers(remaining);
setIsNew(Array(initial.length).fill(false));
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants