ronantakizawa/codereview-bench huggingface dataset:
Repository: toss/es-toolkit
File: src/compat/predicate/isUndefined.spec.ts (TypeScript)

Code before:
import { describe, expect, it } from 'vitest';
import { isUndefined } from '../../predicate';
import { falsey } from '../_internal/falsey';

describe('isUndefined', () => {
  it('should return `true` for `undefined` values', () => {
    expect(isUndefined(undefined)).toBe(true);
  });

  it('should return `false` for non `undefined` values', () => {
    const expected = falsey.map(value => value === undefined);
    const actual = falsey.map((value, index) => (index ? isUndefined(value) : isUndefined(undefined)));
    expect(actual).toEqual(expected);

    (function (..._: any[]) {
      // eslint-disable-next-line prefer-rest-params
      expect(isUndefined(arguments)).toBe(false);
    })(1, 2, 3);
    expect(isUndefined([1, 2, 3])).toBe(false);
    expect(isUndefined(true)).toBe(false);
    expect(isUndefined(new Date())).toBe(false);
    expect(isUndefined(new Error())).toBe(false);
    expect(isUndefined(Array.prototype.slice)).toBe(false);
    expect(isUndefined({ a: 1 })).toBe(false);
    expect(isUndefined(1)).toBe(false);
    expect(isUndefined(/x/)).toBe(false);
    expect(isUndefined('a')).toBe(false);
    expect(isUndefined(Symbol('a'))).toBe(false);
  });
});


Review comment (suggestion):
```suggestion
    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    (function (..._: any[]) {
```

Code after:
import { describe, expect, it } from 'vitest';
import { isUndefined } from '../../predicate';
import { falsey } from '../_internal/falsey';

describe('isUndefined', () => {
  it('should return `true` for `undefined` values', () => {
    expect(isUndefined(undefined)).toBe(true);
  });

  it('should return `false` for non `undefined` values', () => {
    const expected = falsey.map(value => value === undefined);
    const actual = falsey.map((value, index) => (index ? isUndefined(value) : isUndefined(undefined)));
    expect(actual).toEqual(expected);

    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    (function (..._: any[]) {
      // eslint-disable-next-line prefer-rest-params
      expect(isUndefined(arguments)).toBe(false);
    })(1, 2, 3);
    expect(isUndefined([1, 2, 3])).toBe(false);
    expect(isUndefined(true)).toBe(false);
    expect(isUndefined(new Date())).toBe(false);
    expect(isUndefined(new Error())).toBe(false);
    expect(isUndefined(Array.prototype.slice)).toBe(false);
    expect(isUndefined({ a: 1 })).toBe(false);
    expect(isUndefined(1)).toBe(false);
    expect(isUndefined(/x/)).toBe(false);
    expect(isUndefined('a')).toBe(false);
    expect(isUndefined(Symbol('a'))).toBe(false);
  });
});
Repository: toss/es-toolkit
File: src/function/throttle.ts (TypeScript)

Code before:
 * @param {number} throttleMs - The number of milliseconds to throttle executions to.
 * @returns {F} A new throttled function that accepts the same parameters as the original function.
 *
 * @example
 * const throttledFunction = throttle(() => {
 *   console.log('Function executed');
 * }, 1000);
 *
 * // Will log 'Function executed' immediately
 * throttledFunction();
 *
 * // Will not log anything as it is within the throttle time
 * throttledFunction();
 *
 * // After 1 second
 * setTimeout(() => {
 *   throttledFunction(); // Will log 'Function executed'
 * }, 1000);
 */
export function throttle<F extends (...args: any[]) => void>(func: F, throttleMs: number): F {
  let lastCallTime: number | null;

  const throttledFunction = function (...args: Parameters<F>) {
    const now = Date.now();

    if (lastCallTime === null || now - lastCallTime >= throttleMs) {
      lastCallTime = now;
      func(...args);
    }
  } as F;

  return throttledFunction;
}


Review comment (suggestion):
```suggestion
    if (lastCallTime == null || now - lastCallTime >= throttleMs) {
```

Code after:
 * @param {number} throttleMs - The number of milliseconds to throttle executions to.
 * @returns {F} A new throttled function that accepts the same parameters as the original function.
 *
 * @example
 * const throttledFunction = throttle(() => {
 *   console.log('Function executed');
 * }, 1000);
 *
 * // Will log 'Function executed' immediately
 * throttledFunction();
 *
 * // Will not log anything as it is within the throttle time
 * throttledFunction();
 *
 * // After 1 second
 * setTimeout(() => {
 *   throttledFunction(); // Will log 'Function executed'
 * }, 1000);
 */
export function throttle<F extends (...args: any[]) => void>(func: F, throttleMs: number): F {
  let lastCallTime: number | null;

  const throttledFunction = function (...args: Parameters<F>) {
    const now = Date.now();

    if (lastCallTime == null || now - lastCallTime >= throttleMs) {
      lastCallTime = now;
      func(...args);
    }
  } as F;

  return throttledFunction;
}
Repository: toss/es-toolkit
File: src/predicate/isSymbol.ts (TypeScript)

Code before:
import { getTag } from '../compat/_internal/getTag';

/**
 * Check whether a value is a symbol.
 *
 * This function can also serve as a type predicate in TypeScript, narrowing the type of the argument to `symbol`.
 *
 * @param {unknown} value The value to check.
 * @returns {value is symbol} Returns `true` if `value` is a symbol, else `false`.
 */
export function isSymbol(value: unknown): value is symbol {
  return (
    typeof value === 'symbol' || (value !== null && typeof value === 'object' && getTag(value) === '[object Symbol]')
  );
}


Review comment (bug):
While this is great, I think supporting `Object(symbol)`s is not desirable in our library (since I think it is an edge case, and it makes our implementation complicated).

Why not add this branch in our compatibility library, `es-toolkit/compat`?

For example, we have two versions of `isBoolean`:

- [one](https://github.com/toss/es-toolkit/blob/main/src/predicate/isBoolean.ts) in our original library
- [another](https://github.com/toss/es-toolkit/blob/main/src/compat/predicate/isBoolean.ts) in our compatibility library.

Code after:
/**
 * Check whether a value is a symbol.
 *
 * This function can also serve as a type predicate in TypeScript, narrowing the type of the argument to `symbol`.
 *
 * @param {unknown} value The value to check.
 * @returns {value is symbol} Returns `true` if `value` is a symbol, else `false`.
 */
export function isSymbol(value: unknown): value is symbol {
  return typeof value === 'symbol';
}
Repository: toss/es-toolkit
File: src/array/_internal/compareValues.spec.ts (TypeScript)

Code before:
    expect(compareValues(2, 1, 'asc')).toBe(1);
  });

  it('should return 0 if a === b', () => {
    expect(compareValues(1, 1, 'asc')).toBe(0);
  });

  it('should return 1 if a < b and order is desc', () => {
    expect(compareValues(1, 2, 'desc')).toBe(1);
  });

  it('should return -1 if a > b and order is desc', () => {
    expect(compareValues(2, 1, 'desc')).toBe(-1);
  });

  it('should return 0 if a === b and order is desc', () => {
    expect(compareValues(1, 1, 'desc')).toBe(0);
  });

  it('should handle the case where a is string and b is number', () => {
    const a = ['a', '1b', '3', '2'];
    const actual = a.map(value => compareValues(value, 1, 'asc'));
    const expected = a.map(() => 1);

    expect(actual).toEqual(expected);
  });

  it('should handle the case where a is number and b is string', () => {
    const b = ['a', '1b', '3', '2'];
    const actual = b.map(value => compareValues(1, value, 'asc'));
    const expected = b.map(() => -1);

    expect(actual).toEqual(expected);
  });

  it('should return 0 if a and b are not comparable', () => {
    expect(compareValues({ a: 'number' }, 1, 'asc')).toBe(0);
  });
});


Review comment (suggestion):
I guess we could improve the test code like this:

```tsx
  it('should compare numbers and strings by converting number into a string', () => {
    expect(compareValues('0', 1, 'asc')).toBe(-1);
    expect(compareValues('a', 1, 'asc')).toBe(1);
    expect(compareValues('1b', 1, 'asc')).toBe(1);
    expect(compareValues('3', 1, 'asc')).toBe(1);
    expect(compareValues('2', 1, 'asc')).toBe(1);
   });
```

I think it is much easier to understand, and it is more comprehensive (since we handle the (`'0'` case)

Code after:
    expect(compareValues(2, 1, 'asc')).toBe(1);
  });

  it('should return 0 if a === b', () => {
    expect(compareValues(1, 1, 'asc')).toBe(0);
  });

  it('should return 1 if a < b and order is desc', () => {
    expect(compareValues(1, 2, 'desc')).toBe(1);
  });

  it('should return -1 if a > b and order is desc', () => {
    expect(compareValues(2, 1, 'desc')).toBe(-1);
  });

  it('should return 0 if a === b and order is desc', () => {
    expect(compareValues(1, 1, 'desc')).toBe(0);
  });

  it('should handle the case where a is string and b is number', () => {
    expect(compareValues('0', 1, 'asc')).toBe(-1);
    expect(compareValues('a', 1, 'asc')).toBe(1);
    expect(compareValues('1b', 1, 'asc')).toBe(1);
    expect(compareValues('3', 1, 'asc')).toBe(1);
    expect(compareValues('2', 1, 'asc')).toBe(1);
  });

  it('should handle the case where a is number and b is string', () => {
    expect(compareValues(1, '0', 'asc')).toBe(1);
    expect(compareValues(1, 'a', 'asc')).toBe(-1);
    expect(compareValues(1, '1b', 'asc')).toBe(-1);
    expect(compareValues(1, '3', 'asc')).toBe(-1);
    expect(compareValues(1, '2', 'asc')).toBe(-1);
  });

  it('should return 0 if a and b are not comparable', () => {
    expect(compareValues({ a: 'number' }, 1, 'asc')).toBe(0);
  });
});
Repository: toss/es-toolkit
File: src/predicate/index.ts (TypeScript)

Code before:
export { isEqual } from './isEqual.ts';
export { isNil } from './isNil.ts';
export { isNotNil } from './isNotNil.ts';
export { isNull } from './isNull.ts';
export { isUndefined } from './isUndefined.ts';
export { isLength } from './isLength.ts';
export { isPlainObject } from './isPlainObject.ts';
export { isFunction } from './isFunction.ts';
export { isTypedArray } from './isTypedArray.ts';
export { isPrimitive } from './isPrimitive.ts';
export { isBoolean } from './isBoolean.ts';
export { isSymbol } from './isSymbol.ts';
export { isString } from './isString.ts';

Review comment (suggestion):
```suggestion
export { isString } from './isString.ts';

```

Code after:
export { isEqual } from './isEqual.ts';
export { isNil } from './isNil.ts';
export { isNotNil } from './isNotNil.ts';
export { isNull } from './isNull.ts';
export { isUndefined } from './isUndefined.ts';
export { isLength } from './isLength.ts';
export { isPlainObject } from './isPlainObject.ts';
export { isFunction } from './isFunction.ts';
export { isTypedArray } from './isTypedArray.ts';
export { isPrimitive } from './isPrimitive.ts';
export { isBoolean } from './isBoolean.ts';
export { isSymbol } from './isSymbol.ts';
export { isString } from './isString.ts';
Repository: toss/es-toolkit
File: src/compat/predicate/isString.spec.ts (TypeScript)

Code before:
import { describe, expect, it } from 'vitest';
import { isString } from './isString';
import { falsey } from '../_internal/falsey';
import { args } from '../_internal/args';
import { slice } from '../_internal/slice';
import { symbol } from '../_internal/symbol';

describe('isStirng', () => {
  it('returns true if the value is string', () => {
    expect(isString('a')).toBe(true);
    expect(isString(Object('a'))).toBe(true);
  });

  it('returns false if the value is not string', () => {
    const expected = falsey.map(value => value === '');

    const actual = falsey.map((value, index) => isString(value));

    expect(actual).toEqual(expected);

    expect(isString(args)).toBe(false);
    expect(isString([1, 2, 3])).toBe(false);
    expect(isString(true)).toBe(false);
    expect(isString(new Date())).toBe(false);
    expect(isString(new Error())).toBe(false);
    expect(isString(slice)).toBe(false);
    expect(isString({ '0': 1, length: 1 })).toBe(false);
    expect(isString(1)).toBe(false);
    expect(isString(/x/)).toBe(false);
    expect(isString(symbol)).toBe(false);
  });
});


Review comment (suggestion):
```suggestion
    const actual = falsey.map((value) => isString(value));
```

Code after:
import { describe, expect, it } from 'vitest';
import { isString } from './isString';
import { falsey } from '../_internal/falsey';
import { args } from '../_internal/args';
import { slice } from '../_internal/slice';
import { symbol } from '../_internal/symbol';

describe('isString', () => {
  it('returns true if the value is string', () => {
    expect(isString('a')).toBe(true);
    expect(isString(Object('a'))).toBe(true);
  });

  it('returns false if the value is not string', () => {
    const expected = falsey.map(value => value === '');

    const actual = falsey.map((value) => isString(value));

    expect(actual).toEqual(expected);

    expect(isString(args)).toBe(false);
    expect(isString([1, 2, 3])).toBe(false);
    expect(isString(true)).toBe(false);
    expect(isString(new Date())).toBe(false);
    expect(isString(new Error())).toBe(false);
    expect(isString(slice)).toBe(false);
    expect(isString({ '0': 1, length: 1 })).toBe(false);
    expect(isString(1)).toBe(false);
    expect(isString(/x/)).toBe(false);
    expect(isString(symbol)).toBe(false);
  });
});
Repository: toss/es-toolkit
File: src/array/uniqBy.ts (TypeScript)

Code before:
/**
 * Returns a new array containing only the unique elements from the original array,
 * based on the values returned by the mapper function.
 *
 * @template T - The type of elements in the array.
 * @template U - The type of mapped elements.
 * @param {T[]} arr - The array to process.
 * @param {(item: T) => U} mapper - The function used to convert the array elements.
 * @returns {T[]} A new array containing only the unique elements from the original array, based on the values returned by the mapper function.
 *
 * @example
 * ```ts
 * uniqBy([1.2, 1.5, 2.1, 3.2, 5.7, 5.3, 7.19], Math.floor);
 * // [1.2, 2.1, 3.2, 5.7, 7.19]
 * ```
 * 
 * @example
 * ```ts
 * const array = [
 *   { category: 'fruit', name: 'apple' },
 *   { category: 'fruit', name: 'banana' },
 *   { category: 'vegetable', name: 'carrot' },
 * ];
 * uniqBy(array, item => item.category).length
 * // 2
 * ```
 */
export function uniqBy<T, U>(arr: readonly T[], mapper: (item: T) => U): T[] {
  const map = new Map<U, T>();

  for (const item of arr) {
    const key = mapper(item);

    if (!map.has(key)) {
      map.set(key, item);
    }
  }

  return Array.from(map.values());
}


Review comment (suggestion):
```suggestion
 * const array = [
 *   { category: 'fruit', name: 'apple' },
 *   { category: 'fruit', name: 'banana' },
 *   { category: 'vegetable', name: 'carrot' },
 * ];
 * uniqBy(array, item => item.category).length
 * // 2
```

Code after:
/**
 * Returns a new array containing only the unique elements from the original array,
 * based on the values returned by the mapper function.
 *
 * @template T - The type of elements in the array.
 * @template U - The type of mapped elements.
 * @param {T[]} arr - The array to process.
 * @param {(item: T) => U} mapper - The function used to convert the array elements.
 * @returns {T[]} A new array containing only the unique elements from the original array, based on the values returned by the mapper function.
 *
 * @example
 * ```ts
 * uniqBy([1.2, 1.5, 2.1, 3.2, 5.7, 5.3, 7.19], Math.floor);
 * // [1.2, 2.1, 3.2, 5.7, 7.19]
 * ```
 * 
 * @example
 * const array = [
 *   { category: 'fruit', name: 'apple' },
 *   { category: 'fruit', name: 'banana' },
 *   { category: 'vegetable', name: 'carrot' },
 * ];
 * uniqBy(array, item => item.category).length
 * // 2
 * ```
 */
export function uniqBy<T, U>(arr: readonly T[], mapper: (item: T) => U): T[] {
  const map = new Map<U, T>();

  for (const item of arr) {
    const key = mapper(item);

    if (!map.has(key)) {
      map.set(key, item);
    }
  }

  return Array.from(map.values());
}
Repository: toss/es-toolkit
File: src/compat/array/orderBy.ts (TypeScript)

Code before:
 * //   { user: 'fred', age: 48 },
 * //   { user: 'fred', age: 40 },
 * // ]
 */
export function orderBy<T>(
  collection: T[] | object | null | undefined,
  criteria?: ((item: T) => unknown) | PropertyKey | Array<((item: T) => unknown) | PropertyKey | PropertyKey[]>,
  orders?: unknown | unknown[]
): T[] {
  if (collection == null) {
    return [];
  }

  if (!Array.isArray(collection) && typeof collection === 'object') {
    collection = Object.values(collection);
  }

  if (!Array.isArray(criteria)) {
    criteria = criteria == null ? [] : [criteria];
  }

  if (!Array.isArray(orders)) {
    orders = orders == null ? [] : [orders];
  }

  const getVaueByNestedPath = (object: object, path: PropertyKey[]) => {
    let target: object = object;

    for (let i = 0; i < path.length && target != null; i++) {
      target = target[path[i] as keyof typeof target];
    }

    return target;
  };

  const getValueByCriterion = (
    criterion: PropertyKey | ((item: T) => unknown) | PropertyKey[] | { key: PropertyKey; path: string[] },
    object: T
  ) => {
    if (object == null) {
      return object;
    }

    if (typeof criterion === 'function') {
      return criterion(object);
    }

    if (Array.isArray(criterion)) {
      return getVaueByNestedPath(object, criterion);
    }


Review comment (suggestion):
```suggestion
  const getValueByNestedPath = (object: object, path: PropertyKey[]) => {
```

Code after:
 * //   { user: 'fred', age: 48 },
 * //   { user: 'fred', age: 40 },
 * // ]
 */
export function orderBy<T>(
  collection: T[] | object | null | undefined,
  criteria?: ((item: T) => unknown) | PropertyKey | Array<((item: T) => unknown) | PropertyKey | PropertyKey[]>,
  orders?: unknown | unknown[]
): T[] {
  if (collection == null) {
    return [];
  }

  if (!Array.isArray(collection) && typeof collection === 'object') {
    collection = Object.values(collection);
  }

  if (!Array.isArray(criteria)) {
    criteria = criteria == null ? [] : [criteria];
  }

  if (!Array.isArray(orders)) {
    orders = orders == null ? [] : [orders];
  }

  const getValueByNestedPath = (object: object, path: PropertyKey[]) => {
    let target: object = object;

    for (let i = 0; i < path.length && target != null; i++) {
      target = target[path[i] as keyof typeof target];
    }

    return target;
  };

  const getValueByCriterion = (
    criterion: PropertyKey | ((item: T) => unknown) | PropertyKey[] | { key: PropertyKey; path: string[] },
    object: T
  ) => {
    if (object == null) {
      return object;
    }

    if (typeof criterion === 'function') {
      return criterion(object);
    }

    if (Array.isArray(criterion)) {
      return getValueByNestedPath(object, criterion);
    }
Repository: toss/es-toolkit
File: src/compat/array/orderBy.ts (TypeScript)

Code before:
  }

  const getVaueByNestedPath = (object: object, path: PropertyKey[]) => {
    let target: object = object;

    for (let i = 0; i < path.length && target != null; i++) {
      target = target[path[i] as keyof typeof target];
    }

    return target;
  };

  const getValueByCriterion = (
    criterion: PropertyKey | ((item: T) => unknown) | PropertyKey[] | { key: PropertyKey; path: string[] },
    object: T
  ) => {
    if (object == null) {
      return object;
    }

    if (typeof criterion === 'function') {
      return criterion(object);
    }

    if (Array.isArray(criterion)) {
      return getVaueByNestedPath(object, criterion);
    }

    if (typeof criterion !== 'object') {
      return object[criterion as keyof typeof object];
    }

    // Case for possible to be a deep path
    if (Object.hasOwn(object, criterion.key)) {
      return object[criterion.key as keyof typeof object];
    }

    return getVaueByNestedPath(object, criterion.path);
  };

  // Prepare all cases for criteria
  const preparedCriteria = criteria.map(criterion => {
    // lodash handles a array with one element as a single criterion
    if (Array.isArray(criterion) && criterion.length === 1) {
      criterion = criterion[0];
    }

    if (typeof criterion === 'function' || Array.isArray(criterion) || isKey(criterion)) {
      return criterion;
    }


Review comment (suggestion):
```suggestion
      return getValueByNestedPath(object, criterion);
```

Code after:
  }

  const getValueByNestedPath = (object: object, path: PropertyKey[]) => {
    let target: object = object;

    for (let i = 0; i < path.length && target != null; i++) {
      target = target[path[i] as keyof typeof target];
    }

    return target;
  };

  const getValueByCriterion = (
    criterion: PropertyKey | ((item: T) => unknown) | PropertyKey[] | { key: PropertyKey; path: string[] },
    object: T
  ) => {
    if (object == null) {
      return object;
    }

    if (typeof criterion === 'function') {
      return criterion(object);
    }

    if (Array.isArray(criterion)) {
      return getValueByNestedPath(object, criterion);
    }

    if (typeof criterion !== 'object') {
      return object[criterion as keyof typeof object];
    }

    // Case for possible to be a deep path
    if (Object.hasOwn(object, criterion.key)) {
      return object[criterion.key as keyof typeof object];
    }

    return getValueByNestedPath(object, criterion.path);
  };

  // Prepare all cases for criteria
  const preparedCriteria = criteria.map(criterion => {
    // lodash handles a array with one element as a single criterion
    if (Array.isArray(criterion) && criterion.length === 1) {
      criterion = criterion[0];
    }

    if (typeof criterion === 'function' || Array.isArray(criterion) || isKey(criterion)) {
      return criterion;
    }