1회차 코드 리뷰 스터디

[bbearcookie]

이전에 구현했던 로직이 꽤나 복잡하게 느껴졌었어요.
혹시 간단하게 해결할 수 있는 방법이 있을지, 함께 고민해보면 좋을 것 같아서 코드를 가져왔어요. 😊

구현 기능

• 서버로부터 편지 목록을 불러오는 API를 호출하고 받은 배열 데이터를 4개씩 슬라이싱해서 화면에 보여줘야 해요.
  • Carousel 형태의 화면으로 슬라이드하면서 다음 페이지를 보여줘요.
  • 각 Carousel 당 고정된 위치에 물병 이미지의 형태로 편지를 보여줘요.

문제 상황

• 60초 간격의 폴링을 적용했는데, 서버에서 배열 중간에 있는 편지가 제거된다면 그 뒷 번호의 편지가 앞으로 땡겨지면서 사용자 입장에서 기존의 편지 위치가 변경되어 보이는 문제가 있었어요.
  • ex) [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] 을 서버로부터 받아온 상황에서 중간에 5가 삭제된다면
  • 배열은 [1, 2, 3, 4, 6, 7, 8, 9, 10] 이 되어서, ID가 6번보다 뒤에 있는 모든 편지가 한 인덱스 씩 앞으로 이동하는 문제가 있어요.

해결 방법

• 서버로부터 가져왔던 편지 ID의 인덱스를 클라이언트 스토어 상태로 관리하여 기억하되, 폴링 주기마다 서버로부터 가져온 API 데이터에 더 이상 존재하지 않는 데이터라면 스토어에서도 제거해요.

type StateItem = {
  id: number;
  isRead: boolean;
};

/**
 * 원본 아이템 배열에서 새로운 아이디 배열로 동기화합니다.
 * 1. 원본 아이템 배열에서 새로운 아이디 배열에 있지 않은 아이템은 null로 변경합니다.
 * 2. 새로운 아이디 배열에 있는 아이디 중 원본 배열에 없는 아이디는 새로운 아이디로 채웁니다.
 *
 * @param original 원본 아이템 배열
 * @param newIds 새로운 아이디 배열
 */
const syncArrayWithNewIds = (
  original: (StateItem | null)[],
  newIds: number[],
) => {
  const nonExistingIds = newIds.filter(
    (id) => !original.some((item) => item?.id === id),
  );

  return original
    .map((item) => (item && newIds.includes(item.id) ? item : null))
    .map((item) =>
      item
        ? item
        : nonExistingIds.length > 0
          ? { id: nonExistingIds.shift()!, isRead: false }
          : null,
    );
};

[Wisesaturn]

뭔가 쉬우면서 어렵네요 ㅋㅋㅋ
map으로 데이터를 재가공해서 괜찮아 보이는 데
삼항연산자가 많이 껴있어서 조금 복잡스러운 느낌이긴 하군요

하지만 킹갓상훈님의 JsDoc으로 쉽게 이해되는 매직

제가 만-약 수정해본다면 nonExistingIds와 map하는 연산을 reduce로 한 번에 묶어볼 것 같습니다

newIds.reduce<(StateItem | null)[]>((acc, id) => {
    const existingItem = original.find(item => item?.id === id);
    if (existingItem) {
      acc.push(existingItem);
    } else {
      acc.push({ id, isRead: false });
    }
    return acc;
  }, []);

[regularPark]

return original
    .map((item) => (item && newIds.includes(item.id) ? item : null))
    .map((item) =>
      item
        ? item
        : nonExistingIds.length > 0
          ? { id: nonExistingIds.shift()!, isRead: false }
          : null,
    );

이 코드가 map이 이중으로 존재하는데, 가독성이 좋지는 않아보여서 첫번째 map을 따로 변수로 분리를 해도 좋을 듯 싶네여

요런 너낌으로다가?

const updatedArray = original.map(item =>
    item && newIds.includes(item.id) ? item : null
  );

return updatedArray.map( //..

[KyuTae98]

뭔가 알고리즘 문제 느낌이네용!!

저는 js로 알고리즘 문제를 풀 때 map을 여러 개 쓰기보다는 reduce를 애용했었습니다!!

예외 처리할 때 더 직관적으로 찾을 수 있더라구요!!

(이미 재한님이 의견을 주셨군요,,,)

[qhahd78]

• 요런 구현은 할 수 있지만, 더 나아가 좋은 코드를 고민할 때 GPT 한테 많이 물어보곤 하는데 ㅋㅋㅋ
  • GPT 없이 그냥 생각해봤을 때는 재한님 의견처럼 reduce 를 사용해 비교함과 동시에 배열을 만들어내는 방법을 사용할 것 같아요.

아래는 GPT 의 의견인데, Set 을 사용하면 특정값의 존재 여부를 훨씬 빨리 사용할 수 있다고 해요. 저도 처음 안 내용이라 적어두고 갑니다.

Set 을 사용하는 이유

Set의 has 메서드 -> O(1)
배열의 includes 메서드 -> O(n)

GPT 의 의견 )

const syncArrayWithNewIds = (
  original: (StateItem | null)[],
  newIds: number[],
) => {
  // original 배열에서 존재하는 id들을 Set으로 저장하여 빠르게 조회할 수 있도록 합니다.
  const existingIdsSet = new Set(original.filter(item => item).map(item => item!.id));

  // newIds 배열에서 original에 없는 id들을 필터링하여 저장합니다.
  const nonExistingIds = newIds.filter(id => !existingIdsSet.has(id));

  // original 배열을 순회하면서 필요한 변환을 수행합니다.
  return original.map(item => {
    // item이 존재하고 newIds에 포함되어 있으면 그대로 유지합니다.
    if (item && newIds.includes(item.id)) {
      return item;
    }

    // 그렇지 않으면 nonExistingIds에서 id를 하나 꺼내 새로운 객체를 생성합니다.
    if (nonExistingIds.length > 0) {
      return { id: nonExistingIds.shift()!, isRead: false };
    }

    // nonExistingIds가 비어있으면 null을 반환합니다.
    return null;
  });
};