ronantakizawa/codereview-bench huggingface dataset:
Repository: 4ian/GDevelop
File: newIDE/app/src/UI/SoundPlayer/CompactSoundPlayer.js (JavaScript)

Code before:
// @flow

import * as React from 'react';
import IconButton from '@material-ui/core/IconButton';
import Play from '../CustomSvgIcons/Play';
import Pause from '../CustomSvgIcons/Pause';

type Props = {|
  soundSrc: string | null,
  onSoundLoaded?: () => void,
  onSoundError?: () => void,
|};

export type CompactSoundPlayerInterface = {|
  playPause: (forcePlay: boolean) => void,
|};

const CompactSoundPlayer = React.forwardRef<Props, CompactSoundPlayerInterface>(
  ({ soundSrc, onSoundLoaded, onSoundError }, ref) => {
    const mobileAudioRef = React.useRef<?Audio>(null);
    const [isPlaying, setIsPlaying] = React.useState(false);

    const onPlayPause = React.useCallback(
      (forcePlay?: boolean) => {
        if (!soundSrc) return;
        setIsPlaying(_isPlaying => forcePlay || !_isPlaying);
      },
      [soundSrc]
    );

    const onLoad = React.useCallback(() => {
      setIsPlaying(false);
    }, []);

    const onAudioReady = React.useCallback(
      () => {
        if (!mobileAudioRef.current) return;
        if (onSoundLoaded) onSoundLoaded();
      },
      [onSoundLoaded]
    );

    React.useImperativeHandle(ref, () => ({
      playPause: onPlayPause,

Review comment (suggestion):
Interface "playPause" is used nowhere by other components, so no need to complexify the component.

```suggestion
const CompactSoundPlayer = ({ soundSrc, onSoundLoaded, onSoundError }: Props) => {
```

Code after:
// @flow

import * as React from 'react';
import IconButton from '@material-ui/core/IconButton';
import CircularProgress from '@material-ui/core/CircularProgress';
import Play from '../CustomSvgIcons/Play';
import Pause from '../CustomSvgIcons/Pause';
import { useIsMounted } from '../../Utils/UseIsMounted';

type Props = {|
  soundSrc: string | null,
  onSoundLoaded?: () => void,
  onSoundError?: () => void,
|};

const CompactSoundPlayer = ({
  soundSrc,
  onSoundLoaded,
  onSoundError,
}: Props) => {
  const audioRef = React.useRef<?HTMLAudioElement>(null);
  const [isPlaying, setIsPlaying] = React.useState(false);
  const [isLoading, setIsLoading] = React.useState(false);
  const [isAudioLoaded, setIsAudioLoaded] = React.useState(false);
  const isMountedRef = useIsMounted();
  const abortControllerRef = React.useRef<?AbortController>(null);

  const cleanupAudioAndListeners = React.useCallback(() => {
    const audio = audioRef.current;
    if (!audio) return;

    audio.pause();
    if (abortControllerRef.current) abortControllerRef.current.abort();

    audio.src = '';
    audioRef.current = null;
    abortControllerRef.current = null;
    setIsPlaying(false);
    setIsLoading(false);
    setIsAudioLoaded(false);
  }, []);

  const initializeAudio = React.useCallback(
    () => {
Repository: 4ian/GDevelop
File: newIDE/app/src/UI/SoundPlayer/CompactSoundPlayer.js (JavaScript)

Code before:
// @flow

import * as React from 'react';
import IconButton from '@material-ui/core/IconButton';
import Play from '../CustomSvgIcons/Play';
import Pause from '../CustomSvgIcons/Pause';

type Props = {|
  soundSrc: string | null,
  onSoundLoaded?: () => void,
  onSoundError?: () => void,
|};

export type CompactSoundPlayerInterface = {|
  playPause: (forcePlay: boolean) => void,
|};

const CompactSoundPlayer = React.forwardRef<Props, CompactSoundPlayerInterface>(
  ({ soundSrc, onSoundLoaded, onSoundError }, ref) => {
    const mobileAudioRef = React.useRef<?Audio>(null);
    const [isPlaying, setIsPlaying] = React.useState(false);

    const onPlayPause = React.useCallback(
      (forcePlay?: boolean) => {
        if (!soundSrc) return;
        setIsPlaying(_isPlaying => forcePlay || !_isPlaying);
      },
      [soundSrc]
    );

    const onLoad = React.useCallback(() => {
      setIsPlaying(false);
    }, []);

    const onAudioReady = React.useCallback(
      () => {
        if (!mobileAudioRef.current) return;
        if (onSoundLoaded) onSoundLoaded();
      },
      [onSoundLoaded]
    );

    React.useImperativeHandle(ref, () => ({
      playPause: onPlayPause,
    }));

Review comment (suggestion):
```suggestion
    const audioRef = React.useRef<?Audio>(null);
```

No reason to add "mobile" in the name, this is confusing for the reader.

Code after:
// @flow

import * as React from 'react';
import IconButton from '@material-ui/core/IconButton';
import CircularProgress from '@material-ui/core/CircularProgress';
import Play from '../CustomSvgIcons/Play';
import Pause from '../CustomSvgIcons/Pause';
import { useIsMounted } from '../../Utils/UseIsMounted';

type Props = {|
  soundSrc: string | null,
  onSoundLoaded?: () => void,
  onSoundError?: () => void,
|};

const CompactSoundPlayer = ({
  soundSrc,
  onSoundLoaded,
  onSoundError,
}: Props) => {
  const audioRef = React.useRef<?HTMLAudioElement>(null);
  const [isPlaying, setIsPlaying] = React.useState(false);
  const [isLoading, setIsLoading] = React.useState(false);
  const [isAudioLoaded, setIsAudioLoaded] = React.useState(false);
  const isMountedRef = useIsMounted();
  const abortControllerRef = React.useRef<?AbortController>(null);

  const cleanupAudioAndListeners = React.useCallback(() => {
    const audio = audioRef.current;
    if (!audio) return;

    audio.pause();
    if (abortControllerRef.current) abortControllerRef.current.abort();

    audio.src = '';
    audioRef.current = null;
    abortControllerRef.current = null;
    setIsPlaying(false);
    setIsLoading(false);
    setIsAudioLoaded(false);
  }, []);

  const initializeAudio = React.useCallback(
    () => {
      if (!soundSrc || audioRef.current) return;
Repository: 4ian/GDevelop
File: newIDE/app/src/LayersList/index.js (JavaScript)

Code before:
      [forceUpdate]
    );

    const scrollToItem = React.useCallback((itemId: string) => {
      if (treeViewRef.current) {
        treeViewRef.current.scrollToItemFromId(itemId);
      }
    }, []);

    React.useImperativeHandle(ref, () => ({
      forceUpdateList: () => {
        forceUpdate();
        if (treeViewRef.current) treeViewRef.current.forceUpdateList();
      },
    }));

    const editName = React.useCallback(
      (layerName: string) => {
        // Don't allow renaming base layer (empty name)
        if (!layerName) return;

        // Find the layer ID from the layer name
        const layer = layersContainer.getLayer(layerName);
        if (!layer) return;

        const itemId = `layer-${layer.ptr}`;
        const treeView = treeViewRef.current;
        if (treeView) {
          if (isMobile) {
            // Position item at top of the screen to make sure it will be visible
            // once the keyboard is open.
            treeView.scrollToItemFromId(itemId, 'start');
          }
          treeView.renameItemFromId(itemId);
        }
      },
      [isMobile, layersContainer]
    );

    const onTreeModified = React.useCallback(
      (shouldForceUpdateList: boolean) => {
        triggerUnsavedChanges();

        if (shouldForceUpdateList) forceUpdateList();
        else forceUpdate();
      },
      [forceUpdate, forceUpdateList, triggerUnsavedChanges]
    );

    const triggerOnLayersModified = React.useCallback(
      () => {

Review comment (suggestion):
```suggestion
        const itemId = getLayerTreeViewItemId(layer);
```

Code after:
      [forceUpdate]
    );

    const scrollToItem = React.useCallback((itemId: string) => {
      if (treeViewRef.current) {
        treeViewRef.current.scrollToItemFromId(itemId);
      }
    }, []);

    React.useImperativeHandle(ref, () => ({
      forceUpdateList: () => {
        forceUpdate();
        if (treeViewRef.current) treeViewRef.current.forceUpdateList();
      },
    }));

    const editName = React.useCallback(
      (layerName: string) => {
        // Don't allow renaming base layer (empty name)
        if (!layerName) return;

        if (!layersContainer.hasLayerNamed(layerName)) return;
        const layer = layersContainer.getLayer(layerName);

        const itemId = getLayerTreeViewItemId(layer);
        const treeView = treeViewRef.current;
        if (treeView) {
          if (isMobile) {
            // Position item at top of the screen to make sure it will be visible
            // once the keyboard is open.
            treeView.scrollToItemFromId(itemId, 'start');
          }
          treeView.renameItemFromId(itemId);
        }
      },
      [isMobile, layersContainer]
    );

    const onTreeModified = React.useCallback(
      (shouldForceUpdateList: boolean) => {
        triggerUnsavedChanges();

        if (shouldForceUpdateList) forceUpdateList();
        else forceUpdate();
      },
      [forceUpdate, forceUpdateList, triggerUnsavedChanges]
    );

    const triggerOnLayersModified = React.useCallback(
      () => {
        onLayersModified();
Repository: 4ian/GDevelop
File: newIDE/app/src/EditorFunctions/Utils.js (JavaScript)

Code before:
    };
  }

  if (objectType === 'PanelSpriteObject::PanelSprite') {
    const config = gd.asPanelSpriteConfiguration(objectConfiguration);
    const width = config.getWidth();
    const height = config.getHeight();
    return {
      size: `${width}x${height}`,
      origin: '0;0',
      center: `${width / 2};${height / 2}`,
    };
  }

  // Events-based (custom) objects: derive size from their declared area.
  if (project.hasEventsBasedObject(objectType)) {
    const eventsBasedObject = project.getEventsBasedObject(objectType);
    const minX = eventsBasedObject.getAreaMinX();
    const maxX = eventsBasedObject.getAreaMaxX();
    const minY = eventsBasedObject.getAreaMinY();
    const maxY = eventsBasedObject.getAreaMaxY();
    const width = maxX - minX;
    const height = maxY - minY;
    return {
      size: `${width}x${height}`,
      origin: '0;0',
      center: `${(minX + maxX) / 2};${(minY + maxY) / 2}`,
    };
  }

  return null;
};


Review comment (suggestion):
@codex This origin is false because it depends on min/max X/Y/Z (for example if minX = -10, the origin is actually at 10)

Can you also adapt this for 3D objects (IsRenderedIn3D)?
And adapt the tests to add a test case with a non 0;0 origin and another test case for 3D events based objects

Code after:
    };
  }

  if (objectType === 'PanelSpriteObject::PanelSprite') {
    const config = gd.asPanelSpriteConfiguration(objectConfiguration);
    const width = config.getWidth();
    const height = config.getHeight();
    return {
      size: `${width}x${height}`,
      origin: '0;0',
      center: `${width / 2};${height / 2}`,
    };
  }

  // Events-based (custom) objects: derive size from their declared area.
  if (project.hasEventsBasedObject(objectType)) {
    const eventsBasedObject = project.getEventsBasedObject(objectType);
    const minX = eventsBasedObject.getAreaMinX();
    const maxX = eventsBasedObject.getAreaMaxX();
    const minY = eventsBasedObject.getAreaMinY();
    const maxY = eventsBasedObject.getAreaMaxY();
    const isRenderedIn3D = eventsBasedObject.isRenderedIn3D();
    const minZ = isRenderedIn3D ? eventsBasedObject.getAreaMinZ() : 0;
    const maxZ = isRenderedIn3D ? eventsBasedObject.getAreaMaxZ() : 0;
    const width = maxX - minX;
    const height = maxY - minY;
    const depth = maxZ - minZ;

    const origin = `${-minX};${-minY}${isRenderedIn3D ? `;${-minZ}` : ''}`;
    const center = `${width / 2};${height / 2}${
      isRenderedIn3D ? `;${depth / 2}` : ''
    }`;
    return {
      size: `${width}x${height}${isRenderedIn3D ? `x${depth}` : ''}`,
      origin,
      center,
    };
  }

  return null;
};
Repository: Alamofire/Alamofire
File: Source/SessionManager.swift (Swift)

Code before:
            request.retryCount += 1
            request.startTime = CFAbsoluteTimeGetCurrent()
            request.endTime = nil

            task.resume()

            return true
        } catch {
            request.delegate.error = error.underlyingAdaptError ?? error
            return false
        }
    }

    private func allowRetrier(_ retrier: RequestRetrier, toRetry request: Request, with error: Error) {
        DispatchQueue.utility.async { [weak self] in
            guard let strongSelf = self else { return }

            retrier.should(strongSelf, retry: request, with: error) { shouldRetry, timeDelay in
                guard let strongSelf = self else { return }

                guard shouldRetry else {
                    if strongSelf.startRequestsImmediately { request.resume() }
                    return
                }

                let retrySucceeded = strongSelf.retry(request)

                if retrySucceeded, let task = request.task {
                    strongSelf.delegate[task] = request
                } else {
                    if strongSelf.startRequestsImmediately { request.resume() }
                }
            }
        }
    }
}


Review comment (suggestion):
I'm unclear as to whether we should be executing this code on a separate queue or the same queue that originally executed the request. Wouldn't this logic create a new queue for every retry attempt? 

Perhaps at some point one of us should create a runtime diagram that outlines the various queues used...

Code after:
            request.retryCount += 1
            request.startTime = CFAbsoluteTimeGetCurrent()
            request.endTime = nil

            task.resume()

            return true
        } catch {
            request.delegate.error = error.underlyingAdaptError ?? error
            return false
        }
    }

    private func allowRetrier(_ retrier: RequestRetrier, toRetry request: Request, with error: Error) {
        DispatchQueue.utility.async { [weak self] in
            guard let strongSelf = self else { return }

            retrier.should(strongSelf, retry: request, with: error) { shouldRetry, timeDelay in
                guard let strongSelf = self else { return }

                guard shouldRetry else {
                    if strongSelf.startRequestsImmediately { request.resume() }
                    return
                }

                DispatchQueue.utility.after(timeDelay) {
                    guard let strongSelf = self else { return }

                    let retrySucceeded = strongSelf.retry(request)

                    if retrySucceeded, let task = request.task {
                        strongSelf.delegate[task] = request
                    } else {
                        if strongSelf.startRequestsImmediately { request.resume() }
                    }
                }
            }
        }
    }
}
Repository: Alamofire/Alamofire
File: Source/Mutex+Protector.swift (Swift)

Code before:
    fileprivate func lock() {
        let result = pthread_mutex_lock(&mutex)
        assert(result == 0, "Failed to lock mutex")
    }

    fileprivate func unlock() {
        let result = pthread_mutex_unlock(&mutex)
        assert(result == 0, "Failed to unlock mutex")
    }

    /// Execute a value producing closure while aquiring the mutex.
    ///
    /// - Parameter closure: The closure to run.
    /// - Returns:           The value the closure generated.
    func around<T>(_ closure: () -> T) -> T {
        lock(); defer { unlock() }
        return closure()
    }

    /// Execute a closure while aquiring the mutex.
    ///
    /// - Parameter closure: The closure to run.
    func around(_ closure: () -> Void) {
        lock(); defer { unlock() }
        return closure()
    }
}

/// An `os_unfair_lock` wrapper.
@available (iOS 10.0, macOS 10.12, tvOS 10.0, watchOS 3.0, *)
final class UnfairLock: Lock {
    private var unfairLock = os_unfair_lock()

    public init() { }

    fileprivate func lock() {
        os_unfair_lock_lock(&unfairLock)
    }

    fileprivate func unlock() {
        os_unfair_lock_unlock(&unfairLock)
    }

    /// Execute a value producing closure while aquiring the lock.
    ///
    /// - Parameter closure: The closure to run.
    /// - Returns:           The value the closure generated.
    func around<T>(_ closure: () -> T) -> T {
        lock(); defer { unlock() }
        return closure()
    }

Review comment (suggestion):
I think you might be able to implement both `around` functions in a protocol extension that leverages the `lock` and `unlock` APIs.

Code after:
        return closure()
    }
}

// MARK: -

/// A `pthread_mutex` wrapper, inspired by ProcedureKit.
final class Mutex: Lock {
    private var mutex = pthread_mutex_t()

    init() {
        let result = pthread_mutex_init(&mutex, nil)
        precondition(result == 0, "Failed to create pthread mutex")
    }

    deinit {
        let result = pthread_mutex_destroy(&mutex)
        assert(result == 0, "Failed to destroy mutex")
    }

    fileprivate func lock() {
        let result = pthread_mutex_lock(&mutex)
        assert(result == 0, "Failed to lock mutex")
    }

    fileprivate func unlock() {
        let result = pthread_mutex_unlock(&mutex)
        assert(result == 0, "Failed to unlock mutex")
    }
}

// MARK: -

/// An `os_unfair_lock` wrapper.
@available (iOS 10.0, macOS 10.12, tvOS 10.0, watchOS 3.0, *)
final class UnfairLock: Lock {
    private var unfairLock = os_unfair_lock()

    public init() { }

    fileprivate func lock() {
        os_unfair_lock_lock(&unfairLock)
    }

    fileprivate func unlock() {
        os_unfair_lock_unlock(&unfairLock)
    }
}

// MARK: -
Repository: Alamofire/Alamofire
File: Source/Mutex+Protector.swift (Swift)

Code before:
    }

    fileprivate func unlock() {
        os_unfair_lock_unlock(&unfairLock)
    }

    /// Execute a value producing closure while aquiring the lock.
    ///
    /// - Parameter closure: The closure to run.
    /// - Returns:           The value the closure generated.
    func around<T>(_ closure: () -> T) -> T {
        lock(); defer { unlock() }
        return closure()
    }

    /// Execute a closure while aquiring the lock.
    ///
    /// - Parameter closure: The closure to run.
    func around(_ closure: () -> Void) {
        lock(); defer { unlock() }
        return closure()
    }
}

/// A thread-safe wrapper around a value.
final class Protector<T> {
    private let lock: Lock = {
        if #available(iOS 10.0, macOS 10.12, tvOS 10.0, watchOS 3.0, *) {
            return UnfairLock()
        } else {
            return Mutex()
        }
    }()
    private var ward: T

    init(_ ward: T) {
        self.ward = ward
    }

    /// The contained value. Unsafe for anything more than direct read or write.
    var unsafeValue: T {
        get { return lock.around { ward } }
        set { lock.around { ward = newValue } }
    }

    /// Synchronously read or transform the contained value.
    ///
    /// - Parameter closure: The closure to execute.
    /// - Returns:           The return value of the closure passed.
    func read<U>(_ closure: (T) -> U) -> U {
        return lock.around { closure(self.ward) }

Review comment (refactor):
I've built similar abstractions before that were named `Locker` instead of `Protector`. Not really sure which one I prefer more. Thoughts?

Code after:

    public init() { }

    fileprivate func lock() {
        os_unfair_lock_lock(&unfairLock)
    }

    fileprivate func unlock() {
        os_unfair_lock_unlock(&unfairLock)
    }
}

// MARK: -

/// A thread-safe wrapper around a value.
final class Protector<T> {
    private let lock: Lock = {
        if #available(iOS 10.0, macOS 10.12, tvOS 10.0, watchOS 3.0, *) {
            return UnfairLock()
        } else {
            return Mutex()
        }
    }()

    private var value: T

    init(_ value: T) {
        self.value = value
    }

    /// The contained value. Unsafe for anything more than direct read or write.
    var directValue: T {
        get { return lock.around { value } }
        set { lock.around { value = newValue } }
    }

    /// Synchronously read or transform the contained value.
    ///
    /// - Parameter closure: The closure to execute.
    /// - Returns:           The return value of the closure passed.
    func read<U>(_ closure: (T) -> U) -> U {
        return lock.around { closure(self.value) }
    }

    /// Synchronously modify the protected value.
    ///
    /// - Parameter closure: The closure to execute.
    /// - Returns:           The modified value.
    @discardableResult
    func write<U>(_ closure: (inout T) -> U) -> U {
        return lock.around { closure(&self.value) }
Repository: Alamofire/Alamofire
File: Source/ParameterEncoding.swift (Swift)

Code before:
        for key in parameters.keys.sorted(by: <) {
            let value = parameters[key]!
            components += queryComponents(fromKey: key, value: value)
        }
        return components.map { "\($0)=\($1)" }.joined(separator: "&")
    }

    private func encodesParametersInURL(with method: HTTPMethod) -> Bool {
        switch destination {
        case .queryString:
            return true
        case .httpBody:
            return false
        default:
            break
        }

        switch method {
        case .get, .head, .delete:
            return true
        default:
            return false
        }
    }

    private func encodeArrayKey(_ key: String) -> String {
        switch arrayEncoding {
        case .brackets:
            return "\(key)[]"
        case .noBrackets:
            return key
        }
    }

    private func encodeBoolValue(_ value: Bool) -> String {
        switch boolEncoding {
        case .numeric:
            return value ? "1" : "0"
        case .literal:
            return value ? "true" : "false"
        }
    }
}

// MARK: -

/// Uses `JSONSerialization` to create a JSON representation of the parameters object, which is set as the body of the
/// request. The `Content-Type` HTTP header field of an encoded request is set to `application/json`.
public struct JSONEncoding: ParameterEncoding {

    // MARK: Properties

Review comment (suggestion):
It would be cleaner if this was simply an `encode` function on `ArrayEncoding`. Better encapsulation that way.

Code after:
            while index != string.endIndex {
                let startIndex = index
                let endIndex = string.index(index, offsetBy: batchSize, limitedBy: string.endIndex) ?? string.endIndex
                let range = startIndex..<endIndex

                let substring = string[range]

                escaped += substring.addingPercentEncoding(withAllowedCharacters: allowedCharacterSet) ?? String(substring)

                index = endIndex
            }
        }

        return escaped
    }

    private func query(_ parameters: [String: Any]) -> String {
        var components: [(String, String)] = []

        for key in parameters.keys.sorted(by: <) {
            let value = parameters[key]!
            components += queryComponents(fromKey: key, value: value)
        }
        return components.map { "\($0)=\($1)" }.joined(separator: "&")
    }

    private func encodesParametersInURL(with method: HTTPMethod) -> Bool {
        switch destination {
        case .queryString:
            return true
        case .httpBody:
            return false
        default:
            break
        }

        switch method {
        case .get, .head, .delete:
            return true
        default:
            return false
        }
    }
}

// MARK: -

/// Uses `JSONSerialization` to create a JSON representation of the parameters object, which is set as the body of the
/// request. The `Content-Type` HTTP header field of an encoded request is set to `application/json`.
public struct JSONEncoding: ParameterEncoding {