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

[REFACTOR] review & refactoring requests #1

Open
crown-3 opened this issue Jul 18, 2024 · 0 comments
Open

[REFACTOR] review & refactoring requests #1

crown-3 opened this issue Jul 18, 2024 · 0 comments

Comments

@crown-3
Copy link

crown-3 commented Jul 18, 2024

리뷰

스크린샷 2024-07-19 01 11 51
  • 디자인 직접 하신건가요..?? 진짜 대박입니다 👍
  • 버튼 누르고 뗄 때 누르는 애니메이션 넣은 부분에서 감동받았습니다 😭
  • grid, flex를 이용해서 레이아웃을 잘 배치하신 것 같아요. 또 styled-components 사용에 많이 익숙해지신 것 같네요!

개선점

1. eval 사용 지양

eval is evil

  • 계산을 구현하신 핵심 로직에서 eval 함수를 사용하신 걸 확인했어요.
const getResult = () => {
    let replace_str = calc.replace(/×/gi, "*").replace(/÷/gi, "/");

    if (isNaN(eval(replace_str))) {
      setCalc("");
    } else if (eval(replace_str) == Infinity) {
      alert("0으로 나눌수 없습니다.");
      setCalc("");
      return false;
    } else {
      setCalc((prev) => eval(replace_str));
    }
  };
  • GPT나 구글에게 eval을 쓰면 안 되는 이유를 물어보면 확인하실 수 있을 거에요. 대강 말씀해드리면, 코드 성능을 악화하고, 큰 보안 취약점을 유발할 수 있어요.
  • 그리고 eval 함수가 사실상 계산 로직을 거의 대신하고 있는 것이나 마찬가지이기 때문에, eval을 사용해서 구현하는 것이 아니라 직접 계산 로직을 구현해보시면 좋을 것 같아요.

2. Typescript 적용

  • 기존에 JS 레포에서 개발하다가 여기로 로직을 옮겨 오신 것으로 알고 있어요.

  • 하지만 바꿀 게 그렇게 많아 보이지는 않아요! 빨간 줄이 뜨는 부분에 타입을 잘 달아 주면 돼요.

  • 다음부터 코드를 짜실 때는, 제가 드린 Typescript 강의를 기반으로 리액트에 Typescript를 적용해보시면 됩니다.

  • 예를 들어, 여기 있는 getNum 함수에 타입을 달아 볼게요.

const getNum = (e) => {
    setCalc((prev) => prev + e.target.value);
    setOperCheck(true);
  };
  • 타입스크립트가 다음과 같은 오류가 있다고 알려주고 있어요. 함수 인자의 타입을 명시해 주라는 내용이에요.
스크린샷 2024-07-19 01 25 18
  • getNum 함수는 e를 인자로 받고 있죠? 그런데 이 e의 타입을 어떻게 알 수 있을까요?
  • 이걸 알아보기 위해 버튼의 onClick이 눌렸을 때 실행되는 화살표 함수를 하나 만들어 봅시다.
<Button onClick={(e) => {}}/>
  • 그리고 이 e의 타입을 살펴보면, 타입이 React.MouseEvent<HTMLButtonElement, MouseEvent>인 것을 확인할 수 있어요.
스크린샷 2024-07-19 01 33 07
  • 이걸 그대로 함수에 적용시켜주면 되겠죠?
const getNum = (e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => {
    setCalc((prev) => prev + e.target.value);
    setOperCheck(true);
  };
  • getNum이 실행될 때 onClick 안에서 실행될테니까, onClick이 주는 인자가 어떤 타입인지 알아본 뒤 이를 함수에 적용시킨 작업을 해 준 거에요!!

  • 그런데 이걸 적용시키고 보니 다음과 같은 에러가 발생해요.

스크린샷 2024-07-19 01 36 00
  • 내가 알 수 없는 종류의 에러를 해결하려면 에러 메시지를 구글링해보는게 최선이에요.
스크린샷 2024-07-19 01 37 36
  • 그러면 저와 같은 문제를 겪은 개발자들이 대신 문제를 해결한 과정을 알려준 수많은 포스트들을 볼 수 있어요.

  • 제가 방금 타입을 명시함으로써 생긴 에러는 태현님께서 직접 해결해보시면 좋을 것 같아요! 그리고 이와 같은 방식으로 전체 프로젝트에 타입을 적용시켜보면 좋을 것 같아요.

  • 결국 좋은 개발자는 문제 상황을 구글링을 통해 찾아보고, 좋은 해결책을 찾아 스스로 해결해 나갈 수 있는 개발자니까요.

3. eslint가 고치라고 하는 것들 고쳐보기

  • eslint는 코드를 보다 예쁘고 알아보기 쉽게, 그리고 오류가 발생할 수 있는 가능성이 적게 만들어주는 좋은 툴이에요. 인포팀 프론트엔드 템플릿에 자동으로 탑재되어 있죠.
  • 태현님의 코드를 보고 eslint가 몇 개의 오류를 뱉어요. 대표적으로 다음과 같이 let 말고 const를 사용하라는 것이나, 쓰지 않는 변수를 삭제하라는 것 등이 있네요.
스크린샷 2024-07-19 01 43 57 스크린샷 2024-07-19 01 44 06
  • 간단히 해결할 수 있는 문제들이네요.

  • 이외에도 고칠 게 조금 있어 보이긴 해요! 일단 이번 이슈에 제가 제시한 개선점들을 해결해 보시면 좋을 것 같아요.

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

No branches or pull requests

1 participant