[POB] 기업과제 - 원티드 #4

 

1. 과제 내용

이응수 대리를 도와 책 찾기 프로젝트를 진행해 나가실 팀원이며 이응수 대리가 과거 작성한 코드와 결과물을 분석해서 앞으로 어떻게 하면 더 큰 규모의 프로그램을 만들어 나갈 수 있을지 코드 리뷰를 해주시면 됩니다.

 

• 실제 프로덕트라고 가정하고 리뷰한다.
• 확장 가능성에 주안점을 둔다.
• 좋은 점, 나쁜 점에 대해 솔직하게 반응한다.
• 본인의 개발 경험 중심으로 풀어나간다.
• 버그 발견 시, 추측이 아닌 구체적 방법을 제시한다.

2. 리팩토링 사항

📁 components/Books/Books.js

1. 태그의 속성이 너무 길어져 가독성을 해치기 때문에 Inline styling을 분리
2. alt 속성이 없을 경우 이미지가 손상되었을 때 정보를 알 수 없고 검색엔진에 최적화 반영이 제대로 되지 않아 추가

 

❌ 수정 전 

<img
  src={volumeInfo?.imageLinks?.thumbnail}
  className={cx(
    styles.media,
    css({
      width: 128,
      height: 128
    })
  )}
 alt=""
/>

✅ 수정 후

<img
    src={volumeInfo?.imageLinks?.thumbnail}
    className={cx(styles.media, imgSize)}
    alt="Book image"
/>
...

const imgSize = css`
  width: 128px;
  height: 128px;
`

 

📁 components/Books/Pagination.js

1. 인라인에 함수 로직이 포함 되어 있어 가독성을 해쳐 외부에서 이벤트 핸들러 전달

 

❌ 수정 전 

<button
  className={styles.button}
  disabled={startIndex === 0 || isLoading}
  onClick={() => {
    if (isLoading) return;
    dispatch(fetchBooks(location.search, startIndex))
  }}
>
  {isLoading ? '로딩중...' : '더보기'}
</button>

 

✅ 수정 후

const loadBookData = () => {
    if (isLoading) {
      return
    }
    dispatch(fetchBooks(location.search, startIndex))
  }
...
<button
    className={styles.button}
    disabled={startIndex === 0 || isLoading}
    onClick={loadBookData}
>
    {isLoading ? '로딩중...' : '더보기'}
</button>
...

 

📁 components/Form/Select.js, components/searchForm/IconFilter.js

1. SVG 요소를 assets 으로 이동시켜 svgs.js를 모듈로 사용
2. 현재 emotion을 사용중이므로 사용자가 임의로 className을 부여 할 수 있도록 수정

 

❌ 수정 전 

function Select({ id, children, ...rest }) {
  return (
    <label htmlFor={id} className={styles.wrapper}>
      <select id={id} name={id} className={styles.select} {...rest}>
        {children}
      </select>
      <div className={styles.icon}>
        <svg
          className={styles.svg}
          xmlns="http://www.w3.org/2000/svg"
          viewBox="0 0 20 20"
        >
          <path d="M9.293 12.95l.707.707L15.657 8l-1.414-1.414L10 10.828 5.757 6.586 4.343 8z" />
        </svg>
      </div>
    </label>
  )
}

 

✅ 수정 후

// svgs.js

export const SelectSVG = ({ className }) => {
  return (
    <svg
      className={className}
      xmlns="http://www.w3.org/2000/svg"
      viewBox="0 0 20 20"
    >
      <path d="M9.293 12.95l.707.707L15.657 8l-1.414-1.414L10 10.828 5.757 6.586 4.343 8z" />
    </svg>
  )
}

export const IconFilterSVG = () => {
  return (
  	<svg
      xmlns="http://www.w3.org/2000/svg"
      enableBackground="new 0 0 24 24"
      height="24"
      viewBox="0 0 24 24"
      width="24"
      className="fill-current"
    >
      <g>
        <path d="M0,0h24 M24,24H0" fill="none" />
        <path d="M4.25,5.61C6.27,8.2,10,13,10,13v6c0,0.55,0.45,1,1,1h2c0.55,0,1-0.45,1-1v-6c0,0,3.72-4.8,5.74-7.39 C20.25,4.95,19.78,4,18.95,4H5.04C4.21,4,3.74,4.95,4.25,5.61z" />
        <path d="M0,0h24v24H0V0z" fill="none" />
      </g>
    </svg>
  ) 
}

📁 components/SearchForm/index.js, components/SearchForm/LinkToFilter.js, components/hooks/useForm.js

1. pathname 을 상수로 교체하여, 여러 로직에서 사용 되는 pathname을 일괄 수정   

 

❌ 수정 전 

// components/SearchForm/LinkToFilter.js
<Link
  to={{
  pathname: '/filters',
  search: location.search
  }}
  className={styles.wrapper}
>
  <IconFilter />
</Link>

// components/SearchForm/index.js
{location.pathname === '/result' && <LinkToFilter />}

// components/hooks/useForm.js

function handleRouter(query) {
  const path = stringifyUrl({
    url: '/result',
    query
  })
  history.push(path)
}

 

✅ 수정 후

function handleRouter(query) {
    const { result } = ROUTE_PATH
    const path = stringifyUrl({
     rl: result,
    query
  })
  history.push(path)
}

export const ROUTE_PATH = {
  main: '/',
  result: '/result',
  filters: '/filters'
}
...

📁 components/pages/Filters/index.js

1. 반복 되는 fieldset, radio, filter select의 data를 상수로 처리
2. 가독성을 위해 이벤트 핸들러를 onClick에 직접 할당

 

❌ 수정 전 

<Fieldset legend="도서 종류">
   <Stack gaps={[0, 20, 20]} direction="horizontal">
     <Radio
       id="printTypeAll"
       name="printType"
       value="all"
       checked={state.printType === 'all'}
       onChange={handleChange}
       label="전체"
     />
     <Radio
       id="printTypeBooks"
       name="printType"
       value="books"
       checked={state.printType === 'books'}
       onChange={handleChange}
       label="일반도서"
     />
     <Radio
       id="printTypeMagazines"
       name="printType"
       value="magazines"
       checked={state.printType === 'magazines'}
       onChange={handleChange}
       label="잡지"
     />
   </Stack>
</Fieldset>

<Fieldset legend="정렬 순서">
   <Stack gaps={[0, 20, 20]} direction="horizontal">
     <Radio
       id="orderByRelevance"
       name="orderBy"
       value="relevance"
       checked={state.orderBy === 'relevance'}
       onChange={handleChange}
       label="관련성"
     />
     <Radio
       id="orderByNewest"
       name="orderBy"
       value="newest"
       checked={state.orderBy === 'newest'}
       onChange={handleChange}
       label="최신순"
     />
   </Stack>
</Fieldset>

 

✅ 수정 후

//Filter.js
...
{FIELDSET_DATA.map((fieldData) => (
      <Fieldset legend={fieldData.title} key={fieldData.title}>
        <Stack gaps={[0, 20, 20]} direction="horizontal">
          {fieldData.raido.map((radio) => (
            <Radio
              key={radio.id}
              id={radio.id}
              name={fieldData.name}
              value={radio.value}
              checked={state[fieldData.name] === radio.value}
              onChange={handleChange}
              label={radio.label}
            />
          ))}
        </Stack>
      </Fieldset>
    ))}
...
<button
  type="button"
  className={cx([styles.button, styles.cancel])}
  onClick={() => history.goBack()}
>
  취소
</button>

// constants.js
...
export const FIELDSET_DATA = [
  {
    title: '도서종류',
    name: 'printType',
    raido: [
      {
        id: 'printTypeAll',
        value: 'all',
        label: '전체'
      },
      {
        id: 'printTypeBooks',
        value: 'books',
        label: '일반도서'
      },
...

 

📁 components/pages/Result/index.js

useSync와 비슷한 로직을 가지고 실행 되고 있었기에, 함수를 전달 하는 방식으로 수정

 

❌ 수정 전 

function Result() {
  const dispatch = useDispatch()
  const { search } = useLocation()
  const { items } = useSelector(selectBooks)

  useEffect(() => {
    if (!search) {
      return
    }

    dispatch(fetchBooks(search))
  }, [dispatch, search])
  ...
  }

 

✅ 수정 후

function Result() {
  const { items } = useSelector(selectBooks)
  useSync(fetchBooks)
...
}

📁 App.js

1. 기존 Routes 컴포넌트와 App 컴포넌트가 함께 있었기에, 가독성을 위해 이를 두개의 컴포넌트로 분리
2. useTransition에 사용되던 opacity object를 상수로 처리 하여, 추후 설정 추가가 용이할 수 있도록 수정
3. router의 path를 상수로 처리해 가독성을 높이고, 추후 변경이 용이하도록 수정
4. route의 셀프클로징 적용으로 가독성을 향상 시킬 수 있도록 수정

 

❌ 수정 전 

// App.js

function Routes() {
  const location = useLocation()
  const transitions = useTransition(location, location => location.pathname, {
    from: { opacity: 0 },
    enter: { opacity: 1 },
    leave: { opacity: 0 }
  })
...
}

function App() {
...
}

 

✅ 수정 후

//Routes.js
function Routes() {
  const location = useLocation()
  const transitions = useTransition(
    location,
    (location) => location.pathname,
    TRANSITION_OPACITY
  )
  const { main, result, filters } = ROUTE_PATH

  return transitions.map(({ item: location, props, key }) => (
    <animated.div key={key} className="absolute w-full" style={props}>
      <Switch location={location}>
        <Route exact path={main} component={Main} />
        <Route path={result} component={Result} />
        <Route path={filters} component={Filters} />
      </Switch>
    </animated.div>
  ))
}

//constants.js
export const ROUTE_PATH = {
  main: '/',
  result: '/result',
  filters: '/filters'
}
...
const OPACITYLEVEL = {
	transparent : 0,
	nonTransparent: 1
}
export const TRANSITION_OPACITY = {
  from: { opacity: OPACITYLEVEL.transparent },
  enter: { opacity: OPACITYLEVEL.nonTransparent },
  leave: { opacity: OPACITYLEVEL.transparent }
}
...

📁 api.js

1. 데이터를 요청하는 api의 주소가 바뀌거나, 전역에서 사용하게 될 경우를 고려하여 url을 config.js 파일로 이동

 

❌ 수정 전 

export async function getBooks(search, startIndex) {
  const { q, ...rest } = parse(search)
  const input = stringifyUrl({
    url: 'https://www.googleapis.com/books/v1/volumes',
    query: {
      q: `${q}`,
      startIndex,
      projection: 'full',
      ...rest
    }
  })

  return fetch(input)
}

 

✅ 수정 후

//api.js
export async function getBooks(search, startIndex) {
  const { q, ...rest } = parse(search)
  const input = stringifyUrl({
    url: dataFetchURL,
    query: {
      q: `${q}`,
      startIndex,
      projection: 'full',
      ...rest
    }
  })

  return fetch(input)
}

//config.js
export const dataFetchURL = 'https://www.googleapis.com/books/v1/volumes'

📁 components/Books/Pagination.js

Loading.js가 사용 된 곳이 없어 Pagination.js에 적용

 

❌ 수정 전 

function Pagination() {
  const dispatch = useDispatch()
  const location = useLocation()
  const { status, startIndex } = useSelector(selectBooks)
  const isLoading = status === Status.Loading

  return (
    <button
      className={styles.button}
      disabled={startIndex === 0 || isLoading}
      onClick={() => {
        if (isLoading) {
          return
        }
        dispatch(fetchBooks(location.search, startIndex))
      }}
    >
      {isLoading ? '로딩중...' : '더보기'}
    </button>
  )
}

 

✅ 수정 후

import { Loading } from '../Loading'

function Pagination() {
  const dispatch = useDispatch()
  const location = useLocation()
  const { status, startIndex } = useSelector(selectBooks)
  const isLoading = status === Status.Loading

  return (
    <button
      className={styles.button}
      disabled={startIndex === 0 || isLoading}
      onClick={() => {
        if (isLoading) {
          return
        }
        dispatch(fetchBooks(location.search, startIndex))
      }}
    >
      {isLoading ? <Loading /> : '더보기'}
    </button>
  )
}

 

3. 추가 고려사항

• 혼동 될 수 있는 Component의 이름 수정 Ex) Book.js & books.js
• 함수의 역할을 뚜렷하게 나타낼 수 있게 이름을 수정 Ex) handleRouter ⇒ moveToQueryUrl
• 전체적으로 semantic tag가 사용 되지 않았기에, 사용 해보는게 좋을 것 같다