ronantakizawa/codereview-bench huggingface dataset:
Repository: TheAlgorithms/Java
File: src/test/java/com/thealgorithms/matrix/InverseOfMatrixTest.java (Java)

Code before:
package com.thealgorithms.matrix;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;

import java.util.stream.Stream;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

class InverseOfMatrixTest {

    @ParameterizedTest
    @MethodSource("provideTestCases")
    void testInvert(double[][] matrix, double[][] expectedInverse) {
        double[][] result = InverseOfMatrix.invert(matrix);
        assertMatrixEquals(expectedInverse, result);
    }

    private static Stream<Arguments> provideTestCases() {
        return Stream.of(Arguments.of(new double[][] {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}, new double[][] {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}), Arguments.of(new double[][] {{4, 7}, {2, 6}}, new double[][] {{0.6, -0.7}, {-0.2, 0.4}}));
    }

    private void assertMatrixEquals(double[][] expected, double[][] actual) {
        for (int i = 0; i < expected.length; i++) {
            assertArrayEquals(expected[i], actual[i], 1.0E-10, "Row " + i + " is not equal");
        }
    }
}


Review comment (suggestion):
```suggestion
```

This should make `clang-format` happy.

Code after:
package com.thealgorithms.matrix;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;

import java.util.stream.Stream;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

class InverseOfMatrixTest {

    @ParameterizedTest
    @MethodSource("provideTestCases")
    void testInvert(double[][] matrix, double[][] expectedInverse) {
        double[][] result = InverseOfMatrix.invert(matrix);
        assertMatrixEquals(expectedInverse, result);
    }

    private static Stream<Arguments> provideTestCases() {
        return Stream.of(Arguments.of(new double[][] {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}, new double[][] {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}), Arguments.of(new double[][] {{4, 7}, {2, 6}}, new double[][] {{0.6, -0.7}, {-0.2, 0.4}}));
    }

    private void assertMatrixEquals(double[][] expected, double[][] actual) {
        for (int i = 0; i < expected.length; i++) {
            assertArrayEquals(expected[i], actual[i], 1.0E-10, "Row " + i + " is not equal");
        }
    }
}
Repository: TheAlgorithms/Java
File: src/main/java/com/thealgorithms/maths/AbsoluteMax.java (Java)

Code before:
package com.thealgorithms.maths;

public final class AbsoluteMax {
    private AbsoluteMax() {
    }

    /**
     * Finds the absolute maximum value among the given numbers.
     *
     * @param numbers The numbers to compare.
     * @return The absolute maximum value.
     * @throws IllegalArgumentException If the input array is empty or null.
     */
    public static int getMaxValue(int... numbers) {
        if (numbers == null || numbers.length == 0) {
            throw new IllegalArgumentException("Numbers array cannot be empty or null");
        }
        int absMax = numbers[0];
        for (int i = 1; i < numbers.length; i++) {
            if (Math.abs(numbers[i]) > Math.abs(absMax)) {
                absMax = numbers[i];
            }
        }
        return absMax;
    }
}


Review comment (suggestion):
```suggestion
            if (Math.abs(numbers[i]) >= Math.abs(absMax)) {
                absMax = Math.max(absMax, numbers[i]);
            }
```
Can it be done like that? The initially proposed condition looks quite complex

Code after:
package com.thealgorithms.maths;

public final class AbsoluteMax {
    private AbsoluteMax() {
    }

    /**
     * Finds the absolute maximum value among the given numbers.
     *
     * @param numbers The numbers to compare.
     * @return The absolute maximum value.
     * @throws IllegalArgumentException If the input array is empty or null.
     */
    public static int getMaxValue(int... numbers) {
        if (numbers == null || numbers.length == 0) {
            throw new IllegalArgumentException("Numbers array cannot be empty or null");
        }
        int absMax = numbers[0];
        for (int i = 1; i < numbers.length; i++) {
            if (Math.abs(numbers[i]) > Math.abs(absMax) || (Math.abs(numbers[i]) == Math.abs(absMax) && numbers[i] > absMax)) {
                absMax = numbers[i];
            }
        }
        return absMax;
    }
}
Repository: TheAlgorithms/Java
File: src/main/java/com/thealgorithms/randomized/RandomizedQuickSort.java (Java)

Code before:
     * @param high the ending index
     * @return the index of the pivot after partitioning
     */
    private static int partition(int[] arr, int low, int high) {
        int pivotIndex = low + (int) (Math.random() * (high - low + 1));
        int pivotValue = arr[pivotIndex];
        swap(arr, pivotIndex, high); // Move pivot to end
        int storeIndex = low;
        for (int i = low; i < high; i++) {
            if (arr[i] < pivotValue) {
                swap(arr, storeIndex, i);
                storeIndex++;
            }
        }
        swap(arr, storeIndex, high); // Move pivot to its final place
        return storeIndex;
    }

    /**
     * Swaps two elements in the array.
     *
     * @param arr the array in which elements are to be swapped
     * @param i the first index
     * @param j the second index
     */
    private static void swap(int[] arr, int i, int j) {
        int temp = arr[i];
        arr[i] = arr[j];
        arr[j] = temp;
    }
}


Review comment (performance):
- In the swap method, swapping can be skipped when i == j; this is optional and would slightly improve performance. Overall, the implementation looks correct :)

Code after:
     * @param high the ending index
     * @return the index of the pivot after partitioning
     */
    private static int partition(int[] arr, int low, int high) {
        int pivotIndex = low + (int) (Math.random() * (high - low + 1));
        int pivotValue = arr[pivotIndex];
        swap(arr, pivotIndex, high); // Move pivot to end
        int storeIndex = low;
        for (int i = low; i < high; i++) {
            if (arr[i] < pivotValue) {
                swap(arr, storeIndex, i);
                storeIndex++;
            }
        }
        swap(arr, storeIndex, high); // Move pivot to its final place
        return storeIndex;
    }

    /**
     * Swaps two elements in the array, only if the indices are different.
     *
     * @param arr the array in which elements are to be swapped
     * @param i the first index
     * @param j the second index
     */
    private static void swap(int[] arr, int i, int j) {
        // Skip if indices are the same
        if (i == j) {
            return;
        }
        int temp = arr[i];
        arr[i] = arr[j];
        arr[j] = temp;
    }
}
Repository: TheAlgorithms/Java
File: src/test/java/com/thealgorithms/strings/IsomorphicTest.java (Java)

Code before:
package com.thealgorithms.strings;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.stream.Stream;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

public final class IsomorphicTest {

    @ParameterizedTest
    @MethodSource("isomorphicProvider")
    public void testCheckStrings(String str1, String str2, Boolean expected) {
        assertEquals(expected, Isomorphic.checkStrings(str1, str2));
    }

    private static Stream<Arguments> isomorphicProvider() {
        return Stream.of(Arguments.of("abbbbaac", "kffffkkd", Boolean.TRUE), Arguments.of("xyxyxy", "bnbnbn", Boolean.TRUE), Arguments.of("ghjknnmm", "wertpopo", Boolean.FALSE), Arguments.of("aaammmnnn", "ggghhhbbj", Boolean.FALSE));
    }
}


Review comment (suggestion):
What do you think about:
```suggestion
        assertEquals(expected, Isomorphic.checkStrings(str1, str2));
        assertEquals(expected, Isomorphic.checkStrings(str2, str1));
```

Code after:
package com.thealgorithms.strings;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.stream.Stream;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

public final class IsomorphicTest {

    @ParameterizedTest
    @MethodSource("inputs")
    public void testCheckStrings(String str1, String str2, Boolean expected) {
        assertEquals(expected, Isomorphic.checkStrings(str1, str2));
        assertEquals(expected, Isomorphic.checkStrings(str2, str1));
    }

    private static Stream<Arguments> inputs() {
        return Stream.of(Arguments.of("", "", Boolean.TRUE), Arguments.of("", "a", Boolean.FALSE), Arguments.of("aaa", "aa", Boolean.FALSE), Arguments.of("abbbbaac", "kffffkkd", Boolean.TRUE), Arguments.of("xyxyxy", "bnbnbn", Boolean.TRUE), Arguments.of("ghjknnmm", "wertpopo", Boolean.FALSE),
            Arguments.of("aaammmnnn", "ggghhhbbj", Boolean.FALSE));
    }
}
Repository: UnityTechnologies/open-project-1
File: UOP1_Project/Assets/Scripts/SpawnSystem.cs (C#)

Code before:
﻿using System;
using System.Linq;
using UnityEditor.PackageManager;
using UnityEngine;
using UnityEngine.Events;

[Serializable]
public class PlayerSpawnEvent : UnityEvent<Transform> { }

public class SpawnSystem : MonoBehaviour
{
    private static int requestedSpawnIndex = -1;

    [Header("Settings")]
    [SerializeField] private int defaultSpawnIndex = 0;

    [Header("Project References")]
#pragma warning disable 649
    [SerializeField] private Protagonist playerPrefab;
#pragma warning restore 649

    [Header("Scene References")]
    [SerializeField] private InputReader inputReader;
    [SerializeField] private Transform gameplayCamera;
    [SerializeField] private Transform[] spawnLocations;

    [Header("Events")]
    public PlayerSpawnEvent onPlayerSpawnEvent;

    void Awake()
    {
        try
        {
            Spawn();
        }
        catch (Exception e)
        {
            Debug.LogError($"[SpawnSystem] Failed to spawn player. {e.Message}");
        }
    }

    void Reset()
    {
        AutoFill();
    }


Review comment (suggestion):
I'ld rather see a full stacktrace in the logs rather than hiding an exception :/

Since this is run in Awake(), it won't prevent any other scripts from running.

Code after:
﻿using System;
using System.Linq;
using UnityEditor.PackageManager;
using UnityEngine;
using UnityEngine.Events;

public class SpawnSystem : MonoBehaviour
{
	[Header("Settings")]
	[SerializeField] private int _defaultSpawnIndex = 0;

	[Header("Project References")]
	[SerializeField] private Protagonist _playerPrefab = null;

	[Header("Scene References")]
	[SerializeField] private CameraManager _cameraManager;
	[SerializeField] private Transform[] _spawnLocations;

	void Awake()
	{
        try
        {
            Spawn(_defaultSpawnIndex);
        }
        catch (Exception e)
        {
            Debug.LogError($"[SpawnSystem] Failed to spawn player. {e.Message}");
        }
    }

	void Reset()
	{
		AutoFill();
	}

	[ContextMenu("Attempt Auto Fill")]
	private void AutoFill()
	{
		if(_cameraManager == null)
			_cameraManager = FindObjectOfType<CameraManager>();

		if (_spawnLocations == null || _spawnLocations.Length == 0)
			_spawnLocations = transform.GetComponentsInChildren<Transform>(true)
								.Where(t => t != this.transform)
								.ToArray();
	}
Repository: UnityTechnologies/open-project-1
File: UOP1_Project/Assets/Scripts/Pool/PoolSO.cs (C#)

Code before:
using UnityEngine;

namespace UOP1.Pool
{
	/// <summary>
	/// A generic pool that generates members of type T on-demand via a factory.
	/// </summary>
	/// <typeparam name="T">Specifies the type of elements to pool.</typeparam>
	public abstract class PoolSO<T> : ScriptableObject, IPool<T>
	{
		protected readonly Stack<T> _available = new Stack<T>();
		public abstract IFactory<T> Factory { get; set; }

		protected virtual T Create()
		{
			return Factory.Create();
		}

		public virtual T Request()
		{
			return _available.Count > 0 ? _available.Pop() : Create();
		}

		public virtual IEnumerable<T> Request(int num = 1)
		{
			List<T> members = new List<T>(num);
			for (int i = 0; i < num; i++)
			{
				members.Add(Request());
			}
			return members;
		}

		public virtual void Return(T member)
		{
			_available.Push(member);
		}

		public virtual void Return(IEnumerable<T> members)
		{
			foreach (T member in members)
			{
				Return(member);
			}
		}

		public virtual void OnDisable()
		{
			_available.Clear();
		}
	}

Review comment (suggestion):
So I made the decision to only allow prewarming of a pool once. However, this doesn't _need_ to be. We could just allow this method to be called and add however many objects to the pool whenever. In that case, it may make sense to call the method `Warm`.

Code after:
using UnityEngine;

namespace UOP1.Pool
{
	/// <summary>
	/// A generic pool that generates members of type T on-demand via a factory.
	/// </summary>
	/// <typeparam name="T">Specifies the type of elements to pool.</typeparam>
	public abstract class PoolSO<T> : ScriptableObject, IPool<T>
	{
		protected readonly Stack<T> Available = new Stack<T>();
		public abstract IFactory<T> Factory { get; set; }
		protected bool HasBeenPrewarmed { get; set; }

		protected virtual T Create()
		{
			return Factory.Create();
		}

		public virtual void Prewarm(int num)
		{
			if (HasBeenPrewarmed)
			{
				Debug.LogWarning($"Pool {name} has already been prewarmed.");
				return;
			}
			for (int i = 0; i < num; i++)
			{
				Available.Push(Create());
			}
			HasBeenPrewarmed = true;
		}

		public virtual T Request()
		{
			return Available.Count > 0 ? Available.Pop() : Create();
		}

		public virtual IEnumerable<T> Request(int num = 1)
		{
			List<T> members = new List<T>(num);
			for (int i = 0; i < num; i++)
			{
				members.Add(Request());
			}
			return members;
		}

		public virtual void Return(T member)
		{
			Available.Push(member);
Repository: UnityTechnologies/open-project-1
File: UOP1_Project/Assets/Scripts/Interaction/InteractionManager.cs (C#)

Code before:
	{
		ResetInteraction(other.gameObject);
	}

	private void ResetInteraction(GameObject obj)
	{
		Stack<Interaction> updatedStack = new Stack<Interaction>();
		while(_ongoingInteractions.Count > 0)
		{
			Interaction interaction = _ongoingInteractions.Pop();
			if (interaction.InteractableObject != obj)
			{
				updatedStack.Push(interaction);
			}
		}
		_ongoingInteractions = updatedStack;

		if (_ongoingInteractions.Count > 0)
		{
			_toggleInteractionUI.RaiseEvent(true, _ongoingInteractions.Peek().Type);

		}
		else
		{
			_toggleInteractionUI.RaiseEvent(false, InteractionType.None);
		}
	}
}


Review comment (suggestion):
How about if instead of creating new stack, you'd iterate over _ongoingInteractions and remove whatever interaction we've found. If the interaction we've found was the most recent one (the one we are currently interacting with) AND stack is not empty you call DisplayInteractionUI, othherwise _toggleInteractionUI.RaiseEvent(false, InteractionType.None);

This should be more optimal and slightly cleaner as you reuse DisplayInteraction method. 

Code after:
	private void OnTriggerExit(Collider other)
	{
		ResetInteraction(other.gameObject);
	}

	private void ResetInteraction(GameObject obj)
	{
		LinkedListNode<Interaction> currentNode = _ongoingInteractions.First;
		while (currentNode != null)
		{
			if (currentNode.Value.InteractableObject == obj)
			{
				_ongoingInteractions.Remove(currentNode);
				break;
			}
			currentNode = currentNode.Next;
		}

		if (_ongoingInteractions.Count > 0)
		{
			_toggleInteractionUI.RaiseEvent(true, _ongoingInteractions.First.Value.Type);
		}
		else
		{
			_toggleInteractionUI.RaiseEvent(false, InteractionType.None);
		}
	}
}
Repository: activeloopai/Hub
File: deeplake/util/dataset.py (Python)

Code before:
        for t in tensor_keys:
            if t not in tensors:
                raise TensorDoesNotExistError(t)

        tensor_keys = list(tensor_keys)

    # Get full path in case of groups
    return [tensors[k].meta.name or tensors[k].key for k in tensor_keys]


def sanitize_tensor_name(input: str) -> str:
    """Sanitize a string to be a valid tensor name

    Args:
        input (str): A string that will be sanitized

    Returns:
        str: A string with the sanitized tensor name
    """

    invalid_chars = ["[", "]", "@", ".", ",", "?", "!", "/", "\\", "#", "'", '"']
    new_char = "_"
    for char in invalid_chars:
        input = input.replace(char, new_char)

    return input


Review comment (security):
```suggestion
_invalid_chars = {"[", "]", "@", ".", ",", "?", "!", "/", "\\", "#", "'", '"'}
def sanitize_tensor_name(input: str) -> str:
    """Sanitize a string to be a valid tensor name

    Args:
        input (str): A string that will be sanitized

    Returns:
        str: A string with the sanitized tensor name
    """
    return "".join("_" if c in invalid_chars else c for c in input)
```

Code after:
from typing import Optional, Sequence, List
from deeplake.util.exceptions import ReadOnlyModeError, TensorDoesNotExistError


def try_flushing(ds):
    try:
        ds.storage.flush()
    except ReadOnlyModeError:
        pass


def map_tensor_keys(dataset, tensor_keys: Optional[Sequence[str]] = None) -> List[str]:
    """Sanitizes tensor_keys if not None, else returns all the keys present in the dataset."""

    tensors = dataset.tensors

    if tensor_keys is None:
        tensor_keys = list(tensors)
    else:
        for t in tensor_keys:
            if t not in tensors:
                raise TensorDoesNotExistError(t)

        tensor_keys = list(tensor_keys)

    # Get full path in case of groups
    return [tensors[k].meta.name or tensors[k].key for k in tensor_keys]


_invalid_chars = {"[", "]", "@", ".", ",", "?", "!", "/", "\\", "#", "'", '"'}


def sanitize_tensor_name(input: str) -> str:
    """Sanitize a string to be a valid tensor name

    Args:
        input (str): A string that will be sanitized

    Returns:
        str: A string with the sanitized tensor name
    """
    return "".join("_" if c in _invalid_chars else c for c in input)
Repository: activeloopai/Hub
File: deeplake/api/tests/test_reset.py (Python)

Code before:
    with ds:
        ds.create_tensor("abc")
        ds.abc.append(1)
        ds.commit()
        ds.checkout("alt", create=True)
        ds.abc.append(2)
        ds.commit()
        ds.checkout("main")
        ds.abc.append(3)
        ds.commit()

    saved = json.loads(ds.storage["version_control_info.json"].decode("utf-8"))

    for commit_id, commit in saved["commits"].items():
        commit["children"] = [
            c for c in commit["children"] if saved["commits"][c]["branch"] != "alt"
        ]
    alt_id = saved["branches"].pop("alt")
    del saved["commits"][alt_id]
    saved["commits"] = dict(
        filter(
            lambda x: x[0] != alt_id and x[1]["branch"] != "alt",
            saved["commits"].items(),
        )
    )

    ds.storage["version_control_info.json"] = json.dumps(saved).encode("utf-8")
    ds.storage.flush()

    ds = deeplake.load(local_path)

    with pytest.raises(CheckoutError):
        ds.checkout("alt")

    ds.fix_vc()

    ds.checkout("alt")

    np.testing.assert_array_equal(ds.abc.numpy(), [[1], [2]])


Review comment (bug):
Need to remove `alt` branch precisely in order to trigger the case where checkout will fail. If this is not done, version control info rebuild will be triggered during dataset load.

Code after:

    with ds:
        ds.create_tensor("abc")
        ds.abc.append(1)
        ds.commit()
        ds.checkout("alt", create=True)
        ds.abc.append(2)
        ds.commit()
        ds.checkout("main")
        ds.abc.append(3)
        ds.commit()

    saved = json.loads(ds.storage["version_control_info.json"].decode("utf-8"))

    for commit_id, commit in saved["commits"].items():
        commit["children"] = [
            c for c in commit["children"] if saved["commits"][c]["branch"] != "alt"
        ]
    alt_id = saved["branches"].pop("alt")
    del saved["commits"][alt_id]
    saved["commits"] = dict(
        filter(
            lambda x: x[0] != alt_id and x[1]["branch"] != "alt",
            saved["commits"].items(),
        )
    )

    ds.storage["version_control_info.json"] = json.dumps(saved).encode("utf-8")
    ds.storage.flush()

    ds = deeplake.load(local_path)

    with pytest.raises(CheckoutError):
        ds.checkout("alt")

    ds.fix_vc()

    ds.checkout("alt")

    np.testing.assert_array_equal(ds.abc.numpy(), [[1], [2]])


def test_missing_commit_infos(local_ds):
    with local_ds as ds:
        ds.create_tensor("abc")
        ds.abc.append(1)
        a = ds.commit()
        ds.abc.append(2)
        b = ds.commit()
        ds.checkout("alt", create=True)
        ds.abc.append(3)
Repository: activeloopai/Hub
File: deeplake/core/vectorstore/vector_search/filter/filter.py (Python)

Code before:
from deeplake.constants import MB
from deeplake.util.warnings import always_warn

import numpy as np

import uuid
from functools import partial
from typing import Optional, Any, Iterable, List, Dict, Callable, Union


def dp_filter_python(x: dict, filter: Dict) -> bool:
    """Filter helper function for Deep Lake"""

    result = True

    for tensor in filter.keys():
        data = x[tensor].data()["value"]

        if x[tensor].meta.htype == "json":
            result = result and all(
                k in data and v == data[k] for k, v in filter[tensor].items()
            )
        else:
            result = result and data == filter[tensor]

    return result


def attribute_based_filtering_python(
    view, filter: Optional[Union[Dict, Callable]] = None
):
    if len(view) == 0:
        raise ValueError("specified dataset is empty")
    if filter is not None:
        if isinstance(filter, dict):
            for tensor in filter.keys():
                if tensor not in view.tensors:
                    raise ValueError(
                        f"Tensor '{tensor}' is not present in the Vector Store."
                    )  # We keep this check outside of the partial function below in order to not run it on every iteration in the Deep Lake filter

            filter = partial(dp_filter_python, filter=filter)

Review comment (suggestion):
Consider adding `if result:` here so we don't need to iterate all the tensors if mismatch already found. Or maybe just return False when mismatch found and return True after the loop, we don't need result variable this way.

Code after:
from deeplake.constants import MB
from deeplake.util.warnings import always_warn

import numpy as np

import uuid
from functools import partial
from typing import Optional, Any, Iterable, List, Dict, Callable, Union


def dp_filter_python(x: dict, filter: Dict) -> bool:
    """Filter helper function for Deep Lake"""

    result = True

    for tensor in filter.keys():
        if result:  # Only evaluate more data if previous filters were True
            data = x[tensor].data()["value"]

            if x[tensor].meta.htype == "json":
                result = result and all(
                    k in data and v == data[k] for k, v in filter[tensor].items()
                )
            else:
                result = result and data == filter[tensor]

    return result


def attribute_based_filtering_python(
    view, filter: Optional[Union[Dict, Callable]] = None
):
    if len(view) == 0:
        raise ValueError("specified dataset is empty")
    if filter is not None:
        if isinstance(filter, dict):
            for tensor in filter.keys():
                if tensor not in view.tensors:
                    raise ValueError(
                        f"Tensor '{tensor}' is not present in the Vector Store."
                    )  # We keep this check outside of the partial function below in order to not run it on every iteration in the Deep Lake filter