fix: 코드 효율 개선 #124

crucifyer · 2024-06-18T17:32:11Z

Overview

정규식 효율개선, 불필요한 계산의 생략을 했습니다.
각각의 commit 메세지를 참조해주세요.

PR Checklist

[v] I read and included theses actions below

I have read the Contributing Guide
I have written documents and tests, if needed.

…74b5d1h/

changeset-bot · 2024-06-18T17:32:14Z

🦋 Changeset detected

Latest commit: 5e122b2

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2024-06-18T17:32:15Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
es-hangul	❌ Failed (Inspect)			Jun 29, 2024 11:34am

okinawaa · 2024-06-22T14:29:58Z

src/removeLastHangulCharacter.ts

@@ -20,25 +20,13 @@ import { excludeLastElement } from './_internal';
 * removeLastHangulCharacter('신세계') // 신세ㄱ
 */
 export function removeLastHangulCharacter(words: string) {


이 파일에 대해서 개선은, 성능 뿐만 아니라, 가독성도 크게 개선되었네요!

okinawaa · 2024-06-22T14:41:25Z

src/utils.ts

+  if (lastChar == null) {
+    return false;
+  }
+  const charCode = lastChar.charCodeAt(0);
+  const isCompleteHangul = COMPLETE_HANGUL_START_CHARCODE <= charCode && charCode <= COMPLETE_HANGUL_END_CHARCODE;
+
+  if (!isCompleteHangul) {
    return false;
  }

-  const disassembled = disassembleHangul(lastChar);
-  return disassembled.length === 3;
+  const batchimCode = (charCode - COMPLETE_HANGUL_START_CHARCODE) % NUMBER_OF_JONGSUNG;
+  return HANGUL_CHARACTERS_BY_LAST_INDEX[batchimCode].length === 1;


이 부분은 어떠한 이유로 성능 개선이 있는지 궁금해요.

코드의 복잡도 향상에 비해 성능 개선이 더 큰 이점인가요~?

hasSingleBatchim 에서 hasBatchim 을 이용하는데, disassembleHangul 을 반복 호출하게 됩니다.
그게 아까워서 고쳐본건데, 리턴 타입 등 사용방법 자체를 바꾸는건 안될 것 같아서 이렇게 했습니다.
리턴 타입을 바꿀 수 있다면, hasBatchim 의 리턴을 int 로 하고 받침의 개수로 리턴하게 하는것을 제안합니다.
=== 1 이면 hasSingleBatchim 이고 > 0 이면 hasBatchim 인거죠.

자세하게 답변주셔서 감사해요! 이해했습니다~!

okinawaa · 2024-06-22T14:44:29Z

src/utils.ts

-  return disassembleHangulToGroups(word).reduce((chosung, [consonant]) => {
-    return `${chosung}${consonant}`;
-  }, '');
+  return word.normalize('NFD')


이 부분은 어떠한 이유로 성능 개선이 있는지 궁금해요.

코드의 복잡도 향상에 비해 성능 개선이 더 큰 이점인가요~?
word.normalize라는 메서드도 생각보다 내부적으로 복잡하게 구현되어있다면 성능의 이점이 있을까해서요!

js normalize 성능은 믿어도 될겁니다. :)

okinawaa · 2024-06-22T15:23:43Z

src/utils.ts

+  if (lastChar == null) {
+    return false;
+  }
+  const charCode = lastChar.charCodeAt(0);
+  const isCompleteHangul = COMPLETE_HANGUL_START_CHARCODE <= charCode && charCode <= COMPLETE_HANGUL_END_CHARCODE;
+
+  if (!isCompleteHangul) {
    return false;
  }

-  const disassembled = disassembleHangul(lastChar);
-  return disassembled.length === 3;
+  const batchimCode = (charCode - COMPLETE_HANGUL_START_CHARCODE) % NUMBER_OF_JONGSUNG;
+  return HANGUL_CHARACTERS_BY_LAST_INDEX[batchimCode].length === 1;


자세하게 답변주셔서 감사해요! 이해했습니다~!

okinawaa · 2024-06-22T15:32:23Z

src/utils.ts

+    .replace(/[^\u1100-\u1112\u3130-\u314e\s]+/ug, '') // NFD ㄱ-ㅎ, NFC ㄱ-ㅎ 외 문자 삭제
+    .replace(/[\u1100-\u1112]/g, $0 => HANGUL_CHARACTERS_BY_FIRST_INDEX[$0.charCodeAt(0) - 0x1100]); // NFD to NFC


요, 숫자들이 지금은 이해하기 쉽지만, 나중에 이 코드를 유지보수 하시는 분들은 이해하기 어려울 것 같은데요, 주석이 있긴 하지만요. 주석 대신 상수화를 통해 명확히 표현하는 것은 어떤가요?

const ㄱ유니코드 = 0x1100; const ㅎ유니코드 = 0x1112; const ㄱNFC유니코드 = 0x3130; const ㅎNFC유니코드 = 0x314e;

마지막 0x1100 외에는 정규식이라 new RegExp() 로 바꿔야 상수를 사용할 수 있을 것 같은데,
거기까지는 안하고 상수 정의까지만 하는것도 좋을 것 같네요.

constants 에 있는것과 형식을 맞춰보려 했는데, 이렇게 하는게 맞을까요? ;;;

export const HANGUL_NFD_CHOSUNG_START_CHARCODE = '가'.normalize('NFD').charCodeAt(0); // 'ㄱ' 0x1100 export const HANGUL_NFD_CHOSUNG_END_CHARCODE = '하'.normalize('NFD').charCodeAt(0); // 'ㅎ' 0x1112

요렇게 생각해봤어요!
리팩토링해도, 여전히 복잡한 부분은 있지만, 각 절차가 상수나 함수명에 드러나서 보다 읽기 편한 것 같아용!

const HANGUL_UNICODE_RANGE = { NFD_START: 0x1100, NFD_END: 0x1112, NFC_START: 0x3130, NFC_END: 0x314e, }; const NFD_REGEX = new RegExp( `[^\\u${HANGUL_UNICODE_RANGE.NFD_START.toString(16)}-\\u${HANGUL_UNICODE_RANGE.NFD_END.toString(16)}\\u${HANGUL_UNICODE_RANGE.NFC_START.toString(16)}-\\u${HANGUL_UNICODE_RANGE.NFC_END.toString(16)}\\s]+`, 'ug' ); const NFD_TO_NFC_REGEX = new RegExp( `[\\u${HANGUL_UNICODE_RANGE.NFD_START.toString(16)}-\\u${HANGUL_UNICODE_RANGE.NFD_END.toString(16)}]`, 'g' ); export function getChosung(word: string) { return word .normalize('NFD') .replace(NFD_REGEX, '') // NFD ㄱ-ㅎ, NFC ㄱ-ㅎ 외 문자 삭제 .replace( NFD_TO_NFC_REGEX, $0 => HANGUL_CHARACTERS_BY_FIRST_INDEX[$0.charCodeAt(0) - HANGUL_UNICODE_RANGE.NFD_START] ); // NFD to NFC }

range 부분은 constants.ts 에 맞춰서 넣어보려는데 어떤가요?

const _JASO_HANGUL_NFD = [...'각힣'.normalize('NFD')].map(char => char.charCodeAt(0)); // NFC 에 정의되지 않은 문자는 포함하지 않음 export const JASO_HANGUL_NFD = { START_CHOSUNG: _JASO_HANGUL_NFD[0], // ㄱ START_JUNGSUNG: _JASO_HANGUL_NFD[1], // ㅏ START_JONGSUNG: _JASO_HANGUL_NFD[2], // ㄱ END_CHOSUNG: _JASO_HANGUL_NFD[3], // ㅎ END_JUNGSUNG: _JASO_HANGUL_NFD[4], // ㅣ END_JONGSUNG: _JASO_HANGUL_NFD[5], // ㅎ }

추후에도 자주 사용 될 것 같은 상수들이라면 좋은 것 같아요!

적용했습니다.

codecov-commenter · 2024-06-25T15:12:04Z

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.72%. Comparing base (db94449) to head (ed2b72d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
- Coverage   99.56%   98.72%   -0.85%     
==========================================
  Files          14       14              
  Lines         230      235       +5     
  Branches       51       53       +2     
==========================================
+ Hits          229      232       +3     
- Misses          1        3       +2

okinawaa

감사합니다!

crucifyer added 4 commits June 19, 2024 01:56

fix: 모든 글자의 자소를 나누지 않고 마지막 글자만 계산합니다.

91377c5

fix: /\s/g to /\s+/g 일괄치환 효율 증가 bench: https://jsfiddle.net/crucify/g…

2a84713

…74b5d1h/

fix: NFD 로 변환하여 자소계산 과정 생략하고 초성 ㄱ-ㅎ 만 남기게 수정

259c226

fix: 자소계산을 생략하고 받침이 있는지만 검사합니다.

fd83fb3

crucifyer requested review from okinawaa and raon0211 as code owners June 18, 2024 17:32

vercel bot had a problem deploying to Preview June 18, 2024 17:32 Failure

okinawaa reviewed Jun 22, 2024

View reviewed changes

crucifyer added 2 commits June 26, 2024 00:09

fix: 유니코드 상수화 가독성 향상

7ca5521

fix: 기존함수 받침문제 테스트 추가 (이 브랜치에서 해결됨)

3965be4

vercel bot had a problem deploying to Preview June 25, 2024 15:11 Failure

Merge branch 'toss:main' into code-optimize

ed2b72d

vercel bot had a problem deploying to Preview June 25, 2024 15:14 Failure

okinawaa previously approved these changes Jun 29, 2024

View reviewed changes

Create sixty-clouds-repeat.md

5e122b2

okinawaa dismissed their stale review via 5e122b2 June 29, 2024 11:34

vercel bot had a problem deploying to Preview June 29, 2024 11:34 Failure

okinawaa approved these changes Jun 29, 2024

View reviewed changes

okinawaa merged commit 0f38431 into toss:main Jun 29, 2024
9 of 10 checks passed

github-actions bot mentioned this pull request Jun 29, 2024

chore: version packages #143

Merged

jgjgill mentioned this pull request Jun 30, 2024

[Bug]: removeLastHangulCharacter 함수에서 disassembledGroups의 길이가 4개인 경우 에러가 발생합니다. #123

Closed

okinawaa mentioned this pull request Jul 5, 2024

fix. 겹모음과 관련하여 일부 메소드에서 잘못된 동작을 수정합니다. #156

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: 코드 효율 개선 #124

fix: 코드 효율 개선 #124

crucifyer commented Jun 18, 2024

changeset-bot bot commented Jun 18, 2024 •

edited

Loading

vercel bot commented Jun 18, 2024 •

edited

Loading

okinawaa Jun 22, 2024

okinawaa Jun 22, 2024

crucifyer Jun 22, 2024

okinawaa Jun 22, 2024

okinawaa Jun 22, 2024

crucifyer Jun 22, 2024

okinawaa Jun 22, 2024

okinawaa Jun 22, 2024

crucifyer Jun 22, 2024

crucifyer Jun 22, 2024

okinawaa Jun 23, 2024

crucifyer Jun 24, 2024

okinawaa Jun 24, 2024

crucifyer Jun 25, 2024

codecov-commenter commented Jun 25, 2024 •

edited

Loading

okinawaa left a comment

		.replace(/[^\u1100-\u1112\u3130-\u314e\s]+/ug, '') // NFD ㄱ-ㅎ, NFC ㄱ-ㅎ 외 문자 삭제
		.replace(/[\u1100-\u1112]/g, $0 => HANGUL_CHARACTERS_BY_FIRST_INDEX[$0.charCodeAt(0) - 0x1100]); // NFD to NFC

fix: 코드 효율 개선 #124

fix: 코드 효율 개선 #124

Conversation

crucifyer commented Jun 18, 2024

Overview

PR Checklist

changeset-bot bot commented Jun 18, 2024 • edited Loading

🦋 Changeset detected

vercel bot commented Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 25, 2024 • edited Loading

Codecov Report

okinawaa left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Jun 18, 2024 •

edited

Loading

vercel bot commented Jun 18, 2024 •

edited

Loading

codecov-commenter commented Jun 25, 2024 •

edited

Loading