ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/PowerToys
File: src/common/ManagedCommon/Logger.cs (C#)

Code before:
        {
            Log(message, Debug, memberName, sourceFilePath, sourceLineNumber);
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        public static void LogTrace([System.Runtime.CompilerServices.CallerMemberName] string memberName = "", [System.Runtime.CompilerServices.CallerFilePath] string sourceFilePath = "", [System.Runtime.CompilerServices.CallerLineNumber] int sourceLineNumber = 0)
        {
            Log(string.Empty, TraceFlag, memberName, sourceFilePath, sourceLineNumber);
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        private static void Log(string message, string type, string memberName, string sourceFilePath, int sourceLineNumber)
        {
            Trace.WriteLine("[" + DateTime.Now.TimeOfDay + "] [" + type + "] " + GetCallerInfo(memberName, sourceFilePath, sourceLineNumber));
            Trace.Indent();
            if (message != string.Empty)
            {
                Trace.WriteLine(message);
            }

            Trace.Unindent();
        }

        private static string GetCallerInfo(string memberName, string sourceFilePath, int sourceLineNumber)
        {
            string fileName = Path.GetFileName(sourceFilePath);

            return $"{fileName}::{memberName}::{sourceLineNumber}";
        }
    }
}


Review comment (suggestion):
May be Path.GetFileNameWithoutExtension?
Looks like this will throw exception as well, although I don't see why. Just to be safe in case.

Code after:
                Trace.WriteLine(message);
            }

            Trace.Unindent();
        }

        private static string GetCallerInfo(string memberName, string sourceFilePath, int sourceLineNumber)
        {
            string callerFileName = "Unknown";

            try
            {
                string fileName = Path.GetFileName(sourceFilePath);
                if (!string.IsNullOrEmpty(fileName))
                {
                    callerFileName = fileName;
                }
            }
            catch (Exception)
            {
                callerFileName = "Unknown";
#if DEBUG
                throw;
#endif
            }

            return $"{callerFileName}::{memberName}::{sourceLineNumber}";
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/AdvancedPaste/AdvancedPaste.FuzzTests/FuzzTests.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using AdvancedPaste.Helpers;
using Windows.ApplicationModel.DataTransfer;

// OneFuzz currently does not support .NET 9 code testing, so this is a temporary solution.
// Create a .NET 8 project and use a file link to include the code for testing first.
namespace AdvancedPaste.FuzzTests
{
    public class FuzzTests
    {
        public static void FuzzToJsonFromXmlOrCsv(ReadOnlySpan<byte> input)
        {
            var dataPackage = new DataPackage();
            dataPackage.SetText(input.ToString());
            _ = Task.Run(async () => await JsonHelper.ToJsonFromXmlOrCsvAsync(dataPackage.GetView())).Result;
        }
    }
}


Review comment (suggestion):
I think about a little bit here.
Given this is first example of writing this FuzzTest  wrapper, so that other can refer to it:
1. Let's still wrap up with try/catch here, although I know our inner code does, while this code should have NO assumption on the internal implementation.
2. Let's add a link  as comment to fuzzing doc for the code sample.


Code after:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using AdvancedPaste.Helpers;
using Windows.ApplicationModel.DataTransfer;

// OneFuzz currently does not support .NET 9 code testing, so this is a temporary solution.
// Create a .NET 8 project and use a file link to include the code for testing first.
namespace AdvancedPaste.FuzzTests
{
    public class FuzzTests
    {
        public static void FuzzToJsonFromXmlOrCsv(ReadOnlySpan<byte> input)
        {
            try
            {
                var dataPackage = new DataPackage();
                dataPackage.SetText(input.ToString());
                _ = Task.Run(async () => await JsonHelper.ToJsonFromXmlOrCsvAsync(dataPackage.GetView())).Result;
            }
            catch (Exception ex) when (ex is ArgumentException)
            {
                // This is an example. It's important to filter out any *expected* exceptions from our code here.
                // However, catching all exceptions is considered an anti-pattern because it may suppress legitimate
                // issues, such as a NullReferenceException thrown by our code. In this case, we still re-throw
                // the exception, as the ToJsonFromXmlOrCsvAsync method is not expected to throw any exceptions.
                throw;
            }
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/Workspaces/WorkspacesLib/AppUtils.h (C/C++)

Code before:
#include <WorkspacesLib/WorkspacesData.h>

namespace Utils
{
    namespace Apps
    {
        struct AppData
        {
            std::wstring name;
            std::wstring installPath;
            std::wstring packageFullName;
            std::wstring appUserModelId;
            std::wstring pwaAppId;
            bool canLaunchElevated = false;

            bool IsEdge() const;
            bool IsChrome() const;
        };

        using AppList = std::vector<AppData>;

        const std::wstring& GetCurrentFolder();
        const std::wstring& GetCurrentFolderUpper();

        const std::wstring GetGuidFromHwnd(HWND window);

        AppList GetAppsList();
        std::optional<AppData> GetApp(const std::wstring& appPath, DWORD pid, const AppList& apps);
        std::optional<AppData> GetApp(HWND window, const AppList& apps);

        bool UpdateAppVersion(WorkspacesData::WorkspacesProject::Application& app, const AppList& installedApps);
        bool UpdateWorkspacesApps(WorkspacesData::WorkspacesProject& workspace, const AppList& installedApps);
    }
}

Review comment (refactor):
Let's continue managing properties in a single location. Please move this function to `WorkspacesWindowPropertyUtils.h` 

Code after:
#include <WorkspacesLib/WorkspacesData.h>

namespace Utils
{
    namespace Apps
    {
        struct AppData
        {
            std::wstring name;
            std::wstring installPath;
            std::wstring packageFullName;
            std::wstring appUserModelId;
            std::wstring pwaAppId;
            bool canLaunchElevated = false;

            bool IsEdge() const;
            bool IsChrome() const;
        };

        using AppList = std::vector<AppData>;

        const std::wstring& GetCurrentFolder();
        const std::wstring& GetCurrentFolderUpper();

        AppList GetAppsList();
        std::optional<AppData> GetApp(const std::wstring& appPath, DWORD pid, const AppList& apps);
        std::optional<AppData> GetApp(HWND window, const AppList& apps);

        bool UpdateAppVersion(WorkspacesData::WorkspacesProject::Application& app, const AppList& installedApps);
        bool UpdateWorkspacesApps(WorkspacesData::WorkspacesProject& workspace, const AppList& installedApps);
    }
}
Repository: microsoft/PowerToys
File: src/modules/Workspaces/WorkspacesLib/utils.h (C/C++)

Code before:
#pragma once

#include <vector>
#include <string>

#include <workspaces-common/GuidUtils.h>
#include <workspaces-common/InvokePoint.h>

namespace NonLocalizable
{
    const wchar_t restartedString[] = L"restarted";
    const wchar_t needGUIDString[] = L"needGUID";
}

struct CommandLineArgs
{
    std::wstring workspaceId;
    InvokePoint invokePoint;
    bool isRestarted;
    bool isGuidNeeded;
};

CommandLineArgs split(std::wstring s, const std::wstring& delimiter)
{
    CommandLineArgs cmdArgs{};
    cmdArgs.isRestarted = false;
    cmdArgs.isGuidNeeded = false;

    size_t pos = 0;
    std::wstring token;
    std::vector<std::wstring> tokens;
    while ((pos = s.find(delimiter)) != std::wstring::npos)
    {
        token = s.substr(0, pos);
        tokens.push_back(token);
        s.erase(0, pos + delimiter.length());
    }

Review comment (suggestion):
The one minor thing I want to improve here for clarity is `needGUID` name (including all usages). It's not entirely clear what `needGUID` means. I suggest renaming the argument and usage in the code or using arguments that we have already implemented, such as the InvokePoint enum we send to the Launcher. SnapshotTool also becomes dependent on the invoke point, as we read window GUIDs when in "Launch and Edit" mode, so it seems okay to use the same enum. 

Code after:
#pragma once

#include <vector>
#include <string>

#include <workspaces-common/GuidUtils.h>
#include <workspaces-common/InvokePoint.h>

namespace NonLocalizable
{
    const wchar_t restartedString[] = L"restarted";
}

struct CommandLineArgs
{
    std::wstring workspaceId;
    InvokePoint invokePoint;
    bool isRestarted;
};

CommandLineArgs split(std::wstring s, const std::wstring& delimiter)
{
    CommandLineArgs cmdArgs{};
    cmdArgs.isRestarted = false;

    size_t pos = 0;
    std::wstring token;
    std::vector<std::wstring> tokens;
    while ((pos = s.find(delimiter)) != std::wstring::npos)
    {
        token = s.substr(0, pos);
        tokens.push_back(token);
        s.erase(0, pos + delimiter.length());
    }
    tokens.push_back(s);

    for (const auto& token : tokens)
Repository: microsoft/PowerToys
File: src/modules/launcher/Wox.Plugin/Result.cs (C#)

Code before:
                           (SubTitle?.GetHashCode(StringComparison.Ordinal) ?? 0);
            return hashcode;
        }

        public override string ToString()
        {
            // Using CurrentCulture since this is user facing
            return string.Format(CultureInfo.CurrentCulture, "{0} : {1}", Title, SubTitle);
        }

        /// <summary>
        /// Gets or sets additional data associate with this result
        /// </summary>
        public object ContextData { get; set; }

        /// <summary>
        /// Gets plugin ID that generated this result
        /// </summary>
        public string PluginID { get; internal set; }

        /// <summary>
        /// Gets or sets a value indicating whether usage based sorting should be applied to this result.
        /// </summary>
        public bool DisableUsageBasedScoring { get; set; }

        public int GetSortOrder(int selectedItemMultiplier)
        {
            if (DisableUsageBasedScoring)
            {
                return Metadata.WeightBoost + Score;
            }

            return Metadata.WeightBoost + Score + (SelectedCount * selectedItemMultiplier);
        }
    }
}


Review comment (suggestion):
Small improvement:

```suggestion
        public int GetSortOrderScore(int selectedItemMultiplier)
```


Code after:
                           (SubTitle?.GetHashCode(StringComparison.Ordinal) ?? 0);
            return hashcode;
        }

        public override string ToString()
        {
            // Using CurrentCulture since this is user facing
            return string.Format(CultureInfo.CurrentCulture, "{0} : {1}", Title, SubTitle);
        }

        /// <summary>
        /// Gets or sets additional data associate with this result
        /// </summary>
        public object ContextData { get; set; }

        /// <summary>
        /// Gets plugin ID that generated this result
        /// </summary>
        public string PluginID { get; internal set; }

        /// <summary>
        /// Gets or sets a value indicating whether usage based sorting should be applied to this result.
        /// </summary>
        public bool DisableUsageBasedScoring { get; set; }

        public int GetSortOrderScore(int selectedItemMultiplier)
        {
            if (DisableUsageBasedScoring)
            {
                return Metadata.WeightBoost + Score;
            }

            return Metadata.WeightBoost + Score + (SelectedCount * selectedItemMultiplier);
        }
    }
}
Repository: microsoft/PowerToys
File: src/settings-ui/Settings.UI/SettingsXAML/OOBE/Views/OobeWhatsNew.xaml.cs (C#)

Code before:
                DataDiagnosticsSettings.SetEnabledValue(true);
            }
            else
            {
                DataDiagnosticsSettings.SetEnabledValue(false);
            }

            DataDiagnosticsSettings.SetUserActionValue(true);

            this.DispatcherQueue.TryEnqueue(Microsoft.UI.Dispatching.DispatcherQueuePriority.Normal, () =>
            {
                ShellPage.ShellHandler?.SignalGeneralDataUpdate();
            });
        }

        private void DataDiagnostics_InfoBar_Close_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e)
        {
            WhatsNewDataDiagnosticsInfoBar.Visibility = Microsoft.UI.Xaml.Visibility.Collapsed;
        }

        private void DataDiagnostics_OpenSettings_Click(Microsoft.UI.Xaml.Documents.Hyperlink sender, Microsoft.UI.Xaml.Documents.HyperlinkClickEventArgs args)
        {
            Common.UI.SettingsDeepLink.OpenSettings(Common.UI.SettingsDeepLink.SettingsWindow.Overview, true);
        }

        private void SetInfoBar(InfoBar infoBar, bool open)
        {
            infoBar.IsOpen = open;
            infoBar.IsTabStop = open;
        }
    }
}


Review comment (suggestion):
Honestly, you could do without this new function. Setting the **IsOpen** property directly is fine. If you want to keep this, then I suggest a name like `SetInfoBarIsOpen` to be specific.

Code after:
            }

            WhatsNewDataDiagnosticsInfoBarDescText.Visibility = Microsoft.UI.Xaml.Visibility.Collapsed;
            WhatsNewDataDiagnosticsInfoBarDescTextYesClicked.Visibility = Microsoft.UI.Xaml.Visibility.Visible;
            DataDiagnosticsButtonYes.Visibility = Microsoft.UI.Xaml.Visibility.Collapsed;
            DataDiagnosticsButtonNo.Visibility = Microsoft.UI.Xaml.Visibility.Collapsed;

            // Set Data Diagnostics registry values
            if (commandArg == "Yes")
            {
                DataDiagnosticsSettings.SetEnabledValue(true);
            }
            else
            {
                DataDiagnosticsSettings.SetEnabledValue(false);
            }

            DataDiagnosticsSettings.SetUserActionValue(true);

            this.DispatcherQueue.TryEnqueue(Microsoft.UI.Dispatching.DispatcherQueuePriority.Normal, () =>
            {
                ShellPage.ShellHandler?.SignalGeneralDataUpdate();
            });
        }

        private void DataDiagnostics_InfoBar_Close_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e)
        {
            WhatsNewDataDiagnosticsInfoBar.Visibility = Microsoft.UI.Xaml.Visibility.Collapsed;
        }

        private void DataDiagnostics_OpenSettings_Click(Microsoft.UI.Xaml.Documents.Hyperlink sender, Microsoft.UI.Xaml.Documents.HyperlinkClickEventArgs args)
        {
            Common.UI.SettingsDeepLink.OpenSettings(Common.UI.SettingsDeepLink.SettingsWindow.Overview, true);
        }

        private async void LoadReleaseNotes_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e)
        {
            await Reload();
        }
    }
}
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.System/Helpers/Icons.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CommandPalette.Extensions.Toolkit;

namespace Microsoft.CmdPal.Ext.System.Helpers;

public static partial class Icons
{
    public static IconInfo FirmwareSettingsIcon { get; } = new IconInfo("\uEBC4");

    public static IconInfo LockIcon { get; } = new IconInfo("\uE72E");

    public static IconInfo LogoffIcon { get; } = new IconInfo("\uF3B1");

    public static IconInfo NetworkAdapterIcon { get; } = new IconInfo("\uEDA3");

    public static IconInfo RecycleBinIcon { get; } = new IconInfo("\uE74D");

    public static IconInfo RestartIcon { get; } = new IconInfo("\uE777");

    public static IconInfo ShutdownIcon { get; } = new IconInfo("\uE7E8");

    public static IconInfo SleepIcon { get; } = new IconInfo("\uE708");
}


Review comment (suggestion):
okay so this replaces 
![image](https://github.com/user-attachments/assets/afe0046f-b50d-4158-859e-7a9b2d419aa8)

with 

![image](https://github.com/user-attachments/assets/4ae86735-fd2c-403f-9dc0-bce05eecd582)


Code after:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CommandPalette.Extensions.Toolkit;

namespace Microsoft.CmdPal.Ext.System.Helpers;

public static partial class Icons
{
    public static IconInfo FirmwareSettingsIcon { get; } = new IconInfo("\uE950");

    public static IconInfo LockIcon { get; } = new IconInfo("\uE72E");

    public static IconInfo LogoffIcon { get; } = new IconInfo("\uF3B1");

    public static IconInfo NetworkAdapterIcon { get; } = new IconInfo("\uEDA3");

    public static IconInfo RecycleBinIcon { get; } = new IconInfo("\uE74D");

    public static IconInfo RestartIcon { get; } = new IconInfo("\uE777");

    public static IconInfo ShutdownIcon { get; } = new IconInfo("\uE7E8");

    public static IconInfo SleepIcon { get; } = new IconInfo("\uE708");
}
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.System/Helpers/Icons.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CommandPalette.Extensions.Toolkit;

namespace Microsoft.CmdPal.Ext.System.Helpers;

public static partial class Icons
{
    public static IconInfo FirmwareSettingsIcon { get; } = new IconInfo("\uEBC4");

    public static IconInfo LockIcon { get; } = new IconInfo("\uE72E");

    public static IconInfo LogoffIcon { get; } = new IconInfo("\uF3B1");

    public static IconInfo NetworkAdapterIcon { get; } = new IconInfo("\uEDA3");

    public static IconInfo RecycleBinIcon { get; } = new IconInfo("\uE74D");

    public static IconInfo RestartIcon { get; } = new IconInfo("\uE777");

    public static IconInfo ShutdownIcon { get; } = new IconInfo("\uE7E8");

    public static IconInfo SleepIcon { get; } = new IconInfo("\uE708");
}


Review comment (suggestion):
before
![image](https://github.com/user-attachments/assets/9c7d703f-0745-495d-9cf5-09eb058f7c31)


after
![image](https://github.com/user-attachments/assets/e7c69921-b91e-4e9e-9191-ff221cce4041)

(the same)

Code after:
﻿// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CommandPalette.Extensions.Toolkit;

namespace Microsoft.CmdPal.Ext.System.Helpers;

public static partial class Icons
{
    public static IconInfo FirmwareSettingsIcon { get; } = new IconInfo("\uE950");

    public static IconInfo LockIcon { get; } = new IconInfo("\uE72E");

    public static IconInfo LogoffIcon { get; } = new IconInfo("\uF3B1");

    public static IconInfo NetworkAdapterIcon { get; } = new IconInfo("\uEDA3");

    public static IconInfo RecycleBinIcon { get; } = new IconInfo("\uE74D");

    public static IconInfo RestartIcon { get; } = new IconInfo("\uE777");

    public static IconInfo ShutdownIcon { get; } = new IconInfo("\uE7E8");

    public static IconInfo SleepIcon { get; } = new IconInfo("\uE708");
}
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Exts/Microsoft.CmdPal.Ext.WindowsServices/ServiceResult.cs (C#)

Code before:

    public ServiceStartMode StartMode { get; private set; }

    public bool IsRunning { get; private set; }

    public ServiceResult(ServiceController serviceController)
    {
        ArgumentNullException.ThrowIfNull(serviceController);

        ServiceName = serviceController.ServiceName;
        DisplayName = serviceController.DisplayName;
        StartMode = serviceController.StartType;
        IsRunning = serviceController.Status != ServiceControllerStatus.Stopped && serviceController.Status != ServiceControllerStatus.StopPending;
    }

    private ServiceResult()
    {
    }

    public static ServiceResult FromServiceController(ServiceController serviceController)
    {
        try
        {
            ArgumentNullException.ThrowIfNull(serviceController);
            var result = new ServiceResult();
            result.ServiceName = serviceController.ServiceName;
            result.DisplayName = serviceController.DisplayName;
            result.StartMode = serviceController.StartType;
            result.IsRunning = serviceController.Status != ServiceControllerStatus.Stopped && serviceController.Status != ServiceControllerStatus.StopPending;

            return result;
        }
        catch (Exception)
        {
            // try to log the exception in the future
            // retrieve properties from serviceController will throw exception. Such as PlatformNotSupportedException.
        }

        return null;
    }
}


Review comment (bug):
From the bug, looks like the "StartType" cannot be get only.
I will suggest if we can get other important information like ServiceName/DisplayName, other throwing exception we can just show as unknown.

Code after:

    public ServiceStartMode StartMode { get; }

    public bool IsRunning { get; }

    private ServiceResult(ServiceController serviceController)
    {
        ArgumentNullException.ThrowIfNull(serviceController);

        ServiceName = serviceController.ServiceName;
        DisplayName = serviceController.DisplayName;
        StartMode = serviceController.StartType;
        IsRunning = serviceController.Status != ServiceControllerStatus.Stopped && serviceController.Status != ServiceControllerStatus.StopPending;
    }

    public static ServiceResult CreateServiceController(ServiceController serviceController)
    {
        try
        {
            var result = new ServiceResult(serviceController);

            return result;
        }
        catch (Exception)
        {
            // try to log the exception in the future
            // retrieve properties from serviceController will throw exception. Such as PlatformNotSupportedException.
        }

        return null;
    }
}