[PR 코드리뷰] 우아한테크코스 프론트엔드 레벨1 - 지하철 노선도 미션

 

우아한테크코스 프론트엔드 과정 레벨1 <지하철 노선도 미션>을 수행하며 우아한형제들의 @HyunaKwon 님께 리뷰를 받을 수 있었다. 코드를 꼼꼼하게 리뷰해주셨다. 전반적으로 에러핸들링이 부족하다는 피드백을 주셔서 페어와 함께 에러와 에러핸들링을 어떻게 풀어나갈지 고민해볼 수 있어서 좋았다. 👍👍👍

 

미션 1단계 피드백

👉 PR 바로가기

1. 예외처리는 꼼꼼하게

전체적으로 예외 처리가 부족해 보인다. generateInputValidator와 hasPropertyValue에서 throw해주는 Error를 아무데서도 처리하지 않고있다. try-catch 문을 추가하는 연습을 추천한다.

에러 처리는 중구난방으로 하지 않고 통일성을 갖추어 사용하는 것이 좋다. '처리 후 early return' 방식 또는 'error throwing' 방식 중 어떤 방식을 언제 사용하면 좋을지 생각해보자. 우선, 처리 후 early return의 경우에는 보통 사용자에게 피드백을 준 후 return 을 하는 경우이다. 그런데 만약 api 를 모아둔 파일에서 이런 일을 한다면, api 가 너무 많은 일을 하게 된다. 사용자 피드백을 화면에 노출하는 렌더링 역할까지 하는 셈이기 때문이다. 이런 경우에는 Error 를 던지거나 에러메세지를 리턴해 api를 사용하는 사용처에서 에러 핸들링을 하는 것이 좋겠다.
한편, 컴포넌트 단에서 예외처리를 하는 경우에는 early return 을 해도 된다. 컴포넌트가 다 처리를 해도 되기 때문이다.

비개발자 직군의 사람들과 이야기할 때는 '버그', 개발자끼리 소통할 때는 '예외 처리가 되어있지 않다'고 말하는 편이다. 예측 불가능한 에러는 보통의 최상위 컴포넌트에서 예외처리를 추가해 한번 감싸는 구조로 처리한다. 곧 배우게 될 리액트에서는 에러바운더리 라는 것도 이용하게 된다.

에러 리포팅은 사용자에게 보내는 메세지와 개발자가 참고할 로그를 구분해서 작성하는 것이 좋다. 배달의 민족에서도 사용자에게는 좀더 정제된 메세지를, 개발자가 보는 로그에는 있는 그대로의 메세지를 남기는 편이다.

2. 역할이 너무 많은 라우터

라우터는 path 변경과 그에 따른 component 선택만해도 충분할 것 같은데, 렌더 관련까지 하니 router 가 너무 커졌다.

3. 추상화 레벨을 고려하자

getValidityMessage 는 getEmailValidityMessage, getNameValidityMessage, getPasswordValidityMessage 세 가지 중 한가지 함수이다. getEmailValidityMessage가 아닐 경우에는 promise 가 아닌데도 await 를 사용하고 있다. promise를 반환하는 함수와 그렇지 않은 함수를 함께 추상화했다면 추상화 레벨이 다른 함수들을 잘못 추상화한 것으로 보여진다.

const customValidityMessage = await getValidityMessage($input);

4. 기타 피드백

views 폴더안에 뷰와는 관련없는 내용이 있어 분리하는 것이 좋겠다. (views를 components로 변경함!)

코드 컨벤션같은 경우는 eslintrc 나 prettierrc 로도 충분히 맞출 수 있다. 에디터의 설정내용은 개인 파일이라 .vscode/setting.json은 gitignore 에 추가하는 것이 일반적이다. 

 

 

미션 2, 3단계 피드백

👉 PR 바로가기

1. on-* 네이밍은 이벤트 핸들러에만

onClickButton()과 같이 이벤트 핸들러 내부에서 호출하는 함수는 on-* 접두사 없이 바로 동사로 시작하는 이름을 갖는 것이 좋다. 그 함수들은 이벤트 핸들러가 아니기 때문이다. 예를 들어 'onClickEditButton'는 'changeToEditMode'로 바꾸는 것이 좋겠다.

 

 

다른 크루의 리뷰 메모 👀

피드백 내용 앞의 번호는 순번이 아니라 PR 번호이다. 

 

아키텍쳐/패턴

#06 View를 알고 있는 Component에서 바료 API 요청을 보내게 되면, HTTP Status를 처리하는 책임까지 View 단에서 갖게된다. View에서 필요한 것은 그저 화면에 뿌릴 데이터뿐이다. 따라서 컴포넌트와 API 사이에 하나의 레이어(서비스)를 두는 것을 좋겠다. 컴포넌트와 API 사이의 레이어를 두는 것은 나중에 리액트와 상태 라이브러리를 함께 사용할 때, 그 구조를 이해하는데에도 도움이 될 것이다.

#11 render 함수가 비동기가 되는 것은 좋지 않다. 대부분의 "비동기"작업은 "로직"이고, render 함수가 비동기 함수가 되었다는 뜻은 render 함수와 "로직"이 강결합 되었다는 의미로 볼 수 있다. "로직"과 "렌더링"을 분리해보자.

 

함수/클래스

#05 loginAPI, signupAPI는 서로 body 만 다르고 중복되는 내용이 많다. 이 부분을 추상화해서 fetch를 사용하면 더 깔끔하게 사용할 수 있겠다.

#06 의도적으로 Error를 throw하는 경우에는 함수 내부가 아니라 해당 함수를 호출한 쪽에서 catch할 때 사용한다. 로그를 남길 목적으로 내부에서 catch 하더라도 다시 catch 구문 내에서 throw를 해주어야 외부에서 catch 하여 적절한 처리를 할 수 있다.

checkEmail(email).then(...).catch(e => {
  // throw 된것은 여기서 처리
})

#07 기본값은 설정하는 것이 좋다. 기본값을 설정하는 것은 primitive type을 설정해주는 효과가 있다.  undefined 상태에서 런타임 실행 중, primitive 타입이 변경될 경우 강제 형변환이 일어나기 때문에 성능상 좋지 않다. input에서는 빈문자열 ''로 초기화해주지 않으면 undefined가 View에 표시될 수 있다.

#07 매개변수의 default 값으로 ''를 넣기보다는 null을 넣어서 안 나오는 상황에 대한 예외를 알려주는 방법도 좋겠다. (text의 경우에는 ''를 넣어도 좋다.)

#09 에러의 Scope를 어떻게 정의하느냐에 따라 에러핸들링은 달라진다. 다음의 예시 중 어떤 것이 예상되는 동작이고 어떤 것이 에러인지 생각해보자. 어떤 에러가 사용자에게도 알려주는 것이 좋고, 어떤 에러가 개발자에게만 알려주는 것이 좋은지 고민해보자.

- 사용자가 이메일 입력칸에 올바르지 않은 형식의 이메일을 입력하는 것
- API 요청 시 서버가 죽어서 500 상태코드를 받게 되는 것
- API 요청 시 네트워크가 끊겨서 응답을 받지 못하고 timeout이 나는 것

#10 알고 있는 에러에 대해서만 try-catch에서 처리해주고 나머지는 전역 에러로 던져버리자..! try-catch의 catch 안에서 throw하는게 어색하지는 않다. 다만 너무 깊어지지 않도록 함수를 잘 분리해야 한다. catch block안에서 아무것도 하지 않는다면 에러가 조용히 사라진다. 사용자도 모르고 개발자도 모르는 최악의 상황이 발생할 수 있으니 유의해야 한다.

#11 rest parameter와 object spread syntax를 활용해서 다음과 같이 확장하듯 작성할 수 있다.

// rest parameter 사용 전
export const request = async ({ uri, method, type, body, headers }) => {
...
}

// rest parameter 사용 후
export const request = async ({ uri, body, ...rest }) => {
...
}

#15 기존의 객체에서 key값 또는 value값의 일부를 수정해서 비슷한 객체를 만들고 싶은 경우, Object.fromEntries 메서드를 활용하면 코드를 더 깔끔하게 작성할 수 있다. 아래 예시코드는 기존의 RAW_PATHNAMES를 활용하여 value값에 SUBPATH가 반영된 PATH_NAMES라는 새로운 객체를 만들어내는 코드이다.

// Object.fromEntries 사용 전
const PATHNAMES = Object.freeze(
  Object.entries(RAW_PATHNAMES).reduce(
    (acc, [key, route]) => ({
      ...acc,
      [key]: `${SUBPATH}${route}`,
    }),
    {}
  )
);

// Object.fromEntries 사용 후
const RAW_PATHNAME_ENTRIES = Object.entries(RAW_PATHNAMES);
const PATHNAME_ENTRIES = RAW_PATHNAME_ENTRIES.map(([key, route]) => [key, `${SUBPATH}${route}`]);
const PATHNAMES = Object.freeze(Object.fromEntries(PATHNAME_ENTRIES));

#16 constructor()에서 여러 인자를 받을 경우에는 받아온 순서에 맞추어 할당하는 것이 좋다.

// 순서대로 할당하기
constructor(state, router) {
  this.#state = state;
  this.#router = router;
}

 

 

컨벤션/폴더링

#04 CSS에서 button:disabled 와 같이 태그에 공통된 속성은 default.css나 index.css와 같이 상위 파일에서 관리하는 것이 좋겠다.

#07 id, class는 전역 상수로 관리하는 것이 좋다. 엄청나게 바쁜 상황을 가정해보면 두 군데를 트래킹을 하기보다 한 군데에서 관리하는 것이 더 좋다.

#16 코드를 그대로 설명하는 주석은 작성하지 않는 것이 좋다. (<클린코드> 4장 주석 요약)

#18 'mock' 은 실제 객체 대신에 테스트를 위해 가짜로 만들어 넣는 객체 등을 의미한다. 따라서 실제 개발/운영 코드에 사용되는 코드나 파일의 네이밍으로 'mock' 키워드를 사용하는 것은 적합하지 않다.

#19 'stations', 'sections' 와 같이 's'가 붙으면 배열일 것이라고 예상된다. 객체이고 특별한 이유가 없다면 's'없이 'station', 'section'으로 쓰는 것이 낫다.

 

DOM

#04 구버전 브라우저에서는 이벤트를 걸어준 DOM 요소를 삭제할 때 removeEventListener를 해주는 편이 좀 더 안전하다. 하지만 모던 브라우저의 DOM GC는 해당 요소가 DOM에서 제거되어 더 이상 참조되지 않은 경우, 알아서 이벤트리스너를 삭제해주기 때문에 안해도 괜찮다.

#04 CSS에서 ul li button:hover 와 같이 태그의 이름으로 속성을 작성하는 것은 위험하다. class를 부여해서 관리하는 것이 좋겠다.

#13 리액트로 SPA 구현 시기본적인 태그들은 index.html 에 두되, route가 변경될 때마다 컴포넌트를 변경하고 DOM을 다시 그린다. 세부적인 요소라면 display: none 으로 컨트롤하는 경우도 있지만 이 것으로 Page DOM 자체를 관리하지는 않는다.

#16 최상단의 body에 모~든 이벤트를 걸어놓고 이벤트핸들러에서 서로 전혀 상관없는 것들을 if문으로 분기하는 것은 적절한 이벤트위임이 아니다. 이러한 방식은 어플리케이션이 조금만 커져도 관리가 힘들어진다. 이벤트 위임의 핵심은 '공통의 관심사'를 처리하는 것이다.  각 요소마다 핸들러를 두는 대신, '공통의 관심사'를 한번에 처리하기 위해 요소를 감싸는 상위 엘리먼트 하나에 두는 방식이 적절하다.

#18 이메일 중복여부 확인을 위해 fetch 와 같은 통신 API를 사용하더라도 반드시 <form> 태그를 사용해야 하는 것은 아니다.  마크업 구조는 크게 상관이 없다. <form> 태그를 사용하면 내부의 <button type="submit">으로 submit 이벤트를 핸들링하고 <input> 태그의 데이터들을 쉽게 가져 올 수 있기 때문에 사용하는 것이다. 

 

미션 로직 / UX

#04 회원가입 Form에서 유효성검사 후 에러 발생시 어떤 이유로 에러가 나왔는지 focus를 해주거나 에러를 뷰로 띄워주는 것이 좋겠다.

#04 회원가입 Form에서 뒤로 돌아갈 수 있는 버튼이 있으면 좋겠다.

#07 비밀번호 패턴을 제시해 주는 것이 좋겠다.

#07 비밀번호 입력란에서 영어만 입력하게 하고 싶다면, 한글이 입력시도 시 영어로 강제로 바꾸는 것보다는 validation 로직 상에서 한글을 허용하지 않게 하여 사용자에게 바꾸도록 하는게 더 좋다. 사용자의 입력을 강제로 제어하는 것 자체가 좋지 않은 방법이다. 사용자로서는 본인이 생각한 것과 다른 값을 입력할 수도 있기 때문에 잠재적인 오류를 야기할 수 있다.

#09 이메일 중복검사를 위한 중복확인 버튼을 넣는 것은 트렌디한 방식은 아니다. 최근에 많이 사용하는 UX는 다음과 같다. 이메일 작성 후 input에서 focusout 되었을 때 자동으로 유효성 검사를 한다. 클라이언트 사이드에서 검사할 수 있는 항목들을 모두 통과했다면, 사용자가 버튼을 누르지 않아도 API 요청을 바로 보내 중복체크를 한다. 중복된 이메일일 경우 "이미 가입된 이메일입니다" 라는 오류메시지를 뷰에 표시한다. (참고: Github 회원가입)

 

그외 키워드: 정규표현식 성능(#7), 질문을 '잘'하는 방법(#8)