.pr_agent_auto_best_practices

Pattern 1: Add null checks for required parameters and properties before using them, throwing ArgumentNullException with clear error messages that include the parameter name.

Example code before:

public void ProcessData(string input) {
    var result = input.ToUpper(); 
}

Example code after:

public void ProcessData(string input) {
    ArgumentNullException.ThrowIfNull(input, nameof(input));
    var result = input.ToUpper();
}

Relevant past accepted suggestions:

---

Suggestion 1:

Add null check for handler

Add null check for the returned HttpClientHandler from CreateHttpClientHandler() before using it to avoid potential NullReferenceException

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [248-252]

 protected virtual HttpClient CreateHttpClient()
 {
     var httpClientHandler = CreateHttpClientHandler();
+    if (httpClientHandler == null)
+    {
+        throw new InvalidOperationException("CreateHttpClientHandler() returned null");
+    }
 
     HttpMessageHandler handler = httpClientHandler;

---

Suggestion 2:

Add null validation check for required class field to prevent potential NullReferenceException

The CreateHttpClientHandler() method uses this.remoteServerUri without validating that it's not null. Since this is a required field for the class functionality, add a null check at the start of the method to fail fast with a clear error message.

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [229-233]

 protected virtual HttpClientHandler CreateHttpClientHandler()
 {
+    ArgumentNullException.ThrowIfNull(this.remoteServerUri, nameof(remoteServerUri));
     HttpClientHandler httpClientHandler = new HttpClientHandler();
     string userInfo = this.remoteServerUri.UserInfo;
     if (!string.IsNullOrEmpty(userInfo) && userInfo.Contains(":"))

---

Suggestion 3:

Add parameter validation

Add null checks for the subscription ID and event handler parameters to prevent potential NullReferenceException when unsubscribing.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [274-281]

 public async Task UnsubscribeAsync(Modules.Session.Subscription subscription, EventHandler eventHandler)
 {
+    ArgumentNullException.ThrowIfNull(subscription);
+    ArgumentNullException.ThrowIfNull(eventHandler);
+    
     var eventHandlers = _eventHandlers[eventHandler.EventName];
     eventHandlers.Remove(eventHandler);
     await _bidi.SessionModule.UnsubscribeAsync([subscription]).ConfigureAwait(false);
 }

---

Suggestion 4:

Add null parameter validation

Add null check for webDriver parameter to prevent NullReferenceException when calling the extension method with null

dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs [29-31]

 public static async Task<BiDi> AsBiDiAsync(this IWebDriver webDriver)
 {
+    ArgumentNullException.ThrowIfNull(webDriver);
     var webSocketUrl = ((IHasCapabilities)webDriver).Capabilities.GetCapability("webSocketUrl");

---

Suggestion 5:

Add parameter validation to prevent null reference exceptions

Add null check for key parameter in SetPreferenceValue method to prevent potential issues with null keys.

dotnet/src/webdriver/Firefox/Preferences.cs [166-168]

 private void SetPreferenceValue(string key, JsonNode? value)
 {
+    if (key == null)
+        throw new ArgumentNullException(nameof(key));
     if (!this.IsSettablePreference(key))

---

Pattern 2: Initialize collections and required properties with meaningful default values in constructors instead of using null or null-forgiving operators.

Example code before:

public class Request {
    public Dictionary<string, string> Headers { get; set; } = null!;
    public string Method { get; set; } = null!;
}

Example code after:

public class Request {
    public Dictionary<string, string> Headers { get; set; } = new();
    public string Method { get; set; } = "GET";
}

Relevant past accepted suggestions:

---

Suggestion 1:

Initialize collection to prevent nulls

Initialize the Headers dictionary in the parameterless constructor to prevent NullReferenceException when adding headers.

dotnet/src/webdriver/HttpRequestData.cs [34-39]

 public HttpRequestData()
 {
     this.Method = null!;
     this.Url = null!;
-    this.Headers = null!;
+    this.Headers = new Dictionary<string, string>();
 }

---

Suggestion 2:

Use meaningful defaults over nulls

Avoid using null-forgiving operator (!) for required properties. Instead, initialize with meaningful default values.

dotnet/src/webdriver/HttpRequestData.cs [34-39]

 public HttpRequestData()
 {
-    this.Method = null!;
-    this.Url = null!;
-    this.Headers = null!;
+    this.Method = "GET";
+    this.Url = string.Empty;
+    this.Headers = new Dictionary<string, string>();
 }

---

Suggestion 3:

Fix parameters initialization logic

The constructor assigns parameters directly to Parameters property only when not null, but the property already initializes a new dictionary. This can lead to inconsistent state. Consider removing the default initialization.

dotnet/src/webdriver/Command.cs [85]

-public Dictionary<string, object> Parameters { get; } = new Dictionary<string, object>();
+public Dictionary<string, object> Parameters { get; private set; } = new();

---

Suggestion 4:

Fix constructor initialization pattern

The constructor is not properly initializing the page size. The property pageSize should be set directly instead of using PageDimensions to avoid potential null checks and exceptions.

dotnet/src/webdriver/PrintOptions.cs [66-69]

 public PrintOptions()
 {
-    this.PageDimensions = A4; // Default to A4 page size
+    this.pageSize = A4; // Default to A4 page size
 }

---

Pattern 3: Use null-conditional (?.) and null-coalescing (??) operators instead of direct null checks or null-forgiving operators (!) for safer null handling.

Example code before:

string value = response.Value.ToString()!;

Example code after:

string value = response.Value?.ToString() ?? string.Empty;

Relevant past accepted suggestions:

---

Suggestion 1:

Add null checks for response

The PageSource property should handle null responses from Execute() to prevent potential NullReferenceException when accessing Value property.

dotnet/src/webdriver/WebDriver.cs [133-140]

 public string PageSource
 {
     get
     {
         Response commandResponse = this.Execute(DriverCommand.GetPageSource, null);
-
-        return commandResponse.Value.ToString();
+        return commandResponse?.Value?.ToString() ?? string.Empty;
     }
 }

---

Suggestion 2:

Add null check for handles array

The WindowHandles property should validate that handles is not null before attempting to create a new List with its length to prevent potential NullReferenceException.

dotnet/src/webdriver/WebDriver.cs [165-166]

-object[] handles = (object[])commandResponse.Value;
+object[] handles = (object[])commandResponse.Value ?? Array.Empty<object>();
 List<string> handleList = new List<string>(handles.Length);

---

Suggestion 3:

Prevent potential null reference

The Title property should check if commandResponse is null before using the null-coalescing operator on its Value property to avoid potential NullReferenceException.

dotnet/src/webdriver/WebDriver.cs [123]

-object returnedTitle = commandResponse?.Value ?? string.Empty;
+object returnedTitle = commandResponse == null ? string.Empty : commandResponse.Value ?? string.Empty;

---

Suggestion 4:

Use null-conditional operator for safer null checking

Use the null-forgiving operator (!) only when absolutely necessary. Consider using the null-conditional operator (?.) for a safer approach to null checking.

dotnet/src/webdriver/Alert.cs [50]

-return commandResponse.Value.ToString()!;
+return commandResponse.Value?.ToString() ?? string.Empty;

---

Pattern 4: Validate JSON response properties using TryGetProperty before accessing them to prevent JsonException.

Example code before:

var data = doc.RootElement.GetProperty("result").GetString();

Example code after:

if (!doc.RootElement.TryGetProperty("result", out JsonElement resultElement))
    throw new JsonException("Missing required 'result' property");
var data = resultElement.GetString();

Relevant past accepted suggestions:

---

Suggestion 1:

Validate JSON property existence

Add null check for the 'clientWindows' property existence in JSON to prevent potential JsonException

dotnet/src/webdriver/BiDi/Communication/Json/Converters/Enumerable/GetClientWindowsResultConverter.cs [35]

-var clientWindows = doc.RootElement.GetProperty("clientWindows").Deserialize<IReadOnlyList<ClientWindowInfo>>(options);
+if (!doc.RootElement.TryGetProperty("clientWindows", out JsonElement clientWindowsElement))
+    throw new JsonException("Missing required 'clientWindows' property");
+var clientWindows = clientWindowsElement.Deserialize<IReadOnlyList<ClientWindowInfo>>(options);

---

Suggestion 2:

Add null check for response value

Add null check for response.Value in GetContext() method before attempting ToString() conversion to avoid potential NullReferenceException.

dotnet/src/webdriver/Firefox/FirefoxDriver.cs [263-266]

 Response commandResponse = this.Execute(GetContextCommand, null);
 
-if (commandResponse.Value is not string response
+if (commandResponse.Value is null
+    || commandResponse.Value is not string response
     || !Enum.TryParse(response, ignoreCase: true, out FirefoxCommandContext output))

---

Suggestion 3:

Fix incorrect null check

The GetElementFromResponse method should throw NoSuchElementException when response.Value is null, not when response itself is null. The current implementation could mask underlying issues.

dotnet/src/webdriver/WebDriver.cs [524-527]

-if (response == null)
+if (response?.Value == null)
 {
     throw new NoSuchElementException();
 }

---

Pattern 5: Use proper cleanup and disposal patterns for resources like HttpClient, file handles, and authentication handlers.

Example code before:

private HttpClient client;
public void Close() {
    client = null;
}

Example code after:

private HttpClient? client;
public void Dispose()
{
    if (client != null) {
        client.Dispose();
        client = null;
    }
}

Relevant past accepted suggestions:

---

Suggestion 1:

Properly dispose of HTTP client resources to prevent memory leaks

Dispose of the HttpClient instance in the Dispose method to prevent resource leaks. The current implementation might leave the HttpClient hanging.

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [52]

 private HttpClient? client;
 
+public void Dispose()
+{
+    if (!isDisposed)
+    {
+        client?.Dispose();
+        client = null;
+        isDisposed = true;
+    }
+}
+

---

Suggestion 2:

Ensure proper cleanup of resources when removing authentication handlers

Implement a cleanup mechanism in the removeAuthenticationHandler method to ensure that any resources or intercepts associated with the handler are properly cleaned up to prevent memory leaks or dangling references.

javascript/node/selenium-webdriver/lib/network.js [78]

-this.#authHandlers.delete(id)
+if (!this.#authHandlers.has(id)) {
+  throw new Error(`No authentication handler found with ID: ${id}`);
+}
+// Perform any necessary cleanup related to the handler here
+this.#authHandlers.delete(id);
 

---

Suggestion 3:

Improve thread safety for managing authentication callbacks

Consider using a more thread-safe approach for managing AUTH_CALLBACKS. Instead of using a class variable, you could use a thread-local variable or a synchronized data structure to ensure thread safety in multi-threaded environments.

rb/lib/selenium/webdriver/common/network.rb [23-26]

-AUTH_CALLBACKS = {}
+def self.auth_callbacks
+  Thread.current[:auth_callbacks] ||= {}
+end
+
 def initialize(bridge)
   @network = BiDi::Network.new(bridge.bidi)
 end

---

[Auto-generated best practices - 2025-02-27]