feat(utils): chunk 유틸 함수 구현 #184
Conversation
🦋 Changeset detectedLatest commit: e61b7c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 95.23% 95.29% +0.06%
==========================================
Files 79 80 +1
Lines 777 787 +10
Branches 187 190 +3
==========================================
+ Hits 740 750 +10
Misses 31 31
Partials 6 6
|
| export type NaturalNumber<T extends number> = Integer<T> extends never | ||
| ? never | ||
| : `${T}` extends `-${string}` | '0' | ||
| ? never | ||
| : T; |
There was a problem hiding this comment.
Integer, NaturalNumber 모두 멋진 타입이네요!
0을 포함하는 범 자연수 Whole Number도 있으면 좋겠네요 🤗
| export const chunk = <T>(array: T[], size: NaturalNumber<number>): any[][] => { | ||
| return array.reduce((result, _, index) => { | ||
| if (index % size === 0) { | ||
| return [...result, array.slice(index, index + size)]; | ||
| } | ||
| return result; | ||
| }, [] as T[][]); | ||
| }; |
There was a problem hiding this comment.
| export const chunk = <T>(array: T[], size: NaturalNumber<number>): any[][] => { | |
| return array.reduce((result, _, index) => { | |
| if (index % size === 0) { | |
| return [...result, array.slice(index, index + size)]; | |
| } | |
| return result; | |
| }, [] as T[][]); | |
| }; | |
| export const chunk = <T, U extends number>( // (*) | |
| array: T[], | |
| size: NaturalNumber<U> | |
| ): T[][] => { // (*) | |
| if (size >= array.length) { | |
| return [array] as T[][]; // (*) | |
| } | |
| return array.reduce((result, _, index) => { | |
| if (index % size === 0) { | |
| return [...result, array.slice(index, index + size)]; | |
| } | |
| return result; | |
| }, [] as T[][]); | |
| }; |
-
1
size >= array.length조건을 상단에 둔다면 불 필요한 reduce 실행을 방지할 수 있습니다. 곧바로[array]를 반환하면 되는데 굳이 array를 reduce로 순환 할 필요는 없다고 생각합니다 🤗- 고민되는 점은 타입스크립트 기반이고, 자연수만 넣을 수 있게끔 타입을 지정하는 상황이지만, 자바스크립트 환경 기반에서
size 1 미만에대한 처리가 필요한지 고민이네요 🤔 - size가 0이라면 빈 배열 반환 정도(?) 생각해봤습니다. 이것도 위와 같이 불 필요한 reduce 호출을 방지하기 위함입니다.
- 고민되는 점은 타입스크립트 기반이고, 자연수만 넣을 수 있게끔 타입을 지정하는 상황이지만, 자바스크립트 환경 기반에서
-
2 제네릭 타입
U가 필요합니다. 기존 코드로는NaturalNumber<number>로 size 타입이 고정되어 있으므로 chunk의 두 번째 인자에음수,0,소수,NaN을 넣어도 모두 타입 통과합니다.- NaturalNumber을 사용하신 의도가 이런걸 방지하기 위함이라고 생각해서 말씀드려봅니다 ! 🙏
-
3 반환 타입이
any[][]가 아닌 더 명확한T[][]가 되어야 할 것 같습니다 🤗
There was a problem hiding this comment.
처음에는 reduce 내부에 전개 연산자, slice 등이 사용되서 시간복잡도상 성능에 안좋을 것으로 예상했는데요
그래서 toss slash의 chunk 와 비교해서 테스트를 진행해 봤습니다.
- chunk1이 @Collection50 님이 제안주신 코드
- chunk2가 slash chunk 코드
테스트 코드 결과에는 여러 영향이 있겠지만 현재로서는 제안주신 코드가 더 효율적으로 판단되네요 👍
추후에 조금 더 살펴보면서 더 좋은 방향이 있을지 고민해봐도 좋을 것 같습니다 🤗
There was a problem hiding this comment.
1 size >= array.length 조건을 상단에 둔다면 불 필요한 reduce 실행을 방지할 수 있습니다. 곧바로 [array]를 반환하면 되는데 굳이 array를 reduce로 순환 할 필요는 없다고 생각합니다 🤗
위 조건문은 좋은 것 같습니다 !
추가해두겠습니다.
There was a problem hiding this comment.
size 1 미만에대한 처리가 필요한지 고민이네요 🤔
size 1 미만에 대한 처리를 따로하지 않고 가독성 향상을 위해 NaturalNumber 타입을 구현했는데 어떻게 생각하시나요??
There was a problem hiding this comment.
제네릭 타입 U가 필요합니다. 기존 코드로는 NaturalNumber로 size 타입이 고정되어 있으므로 chunk의 두 번째 인자에 음수, 0, 소수, NaN을 넣어도 모두 타입 통과합니다.
이 타입은 제가 작성해두고... 테스트까지 해두었는데 빠트린 부분이네요... 😳
다시 수정해두겠습니다.
There was a problem hiding this comment.
3 반환 타입이 any[][]가 아닌 더 명확한 T[][]가 되어야 할 것 같습니다 🤗
넵 !
There was a problem hiding this comment.
테스트 코드 결과에는 여러 영향이 있겠지만 현재로서는 제안주신 코드가 더 효율적으로 판단되네요 👍
추후에 조금 더 살펴보면서 더 좋은 방향이 있을지 고민해봐도 좋을 것 같습니다 🤗
가독성이 좋은 코드로 작성하고자 했는데 유틸 함수의 경우 성능도 중요하게 생각하겠습니다 !
|
@Collection50 아래는 최종 제안드리는 코드입니다. export const chunk = <T, U extends number>(
array: T[],
size: NaturalNumber<U>
): T[][] => {
if (array.length === 0 || size === 0) {
return [];
}
if (size >= array.length) {
return [array];
}
return array.reduce((result, _, index) => {
if (index % size === 0) {
return [...result, array.slice(index, index + size)];
}
return result;
}, [] as T[][]);
}; |
|
@Collection50 참고로, 둘 의 성능은 거의 차이가 없었습니다 export const chunk = <T, U extends number>(
array: T[],
size: NaturalNumber<U>
): T[][] => {
if (array.length === 0 || size === 0) {
return [];
}
if (size >= array.length) {
return [array];
}
return array.reduce((result, _, index) => {
if (index % size === 0) {
result.push(array.slice(index, index + size));
}
return result;
}, [] as T[][]);
}; |
성능 차이가 없다면 매개변수를 변경하는 안티패턴을 사용하지 않는 것이 좋다고 생각합니다. |
|
test('2번째 매개변수가 0인 경우 빈 배열을 반환한다.', () => {
const arr = [1, 2];
expect(chunk(arr, 0 as number)).toEqual([]);
});또한 테스트코드에서 |
|
@Collection50
혹시 매개변수를 바꿔 안티패턴이라는 부분이 reduce의 accumulator 변수의 변화를 말씀하시는걸 제가 정확한게 이해한게 맞을까요? 😲 chunk의 인자로 받은 array에 대한 변화가 있다면, 사이드 이펙트가 발생해 말씀주신 안티 패턴이라고 생각이 들긴하는데, 현재 코드에서 accmulator 변수의 변화는 크게 문제가 없지 않나요? (사이드 이펙트 위험이 있는 array를 다루는 경우에는 slice를 활용중) 저는 두 가지 방법도 크게 상관은 없지만, 다양한 의견이 궁금해서 여쭤봅니다 !! ㅎㅎㅎ 스택 오버플로우에 관련 내용이 있으니 한번 참고해보셔도 좋을 것 같습니다! 추가 피드백주시면 감사드립니다 👍🤗 |
네 좋습니다 감사합니다 :) 단, 다른 테스트 코드들과 통일성을 위해
|
|
@ssi02014 테스트 코드 또한 변경하겠습니다 ! |
packages/types/src/Integer/index.ts
Outdated
| @@ -0,0 +1,5 @@ | |||
| export type Integer<T extends number> = `${T}` extends | |||
There was a problem hiding this comment.
먼저 좋은 작업 감사합니다! 👍
한가지 궁금한점은 Integer에서 체크하고자하는 'NaN'이 제대로 체킹되시는지 궁금합니다!
저도 기존에 NaN을 타입레벨에서 정확하게 잡아내는게 불가능한 것으로 알고있었고, 실제로 TS Playground에서 실행해보니 에러 체킹이 안되는 것으로 보여서요! (제가 잘못 시도했거나 모르는 부분이 있는거라면 알려주시면 감사드리겠습니다! 😄 )
There was a problem hiding this comment.
위의 체킹이 제대로 이루어지지 않는 것이 맞다면, 함수 내에서 NaN에 대한 방어코드를 작성해두는 것이 좋을듯하여 의견드려봅니다! 😄
// 현재 코드를 기준
// NaN 케이스에 대한 방어코드 추가
if(isNaN(size)) {
return []
}
if (array.length === 0 || size === 0) {
return [];
}There was a problem hiding this comment.
완전 여담으로 위의 발상이 시작된 지점은 NatualNumber타입에 Infinity와 -Infinity가 존재하지 않아 생각했었습니다!
NaN, Inifinity, -Infinity 모두 number 타입이나 타입레벨에서 정확하게 찾아내는건 불가능한것으로 알고있어서요! 😭
There was a problem hiding this comment.
제가 확인하지 못한 부분이 있었네요 !
코드를 리뷰해주셔서 감사합니다 🙌
혹시 NaN을 아래와 같이 사용하는 것은 어떻게 생각하시나요?
if(Number.isNaN(size)) {
return [array];
}Number.isNaN은 인수를Number타입으로 변환하지 않습니다.- 더 좁은 타입사용이 가능합니다. (
size를U타입으로 선언해두어서 괜찮을 것 같기도 합니다 !)
- 더 좁은 타입사용이 가능합니다. (
array를 반환하는것
There was a problem hiding this comment.
완전 여담으로 위의 발상이 시작된 지점은 NatualNumber타입에 Infinity와 -Infinity가 존재하지 않아 생각했었습니다! NaN, Inifinity, -Infinity 모두 number 타입이나 타입레벨에서 정확하게 찾아내는건 불가능한것으로 알고있어서요! 😭
새로운 사실을 알게되어 기쁩니다 !
There was a problem hiding this comment.
@Collection50 1, 2번 의견 주신 부분에 동의합니다! 간단하게 생각했던 의견이었는데 더 좋은 방향으로 제시해주셔서 감사합니다! 👍👍👍
NaN, Infinity, -Infinity 에 대한 부분은 저도 현재는 이렇게 알고있으나 잘못 알고있는 부분일수도 있습니다 😂
There was a problem hiding this comment.
@Collection50 @Sangminnn 제가 지금 외부라 확인이 좀 늦었습니다! 위 의견들 모두 확인했고 너무 좋았는데요!
마지막으로 NaN의 경우 [array]를 반환하는 의도가 있을까요?
NaN이 넘어온다는게 적절한 케이스는 아니고 예외 케이스라고 생각이 드는데요! 예외 케이스라는 의미에서 빈 배열([])을 반환하는게 낫지 않을까요?
- 현재
[array]를 반환한다는 것은size가 array.length와 동일하거나 그 이상이라는 뭔가 특정 케이스가 존재합니다. - NaN의 경우에는
size가 0(예외 케이스라고 생각한 이유는 size가 0이라면 사실 chunk를 사용 할 이유가 없기 때문)과 같이 뭔가예외 케이스라 생각이들어서요! 그런 의미에서 빈 배열이 낫지않나 의견드려봅니다
추가로 테스트 커버리지를 위해 NaN의 경우 테스트 코드가 추가되면 좋을 것 같습니다 !!
There was a problem hiding this comment.
이 부분의 경우 최초에는 작성자분이 의도하신 바가 있는 것으로 생각했었습니다만 말씀주신 것을 들어보면 정리가 필요할 수 있겠네요!
다시 생각해보면 처리 과정에서 특정 케이스를 어떻게 분류하고 처리하는지에 대해 차이가 있는것처럼 보여서 그런것같습니다.
현재는 NaN이라는 케이스와 0이 들어오는 케이스를 다르게 처리하고있으나 NaN의 케이스와 0이 들어오는 케이스 모두를 의도가 아닌 예상치 못하게 들어오는 케이스로 분류하게 된다면, 둘을 동일하게 처리하는게 더 자연스럽겠네요! 😄
저도 위에 남겨주신 의견 이외에는 작성해주신 부분은 다 너무 좋습니다! 고생많으셨습니다! 👍👍
There was a problem hiding this comment.
@Collection50 @Sangminnn
맞습니다 저는 array.length 0, size 0과 NaN을 모두 예외 케이스로 묶고 동일하게 처리하면 어떨까 생각했습니다.
위 3가지 케이스는 모두 chunk를 사용 할 적절한 케이스가 아니기 때문입니다..!!
There was a problem hiding this comment.
좋습니다 !!
변경된 사항 적용했습니다. (4fc45df)
|
최종적으로 코드 수정했습니다 ! |
packages/types/src/Integer/index.ts
Outdated
| @@ -0,0 +1,5 @@ | |||
| export type Integer<T extends number> = `${T}` extends | |||
| | `${string}.${string}` | |||
| | 'NaN' | |||
There was a problem hiding this comment.
NaN이 정확한 타입 추론이 안된다면 제거해도 될 것 같기도 하네요 !
Overview
Issue: #158
chunk메서드를 구현했습니다. (d815021)Integer,NaturalNumber타입을 함께 구현했습니다. (8aa958e)close #158
PR Checklist
Contributing Guide