Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: amountToHangul이 선행 0이 있는 문자열을 잘못 변환하는 오류를 수정합니다. #168

Merged
merged 10 commits into from
Jul 12, 2024

Conversation

jungwoo3490
Copy link
Contributor

@jungwoo3490 jungwoo3490 commented Jul 4, 2024

Overview

resolve #167

Issue에도 설명되었듯이, amountToHangul이 선행 0이 있는 문자열을 처리할 경우 의도치 않은 동작을 하는 것을 확인했어요.

image

이를 해결하기 위해 다음과 같이 integerPart의 선행 0을 모두 제거하는 로직을 추가했어요.

const integerPart = tempIntegerPart !== '0' ? tempIntegerPart.replace(/^0+/, '') : tempIntegerPart;

여기서 '0'인지 검사하는 이유는, tempDecimalPart가 존재하지 않는 상황에서 integerPart가 위 정규표현식에 의해 제거되어 빈 문자열이 되게 되면

if (integerPart === '0' || (integerPart === '' && tempDecimalPart))

이 조건식이 false가 되어 결국 '0'일 경우 "영"이 아닌 빈 문자열로 변환을 하게 되는 문제가 발생하는 것을 확인했어요.

그래서 '0'이 입력으로 들어왔을 경우만 선행 0을 제거하는 정규표현식이 동작하지 않도록 해주었습니다.

PR Checklist

  • I read and included theses actions below
  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.

Copy link

changeset-bot bot commented Jul 4, 2024

🦋 Changeset detected

Latest commit: 4a615ae

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

Copy link

vercel bot commented Jul 4, 2024

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

Name Status Preview Comments Updated (UTC)
es-hangul ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 12, 2024 3:06pm

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (8f7cc9a) to head (d5da627).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #168   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines          260       261    +1     
  Branches        58        59    +1     
=========================================
+ Hits           260       261    +1     

Comment on lines 29 to 33
const [tempIntegerPart, tempDecimalPart] = String(amount)
.replace(/[^\d.]+/g, '')
.split('.');

const integerPart = tempIntegerPart !== '0' ? tempIntegerPart.replace(/^0+/, '') : tempIntegerPart;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요렇게 temp가 아닌 조금 더 명확한 이름을 사용해보면 어떤가요?
또한 decimal같은경우는 temp가 붙을 필요가 없을것 같기두 해요!

Suggested change
const [tempIntegerPart, tempDecimalPart] = String(amount)
.replace(/[^\d.]+/g, '')
.split('.');
const integerPart = tempIntegerPart !== '0' ? tempIntegerPart.replace(/^0+/, '') : tempIntegerPart;
const [rawIntegerPart, decimalPart] = String(amount)
.replace(/[^\d.]+/g, '')
.split('.');
const integerPart = rawIntegerPart !== '0' ? rawIntegerPart.replace(/^0+/, '') : rawIntegerPart;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요렇게 temp가 아닌 조금 더 명확한 이름을 사용해보면 어떤가요? 또한 decimal같은경우는 temp가 붙을 필요가 없을것 같기두 해요!

훨씬 좋은 것 같아요!!

demicalPart의 경우에도 소수부 뒷자리의 0을 제거하는 정규표현식을 한 번 거치는 과정이 있더라구요!

const decimalPart = tempDecimalPart?.replace(/0+$/, '');

이 부분도 tempDemicalPart 대신 rawDemicalPart로 네이밍하는 게 더 좋을 것 같네요 ☺️

@jungwoo3490
Copy link
Contributor Author

@okinawaa 리뷰 반영 완료했습니다!! 확인 부탁드려요 :)

Copy link
Member

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다~!

@okinawaa okinawaa merged commit e3d0259 into toss:main Jul 12, 2024
2 of 10 checks passed
@github-actions github-actions bot mentioned this pull request Jul 12, 2024
@github-actions github-actions bot mentioned this pull request Jul 12, 2024
okinawaa added a commit that referenced this pull request Jul 13, 2024
* fix: 호환성을 위해 배열의 마지막 요소 가져오는 부분 수정

* fix: 선행 0도 처리 가능하도록 amountToHangul 수정

* test: 선행 0 처리 테스트 코드 추가

* fix: 변수 네이밍 수정

* Create curvy-bats-nail.md

---------

Co-authored-by: 박찬혁 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: amountHangul이 선행 0이 있는 문자열을 잘못 변환합니다.
3 participants