[코드 리뷰] 채용 관련 과제 전형 자체 코드 리뷰

Wonhyun Kwon·2023년 6월 27일

코드 리뷰

목록 보기

1/1

1. 서론

이직을 준비하면서 처음으로 서류를 넣었던 곳으로부터 서류 합격 통보를 받았다. 통보와 함께 과제를 받았고, 일주일 간 구현했지만 결국 불합격 통보를 받았다.. 😢

그래도 다행인 점은 제출한 과제에 대하여 피드백을 원할 경우 회신을 달라고 했고 곧바로 피드백을 받고자 회신을 줬다.

다음은 지원했던 회사로부터 받은 피드백 내용이다.

권원현님께서는 과제 평가자 및 읽는 사람을 고려했고 배려했다는 점이 인상 깊었습니다.
다만, 코드 분리가 적절하게 적용되지 않아 가독성이 떨어지고 이해에 어려움이 있었습니다.
inline style 과 style을 정의해서 사용되는 것이 혼재되어 있는데, 공통된 inline style이 많아 최적화에 아쉬움이 있었습니다.
hook 사용 시, 불필요한 dependency 설정되어 있는 항목들로 인한 side effect 가능성이 고려되지 않는 부분에 대한 아쉬웠습니다.

정리하자면,

공통된 inline style이 많다.
hook 사용 시 불필요한 deps

이 두 가지를 중점으로 리뷰를 해보자.

1. 프로젝트 결과

리뷰에 앞서 대충 어떤 프로젝트 내용이었는지 간략하게 살펴보자.

1) 프로토타입

2) 구현 모습

메인화면 & 조회 기능
등록 기능
서치바 기능
삭제 기능
인앱 브라우저

3. 코드 리뷰

1) style 정의해서 사용 vs inline style 사용

나는 보통 아래와 같이 StyleSheet 를 객체별로 정의해서 사용하고 한다.

const styles = StyleSheet.create({
  container: {
    flex: 1,
    paddingVertical: 20,
    borderBottomWidth: 0.5,
    borderBottomColor: colors.warmGray100,
  },
  header: {
    color: colors.backgroundGray,
    fontSize: 15,
    fontWeight: '500',
  },
  text: {
    color: colors.backgroundGray,
  },
});

그리고 혼재하여 사용하는 대표적인 예시는 다음과 같다.

<SafeAreaView style={styles.container}>
  <View style={{flex: 0.05}} />
  <View style={styles.header}>
    <Text
          style={{
          fontSize: 20,
          color: colors.backgroundGray,
          fontWeight: '700',
          }}>
      {string.list}
    </Text>
    <View style={{marginHorizontal: 5}} />
    <Icon name="bookmark" size={20} color={colors.orange400} />
    <TouchableOpacity style={{position: 'absolute', right: 20, top: 5}}>
      <PopOver text={string.listInfo} />
    </TouchableOpacity>
  </View>
  <View style={{flex: 0.05}} />
  ...

이렇게 정의해서 사용하는 것과 inline style을 사용하는 기준은 큰 단위의 태그는 아무래도 중요한 컨테이너 역할과 같은 큰 틀 역할을 하기 때문에 정의해서 사용하고, 그 외 한 번만 사용될 것 같거나 Text 한줄에 필요한 style은 inline style로 작업한다.
또한, 반응형은 주로 flex 로 즐겨하는데, flex 역시 무조건 inline style로 작업한다. 오히려 난 이게 더 가독성이 좋다.

그렇게 큰 불편함 없이 작업을 해왔지만, 피드백을 받고 나서 다시 나의 코드를 보니 정말로 지저분하게 보이긴 했다.

결국 나의 문제점은 내가 보기 좋으라고 짜놓은 코드이지, 절대로 불특정 다수의 사람이 보았을 때 가독성이 좋은 코드는 아니었다.
어떻게 보면 같이 개발하는 소수의 개발자들과는 미리 약속되었다면 크게 상관이 없겠지만, 만약에 다른 개발자를 갑자기 내 앞에 데려다 놓고 바로 해석해보라 했을 때 '해석하기 싫다.' 라는 반응이 나오겠다라는 생각이 들었다.

그렇다면 어떻게 해야 style 작업할 때 태그들이 깔끔하게 보일 수 있을까?

inline style 사용을 지양하자.
보통은 style을 보기 보단 컴포넌트가 내뱉는 View를 가장 먼저 보기 때문에 태그가 깔끔해야 머릿속에 UI가 그려진다. 그런 의미에서 style은 정의해서 사용하도록 해보자.
중복은 코드 재사용성을 이용하자.
보통 <View style={{ flex: 0.2 }} /> 와 같이 반응형에 필요한 View 태그를 많이 사용한다. 이를 컴포넌트화 하여 flex에 필요한 value는 props로 던지고 저 컴포넌트를 불러 사용한다면 불필요한 코드를 덜 사용할 것 같다.
공통 된 style은 글로벌로 관리하자.
컴포넌트에서 보통 container 로 시작하는 가장 최상위 태그는 사실상 style이 다 비슷비슷하다. 2번처럼 아예 컴포넌트화하여 프로젝트 전반에서 글로벌하게 사용하던가 아니면 StyleSheet 만 따로 파일에서 관리하는 것도 나쁘지 않을 것 같다.

한번 위를 적용해서 위에 작성한 예시를 바꿔보자

<SafeAreaView style={styles.container}>
  <FlexView flex={0.05} />
  <View style={styles.header}>
    <Text style={styles.list}>
      {string.list}
    </Text>
    <FlexView marginHorizontal={5} />
    <IconTab name={string.bookmark} />
    <TouchableOpacity style={styles.popOver}>
      <PopOver text={string.listInfo} />
    </TouchableOpacity>
  </View>
  <FlexView flex={0.05} />
  ...

아주 큰 변화는 없지만, 이 전 코드보다는 다소 간결해진 것을 확인할 수 있다.
반응형에 필요한 View 태그는 따로 FlexView 라는 컴포넌트로 뺐고, 짜잘한 inline style은 전부 정의하여 사용함으로서 최대한 가독성을 높여봤다.

2) hook 사용 시 불필요한 deps 줄이기

음.. 사실 어느 훅에서 어떤 부분이 side effect가 고려되지 않았는지에 대한 상세한 피드백은 못받았기 때문에 내가 하나부터 열까지 다 고려해봐야 하는 부분이다.
사실 좀 어렵다..
다행히 이 프로젝트에서 useEffect 훅을 5개 사용했는데, 그 안에서 추려보면 되지 않을까 싶다.

2-1) deps가 무조건 작동하는 조건?

아래 예제를 보면 나는 searchBtnPressed 라는 useState<boolean> 상태를 넣어뒀다. 즉, 그말은 boolean이 true 이든 false 이든 상관없이 일단 동작한다 라는 것이 주요 포인트이다.

useEffect(() => {
  const searchFunc = async () => {
    if (searchBtnPressed) {
      if (textUrlRef.current === '') {
        showToast({type: 'custom', text1: string.alertNoInput});
      } else {
        setIsLoading(true);
        showToast({type: 'custom', text1: string.workingSearch});
        await getArchiveData({url: textUrlRef.current, years: years});
        showToast({type: 'custom', text1: string.doneSearch});
      }
      setIsLoading(false);
      setSearchBtnPressed(false);
    }
  };
  searchFunc();
}, [searchBtnPressed]);

내가 의도했던 로직은 일단 useEffect 를 실행을 하지만, searchFunc 안의 조건에 의해 실제 기능에 영향을 끼치는 함수는 실행할 수도 안할 수도 있다 라는 것이었다.
그 말은, 의도하든 의도치 않든 일단 버튼을 누르면 무조건 렌더링된다!! 라는 것이다.

근데 과연 버튼을 한번 누름으로서 다시 리렌더링 된다는 게 비효율적인 것은 맞지만 '치명적인가?' 에 대해선 아직도 물음표이다. 조건이 안맞으면 유저에게 알림을 전달해야할텐데 이런 경우엔 리렌더링없이 정말 알림만 딱! 띄워줄 수 있는지에 대한 여부는 아직 모르겠다.

useCallback 훅으로 최적화를 해야했었을까? 하는 생각이 스쳐지나갔다. 사실 useCallback 과 useMemo 와 같은 대표적인 메모이제이션은 많이 다뤄보지 않아서 사용하는 데 있어 서툰 것은 사실이다. 일단 메모이제이션으로 재접근하는 방향으로 다른 포스트에서 다뤄봐야겠다.

2-2) 리렌더링의 조건

다음 코드에서 refresh 를 봐보자. setRefresh 의 상태관리로 내가 의도했던 바는, async-storage에 저장되어 있는 어떠한 데이터를 삭제하면 그걸 갱신해줘야 했기 때문에 강제로 리렌더링을 유도하고자 useState 를 사용하고 그것을 훅에서 deps로 참조한 것이다.

useEffect(() => {
  const getAddList = async () => {
    const getStorage = await asyncStorage.get('url');
    if (getStorage) {
      addList.current = getStorage;
      setCountList(Object.keys(addList.current).length);
    }
  };
  getAddList();
}, [isFocused, refresh]);

그런데 이걸 redux와 같은 전역 상태 관리 툴을 이용해서 감지하고 변경된다면 이 컴포넌트가 자동으로 갱신되게 한다면?
물론, async-storage에 저장된 것을 상태 관리 관점에서 봐야한다는 것은 공부를 해야겠지만, 그렇게 된다면 지금처럼 강제로 refresh 해야 하는 risk가 큰 코딩은 지양했을 것 같다.

2-3) useRef 훅에 대한 부족한 지식

이건 deps에 따른 side effect 문제랑 별개지만, 훅을 잘못 사용하고 있음은 동일해서 적는다.

TextInput 박스에 ref 를 걸고 text를 입력하면 그 값을 다른 함수에 전달해서 원하는 기능을 구현하고자 했다.
근데, focus 를 받는 ref 와 text를 받는 ref 를 따로따로 구현해서 사용했다.
그냥 ref 하나만 걸어두면 둘 다 되는 것인데, 몰라서 두 개를 사용했다.

잘 못된 예시는 아래와 같다.

const inputUrlRef = useRef<TextInput>(null);
const textUrlRef = useRef<string>('');

const handleInputUrlChange = (text: string) => {
  textUrlRef.current = text;
};

<TextInput
  ref={inputUrlRef}
  style={styles.textInput}
  placeholder={string.enterUrl}
  placeholderTextColor={colors.gray400}
  onChangeText={handleInputUrlChange}
  />

다음은 수정한 코드이다.

const inputUrlRef = useRef<string>('');

const handleUrlTextInput = (text: string) => {
  inputUrlRef.current = text;
};

<TextInput
  style={styles.textInput}
  placeholder={string.enterUrl}
  placeholderTextColor={colors.gray400}
  onChangeText={handleUrlTextInput}
  />

결론적으로 TextInput 의 프로퍼티인 ref는 제거했다. ref 프로퍼티에 inputUrlRef 를 걸어두고 current 객체 속성에 할당받으려 했으나 실패했다. 더 높은 RN 버전에선 어떻게 작동하는진 모르겠지만, text를 받을 useRef 를 따로 만들어두고 프로퍼티 ref는 비워둬야지만 다른 함수에서 inputUrlRef.current 에 담긴 text를 받아 사용할 수 있다.

3) 그 외

3-1) 복잡한 익명함수는 선언하여 사용하자

<TouchableOpacity
  style={styles.btn}
  onPress={() => {
    setSearchBtnPressed(true);
    inputUrlRef.current?.blur();
    inputNicknameRef.current?.blur();
  }}>
  ...

onPress 프로퍼티 안에 너무 많은 함수가 존재한다. 이를 하나의 정의 된 함수로 빼서 가독성을 높이자.

const searchBtnPressed = () => {
	setSearchBtnPressed(true);
	inputUrlRef.current?.blur();
	inputNicknameRef.current?.blur();
}

<TouchableOpacity
  style={styles.btn}
  onPress={searchBtnPressed}>
  ...

3-2) 최종 return tag는 최소화하자.

내가 작성한 컴포넌트 return 문을 보자.
내가 작성했는데도 스스로가 읽고 싶겠는가?

  return (
    <SafeAreaView style={styles.container}>
      <View style={{flex: 1, margin: 20}}>
        <View style={{flex: 0.1}}>
          <View style={{flexDirection: 'row'}}>
            <Image
              source={require('../../assets/images/logo.png')}
              style={{
                width: 30,
                height: 30,
              }}
              resizeMode="contain"
            />
            <View style={{justifyContent: 'center', marginLeft: 10}}>
              <Text
                style={{
                  color: colors.backgroundGray,
                  fontSize: 25,
                  fontWeight: '700',
                }}>
                {string.appName}
              </Text>
            </View>
          </View>
        </View>
        <View style={{flex: 0.05}} />
        <View style={{flex: 0.05, flexDirection: 'row'}}>
          <Text style={styles.commonText}>{string.shortGuide}</Text>
          <View style={{marginHorizontal: 5, justifyContent: 'center'}} />
          <PopOver text={string.homeInfo} />
        </View>
        <View style={{flex: 0.08}}>
          <TextInput
            ref={inputUrlRef}
            style={styles.textInput}
            placeholder={string.enterUrl}
            placeholderTextColor={colors.gray400}
            onChangeText={handleInputUrlChange}
          />
        </View>
        <View style={{flex: 0.02}} />
        <View style={{flex: 0.08}}>
          <TextInput
            ref={inputNicknameRef}
            style={styles.textInput}
            placeholder={string.enterNickname}
            placeholderTextColor={colors.gray400}
            onChangeText={handleInputNicknameChange}
          />
        </View>
        <View style={{flex: 0.05}} />
        <Text style={{color: colors.backgroundWhite}}>
          {string.howManyYears}
        </Text>
        <View style={{flex: 0.02}} />
        <View style={{flex: 0.05}}>
          <Sliders
            unit={string.years}
            minimiumVlaue={1}
            maximumValue={20}
            step={1}
            value={years}
            onValueChange={setYears}
          />
        </View>
        <View style={{flex: 0.05}} />
        <View style={[{flex: 0.1}, styles.btns]}>
          {isLoading ? (
            <ActivityIndicator size={'large'} />
          ) : (
            <View style={{flexDirection: 'row'}}>
              <TouchableOpacity
                style={styles.btn}
                onPress={() => {
                  setSearchBtnPressed(true);
                  inputUrlRef.current?.blur();
                  inputNicknameRef.current?.blur();
                }}>
                <Text
                  style={[
                    {fontSize: 18, fontWeight: '700'},
                    styles.commonText,
                  ]}>
                  {string.search}
                </Text>
              </TouchableOpacity>
              <View style={{flex: 0.1}} />
              <TouchableOpacity
                style={styles.btn}
                onPress={() => {
                  setAddBtnPressed(true);
                  inputUrlRef.current?.blur();
                  inputNicknameRef.current?.blur();
                }}>
                <Text
                  style={[
                    {fontSize: 18, fontWeight: '700'},
                    styles.commonText,
                  ]}>
                  {string.add}
                </Text>
              </TouchableOpacity>
            </View>
          )}
        </View>
        <View style={{flex: 0.08}} />
        <View style={{flex: 0.07, flexDirection: 'row'}}>
          <Text style={[{fontWeight: '700', fontSize: 20}, styles.commonText]}>
            {string.appName}
          </Text>
          <Text style={[{fontSize: 20}, styles.commonText]}>
            {string.whatIsApp}
          </Text>
        </View>
        <View style={{flex: 0.2}}>
          <Text style={styles.commonText}>{string.appDescription}</Text>
        </View>
      </View>
      <ListModal
        navigation={navigation}
        isPressed={textUrlRef.current === '' ? false : searchBtnPressed}
        list={searchData}
        onClose={() => setSearchBtnPressed(false)}
      />
    </SafeAreaView>
  );
};

물론, 반응형 만든다고 좀 더 복잡하게 보이는 것도 있긴하다.
그럼에도 불구하고 무조건 나눌 수 있는 부분은 나눴어야 한다고 본다.
예를 들면 isLoading 조건에 따라 렌더링되는 부분을 나눠서 표현했으면 어떨가 싶다.

3-3) flex는 좀 더 연구해보자

다음은 내가 자주 이용하는 반응형 형태이다.

<View style={{flex: 0.05}} />

확실한 반응형을 만드는 데엔 성능은 확실하지만, 가독성이 매~~우 떨어진다.
flex 는 당연히 inline style은 지양하되, 어떻게 하면 flex 를 통한 반응형도 가져가면서 동시에 가독성을 높일 수 있는지는 공부가 더 필요할 것 같다.

Wonhyun Kwon

모든 사용자가 만족하는 UI를 만드는 FE 개발자 권원현입니다.