[Bug]: 공백이 포함된 초성 검색 시 예상치 못한 false 반환 문제

[BO-LIKE-CHICKEN]

Bug description

Describe the bug

다음과 같은 테스트 케이스를 추가했으나 false가 반환되어 테스트가 실패하는 현상을 발견했습니다.

it('should return true when "ㅍㄹㅌㅇㄷ ㄱㅂㅈ" is entered for searching "프론트엔드"', () => {
    expect(chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ')).toBe(true);
 });

Expected behavior

'ㅍㄹㅌㅇㄷ ㄱㅂㅈ' 같은 입력에서도 공백이 포함되어 있어도 정상적으로 동작해야 합니다. 특히 다음 코드 부분에서 공백을 제거하는 로직이 포함되어 있어, 의도하지 않은 버그로 보입니다.

const initialConsonantsY = getFirstConsonants(y).replace(/\s/g, '');

예를 들어 '빌즈 강남'을 'ㅂㅈ ㄱㄴ'으로 검색했을 때 일치하는 결과가 반환되면 사용자 경험에 좋을 것입니다.

To Reproduce

chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ')

Possible Solution

판단 기준하에 다음과 같은 두 가지 방법으로 문제를 해결 혹은 개선할 수 있을 것으로 예상됩니다.

1. 공백 포함 입력도 정상적으로 처리:
   함수를 수정하고 테스트 코드를 추가합니다.

import { HANGUL_CHARACTERS_BY_FIRST_INDEX } from './constants';
import { disassembleHangulToGroups } from './disassemble';
import { getFirstConsonants, hasValueInReadOnlyStringList } from './utils';

export function chosungIncludes(x: string, y: string) {
  if (!isOnlyInitialConsonant(y)) {
    return false;
  }

  const initialConsonantsX = getFirstConsonants(x).replace(/\s/g, '');
  const initialConsonantsY = getFirstConsonants(y).replace(/\s/g, '');

  return initialConsonantsX.includes(initialConsonantsY);
}

/*
 * @description 공백을 제외한 문자열이 한글초성으로만 주어진 경우
 */
function isOnlyInitialConsonant(str: string) {
  const trimStr = str.replace(/\s/g, '');

  return disassembleHangulToGroups(trimStr).every(disassembled => {
    return disassembled.length === 1 && hasValueInReadOnlyStringList(HANGUL_CHARACTERS_BY_FIRST_INDEX, disassembled[0]);
  });
}

  it('should return true when "ㅍㄹㅌㅇㄷㄱㅂㅈ" is entered for searching "프론트엔드 개발자"', () => {
    expect(chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷㄱㅂㅈ')).toBe(true);
  });

  it('should return true when "ㅍㄹㅌㅇㄷ ㄱㅂㅈ" is entered for searching "프론트엔드 개발자"', () => {
    expect(chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ')).toBe(true);
  });

2. 공백 없이 초성만 입력받아 처리:
   initialConsonantsY에서 공백을 제거할 필요 없이 처리하도록 개선하고 테스트 케이스를 추가합니다.

const initialConsonantsY = getFirstConsonants(y)

it('should return true when "ㅍㄹㅌㅇㄷ ㄱㅂㅈ" is entered for searching "프론트엔드"', () => {
    expect(chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ')).toBe(false);
});

Additional context

1. 두 번째 인자에 공백이 들어가더라도 기능이 정상적으로 동작해야 한다.

로 선택한다면 공백 포함 여부를 어떻게 처리할지에 대한 결정이 중요한 요소로 보입니다.

감사합니다 😁

[okinawaa]

좋은 의견과, 해결책까지 제시해주셔서 감사해요!

1. 엣지 케이스를 대응하기 위해, 유지보수 난이도 상승(관련 코멘트)
2. 함수의 관심사를 명확하게 분리하기

코맨트와 비슷한 의견인데요, 함수의 목적에 맞게 chosungIncludes는 문자에, 초성이 포함되는지 까지의 역할을 담당하고,
더 나은 유저 경험을 위해 띄어쓰기 까지 고려하는 경우는 해당 함수를 사용하는 서비스 개발자에게 맡기는것은 어떠신가용?

예시

'빌즈 강남'을 'ㅂㅈ ㄱㄴ'으로 검색했을 때 일치하는 결과가 반환되길 원한다면 아래와 같이 사용해도 좋을 것 같아요!

const word = "빌즈 강남";
const chosung = "ㅂㅈ ㄱㄴ";

const trimmedWord = word.replace(/\s/g, '');
const trimmedChosung = chosung.replace(/\s/g, '');

const 유저가_입력한_초성이_단어에_맞는가 = chosungIncludes(trimmedWord, trimmedChosung);

---

함수 내부에서 처리하지 않기로 한다면, DOCS에 추가 꿀팁 같은것을 남겨도 좋을 것 같아요.

예시

🍯 공백을 포함한 단어도 처리하고 싶다면, 다음과 같이 사용해보세요!

[BO-LIKE-CHICKEN]

좋은 의견과, 해결책까지 제시해주셔서 감사해요!

1. 엣지 케이스를 대응하기 위해, 유지보수 난이도 상승(관련 코멘트)
2. 함수의 관심사를 명확하게 분리하기

코맨트와 비슷한 의견인데요, 함수의 목적에 맞게 chosungIncludes는 문자에, 초성이 포함되는지 까지의 역할을 담당하고, 더 나은 유저 경험을 위해 띄어쓰기 까지 고려하는 경우는 해당 함수를 사용하는 서비스 개발자에게 맡기는것은 어떠신가용?

예시

'빌즈 강남'을 'ㅂㅈ ㄱㄴ'으로 검색했을 때 일치하는 결과가 반환되길 원한다면 아래와 같이 사용해도 좋을 것 같아요!

const word = "빌즈 강남";
const chosung = "ㅂㅈ ㄱㄴ";

const trimmedWord = word.replace(/\s/g, '');
const trimmedChosung = chosung.replace(/\s/g, '');

const 유저가_입력한_초성이_단어에_맞는가 = chosungIncludes(trimmedWord, trimmedChosung);

함수 내부에서 처리하지 않기로 한다면, DOCS에 추가 꿀팁 같은것을 남겨도 좋을 것 같아요.

예시

🍯 공백을 포함한 단어도 처리하고 싶다면, 다음과 같이 사용해보세요!

---

먼저, 좋은 의견 감사드립니다! 첨부해 주신 코멘트 또한 매우 유용했습니다.
이슈를 제기하기 전에 이 부분이 마이너한 케이스인지에 대해 많은 고민을 했습니다. 🤔
앞서 언급하신대로 이 부분을 버그로 판단하지 않았던 이유는 아래 코드 때문입니다.

기존코드

const initialConsonantsY = getFirstConsonants(y).replace(/\s/g, '');

isOnlyInitialConsonant 함수는 인자로 받는 문자열에 공백이 포함되면 반드시 false를 반환합니다. 따라서 initialConsonantsY에서 공백을 제거하는 것은 불필요합니다.

따라서, 더 나은 사용자 경험을 위해 띄어쓰기까지 고려하는 것을 서비스 개발자에게 맡긴다면, 다음과 같이 개선할 수 있을 것 같습니다.

개선하기

1. 불필요한 공백 제거 로직을 제외한다

const initialConsonantsY = getFirstConsonants(y);

2. 문서화의 역할을 겸하는 테스트코드를 추가한다

it('should return false when "ㅍㅌㅌㅇㄷ ㄱㅂㅈ" is entered for searching "프론트엔드 개발자" as the function does not support spaces in the input', () => {
    expect(chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ')).toBe(false);
  });

3. chosungIncludes.md에 공백이 포함된 문자열에서 사용하는 방법을 예시로 추가한다.

Tips

공백을 포함한 단어도 처리하고 싶다면, 다음과 같이 사용해보세요!

const word = '프론트엔드 개발자';
const chosung = 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ';

const trimmedWord = word.replace(/\s/g, '');
const trimmedChosung = chosung.replace(/\s/g, '');

const 유저가_입력한_초성이_단어에_맞는가 = chosungIncludes(trimmedWord, trimmedChosung);

덧붙여 es-hangul에서는 utils를 제외한 함수들에 TSDoc을 사용하지 않는 것으로 보이는데요. 이는 각각의 기능에 대한 명세를 chosungIncludes.md 같은 파일에서 관리하고자는 의도일까요? 이 부분이 아니라면 TSDoc을 활용해서 함수를 사용하는 서비스 개발자에게 알려주어도 무척 좋겠다는 생각이 듭니다.

감사합니다!

[okinawaa]

es-hangul에서는 utils를 제외한 함수들에 TSDoc을 사용하지 않는 것으로 보이는데요. 이는 각각의 기능에 대한 명세를 chosungIncludes.md 같은 파일에서 관리하고자는 의도일까요?

네! Docs에 접근하는게 크게 어렵진 않을 것 같아서, Docs에 라이브러리 맥락에 대한 정보를 최대한 담으려고 하고 있어요! TSDoc도 사용하면 좋겠지만, TSDoc과, Docs 두벌을 관리하기에는 점점 라이브러리가 커감에 따라, 두 문서간 어긋나는 부분도 생기고 SSOT를 지키기 어려울 것 같다고 생각해요. 당연히 TSDoc를 사용하면 개발자 경험은 좋을 것이라는 의견에는 동의합니다! 나중에 한번 더 이 부분 리비짓 해보시죠!

[evan-moon]

좋은 의견과, 해결책까지 제시해주셔서 감사해요!

1. 엣지 케이스를 대응하기 위해, 유지보수 난이도 상승(관련 코멘트)
2. 함수의 관심사를 명확하게 분리하기

코맨트와 비슷한 의견인데요, 함수의 목적에 맞게 chosungIncludes는 문자에, 초성이 포함되는지 까지의 역할을 담당하고, 더 나은 유저 경험을 위해 띄어쓰기 까지 고려하는 경우는 해당 함수를 사용하는 서비스 개발자에게 맡기는것은 어떠신가용?

예시

'빌즈 강남'을 'ㅂㅈ ㄱㄴ'으로 검색했을 때 일치하는 결과가 반환되길 원한다면 아래와 같이 사용해도 좋을 것 같아요!

const word = "빌즈 강남";
const chosung = "ㅂㅈ ㄱㄴ";

const trimmedWord = word.replace(/\s/g, '');
const trimmedChosung = chosung.replace(/\s/g, '');

const 유저가_입력한_초성이_단어에_맞는가 = chosungIncludes(trimmedWord, trimmedChosung);

함수 내부에서 처리하지 않기로 한다면, DOCS에 추가 꿀팁 같은것을 남겨도 좋을 것 같아요.

예시

🍯 공백을 포함한 단어도 처리하고 싶다면, 다음과 같이 사용해보세요!

@okinawaa @BO-LIKE-CHICKEN

저는 공백까지 처리하는 것이 함수의 관심사에서 크게 벗어나는 것은 아닌 것 같다고 생각합니다!
#32 에서 논의했던 특수한 문법인 괄호와는 다르게, 띄어쓰기라는 요소는 한글과 떼어내기 어려운 부분이라고 생각해서요.

가령 두 분이 작성해주신 코멘트나 제가 작성하고 있는 이 글만 봐도 공백이 없는 문자열을 찾아볼 수 없다는 것을 알 수 있슴다.

한글이라는 관심사의 특수성

chosungIncludes와 유사한 동작을 가진 String#includes는 공백 여부까지 일치하지 않으면 false를 반환하기는 하는데, 이건 이 친구가 추상 레벨이 한글보다 낮은 "문자열"에 대한 관심사를 다루고 있기 때문이라고 생각해요.

한글을 다루는 녀석이라면 한글 문법에서 필수적인 띄어쓰기까지도 관심사에 포함해야하는 것이 아닌가 하는 생각이 들었습니다.

DX에 대한 Concern

띄어쓰기가 한글에서 떼어내기 힘든 요소라고 가정했을때, 결국 chosungIncludes가 공백을 처리할 수 없다면 한글 문장을 가져와 chosungIncludes를 사용할 때마다 거의 매번 String#replace(/\s/, '') 처리를 해줘야 한다는 것을 의미합니다.

게다가 클라이언트에서 다루는 값은 대부분 어플리케이션 외부 세계인 API 응답, 유저의 입력 등과 같은 곳에서 생성되는 경우가 많으니, 이러한 예외처리를 건너뛸 수 있는 상황도 드물 것 같아요.

이렇게 어떤 함수를 사용하기 전에 매번 예외처리를 해줘야 한다면 생산성 측면에서 DX가 떨어지지는 않을까 하는 우려가 됩니다.

결론

그래서 저는 아래와 같이 chosungIncludes가 두 인자의 공백을 무시하고 결과를 반환해주는 것이 맞지 않을까 생각했슴다 🙇

chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ') // true
chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷㄱㅂㅈ') // true
chosungIncludes('프론트엔드개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ') // true
chosungIncludes('프론트엔드개발자', 'ㅍㄹㅌㅇㄷㄱㅂㅈ') // true
chosungIncludes('프론트엔드 개발자', ' ') // 공백만 있으면 false

[BO-LIKE-CHICKEN]

@evan-moon , @okinawaa 님의 소중한 의견을 모두 귀담아 듣고 다음과 같이 제 의견을 정리해보았습니다.

제가 생각하는chosungIncludes의 개선 방향

기존 이슈를 등록할 때의 의도처럼 공백이 포함된 초성 검색 시 두 인자에 포함된 공백을 무시하고 초성의 일치여부를 검색하면 좋을 것 같습니다. 🙂

아무래도 한글이라는 특수성을 고려하며 다양한 유즈케이스를 생각해 본다면 서비스 개발자의 불필요한 반복을 줄일 수 있는 방향으로 가는것이 좋을 것 같다는 생각이 들었습니다. 특히 가장 많이 사용될 것으로 예상되는 검색이라는 영역에서 더 유리하게 작용할 것 같습니다.

chosungIncludes('반포자이', 'ㅂㅍㅈㅇ') // true
chosungIncludes('반포자이', 'ㅂㅍ ㅈㅇ') // true
chosungIncludes('반포 자이', 'ㅂㅍㅈㅇ') // true
chosungIncludes('반포 자이', 'ㅂㅍ ㅈㅇ') // true

다만, isOnlyInitialConsonant함수의 역할을 유지하며 chosungIncludes에 공백과 상관없이 초성이 포함되었는지 여부를 검사해주도록 변경하면 더욱 좋을 것 같습니다.

개선된 코드

import { disassembleHangulToGroups } from './disassemble';
import { canBeChosung, getFirstConsonants } from './utils';

export function chosungIncludes(x: string, y: string) {
  const trimY = y.replace(/\s/g, '');

  if (!isOnlyInitialConsonant(trimY)) {
    return false;
  }

  const initialConsonantsX = getFirstConsonants(x).replace(/\s/g, '');
  const initialConsonantsY = trimY;

  return initialConsonantsX.includes(initialConsonantsY);
}

/*
 * @description 문자열이 한글초성으로만 주어진 경우
 */
function isOnlyInitialConsonant(str: string) {
  return disassembleHangulToGroups(str).every(disassembled => {
    return disassembled.length === 1 && canBeChosung(disassembled[0]);
  });
}

앞으로의 개선점

• 공백을 포함하여 정확히 일치하는지의 검사도 필요한 경우가 있을 것 같습니다.

chosungIncludes('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷ ㄱㅂㅈ') // true
chosungIncludesExact('프론트엔드 개발자', 'ㅍㄹㅌㅇㄷㄱㅂㅈ') // false

비록 마이너한 케이스이지만, 유즈케이스가 많아진다면 #49 와 함께 개선해보면 좋을 것 같습니다.

---

제가 생각하는chosungIncludes의 개선 방향에 대한 반영은 #42 에 이어서 반영해 두겠습니다.
의견남겨주시면 무척 감사하겠습니다. 🙏🏻

[okinawaa]

@evan-moon 님 @BO-LIKE-CHICKEN 님 의견 모두 동의합니다! 좋은 코맨트들 남겨주셔서 너무 많이 배웠어요!
#42가 머지되어서, 이 이슈는 닫아주셔도 좋을 것 같아요!