.pr_agent_auto_best_practices

Pattern 1: Add null checks for potentially null parameters and properties before using them to prevent NullReferenceExceptions. Always validate that parameters and properties are not null before accessing their members or methods.

Example code before:

public Task SendAsJsonAsync<T>(T command, JsonSerializerContext jsonSerializerContext)
{
    return transport.SendJsonAsync(command.ToString());
}

Example code after:

public Task SendAsJsonAsync<T>(T command, JsonSerializerContext jsonSerializerContext)
{
    ArgumentNullException.ThrowIfNull(command, nameof(command));
    ArgumentNullException.ThrowIfNull(jsonSerializerContext, nameof(jsonSerializerContext));
    return transport.SendJsonAsync(command.ToString());
}

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 null checks for nullable delegates

The FindElementMethod and FindElementsMethod properties are marked as nullable but used without null checks in FindElement and FindElements methods. Add null checks to prevent potential NullReferenceException.

dotnet/src/webdriver/By.cs [332-333]

 public virtual IWebElement FindElement(ISearchContext context)
 {
+    if (this.FindElementMethod == null)
+    {
+        throw new InvalidOperationException("FindElementMethod not set");
+    }
     return this.FindElementMethod(context);
 }

Suggestion 4:

Add null validation checks for nullable delegate properties before invoking them to prevent NullReferenceExceptions

The FindElement and FindElements methods should validate that their FindElementMethod and FindElementsMethod properties are not null before using them, since they are marked as nullable. Add ArgumentNullException checks at the start of these methods.

dotnet/src/webdriver/By.cs [330-344]

 public virtual IWebElement FindElement(ISearchContext context)
 {
+    ArgumentNullException.ThrowIfNull(this.FindElementMethod, nameof(FindElementMethod));
     return this.FindElementMethod(context);
 }
 
-public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context) 
+public virtual ReadOnlyCollection<IWebElement> FindElements(ISearchContext context)
 {
+    ArgumentNullException.ThrowIfNull(this.FindElementsMethod, nameof(FindElementsMethod)); 
     return this.FindElementsMethod(context);
 }

Suggestion 5:

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 6:

Add null validation checks for nullable parameters at the start of methods to prevent NullReferenceExceptions

Add ArgumentNullException.ThrowIfNull() checks for nullable parameters 'attributeName', 'propertyName', and 'text' at the start of GetAttribute(), GetDomAttribute(), GetDomProperty(), GetCssValue() and SendKeys() methods to prevent potential NullReferenceExceptions.

dotnet/src/webdriver/WebElement.cs [445-452]

 public virtual string? GetAttribute(string attributeName)
 {
+    ArgumentNullException.ThrowIfNull(attributeName, nameof(attributeName));
+    
     Dictionary<string, object> parameters = new Dictionary<string, object>();
     string atom = GetAtom("get-attribute.js");
     parameters.Add("script", atom);
     parameters.Add("args", new object[] { ((IWebDriverObjectReference)this).ToDictionary(), attributeName });

Suggestion 7:

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 8:

Add proper validation for nullable values before using them to prevent potential runtime errors

The code uses null-forgiving operator (!) on webSocketUrl.ToString() which could lead to runtime errors if webSocketUrl is null. Instead, validate the value before using it and provide a clear error message.

dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs [33-35]

-if (webSocketUrl is null) throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");
-var bidi = await BiDi.ConnectAsync(webSocketUrl.ToString()!).ConfigureAwait(false);
+if (webSocketUrl is null)
+    throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");
 
+var webSocketUrlStr = webSocketUrl.ToString();
+if (string.IsNullOrEmpty(webSocketUrlStr))
+    throw new BiDiException("Invalid empty webSocketUrl value");
+    
+var bidi = await BiDi.ConnectAsync(webSocketUrlStr).ConfigureAwait(false);
+

Suggestion 9:

Add null check for required parameter to prevent runtime errors

Add null check for target parameter in ConvertElement() since it's marked as nullable but the method assumes it's non-null.

dotnet/src/webdriver/Interactions/PointerInputDevice.cs [584-588]

 private Dictionary<string, object> ConvertElement()
 {
+    if (this.target == null)
+    {
+        throw new ArgumentNullException(nameof(target));
+    }
     if (this.target is IWebDriverObjectReference element)
     {
         return element.ToDictionary();
     }

Suggestion 10:

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: Use null-conditional operators (?.) and null-coalescing operators (??) instead of null-forgiving operators (!) to safely handle potentially null values and provide default values when needed.

Example code before:

return commandResponse.Value.ToString()!;

Example code after:

return commandResponse.Value?.ToString() ?? string.Empty;

Relevant past accepted suggestions:

Suggestion 1:

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 2:

Add null check for path

Add null check for Path.GetDirectoryName() result since it can return null for invalid paths. The current forced null-forgiving operator (!) could lead to NullReferenceException.

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [203]

-driverPath = Path.GetDirectoryName(driverPath)!;
+var dirPath = Path.GetDirectoryName(driverPath);
+if (dirPath == null) throw new ArgumentException("Invalid driver path provided", nameof(driverPath));
+driverPath = dirPath;

Suggestion 3:

Add proper validation for nullable values before using them to prevent potential runtime errors

The code uses null-forgiving operator (!) on webSocketUrl.ToString() which could lead to runtime errors if webSocketUrl is null. Instead, validate the value before using it and provide a clear error message.

dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs [33-35]

-if (webSocketUrl is null) throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");
-var bidi = await BiDi.ConnectAsync(webSocketUrl.ToString()!).ConfigureAwait(false);
+if (webSocketUrl is null)
+    throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");
 
+var webSocketUrlStr = webSocketUrl.ToString();
+if (string.IsNullOrEmpty(webSocketUrlStr))
+    throw new BiDiException("Invalid empty webSocketUrl value");
+    
+var bidi = await BiDi.ConnectAsync(webSocketUrlStr).ConfigureAwait(false);
+

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 3: Initialize collections and properties with safe default values instead of null to prevent potential NullReferenceExceptions when the collections are accessed or modified later.

Example code before:

public Func<HttpRequestData, bool> RequestMatcher { get; set; } = null!;

Example code after:

public Func<HttpRequestData, bool> RequestMatcher { get; set; } = _ => false;

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:

Avoid null reference runtime exceptions

Initialize RequestMatcher in the constructor instead of using null! to avoid potential null reference exceptions at runtime.

dotnet/src/webdriver/NetworkRequestHandler.cs [35]

-public Func<HttpRequestData, bool> RequestMatcher { get; set; } = null!;
+public Func<HttpRequestData, bool> RequestMatcher { get; set; } = _ => false;

Suggestion 4:

Provide safe default function implementations

Initialize ResponseMatcher and ResponseTransformer in the constructor with safe default implementations instead of using null! to avoid potential null reference exceptions.

dotnet/src/webdriver/NetworkResponseHandler.cs [35-42]

-public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = null!;
-public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = null!;
+public Func<HttpResponseData, bool> ResponseMatcher { get; set; } = _ => false;
+public Func<HttpResponseData, HttpResponseData> ResponseTransformer { get; set; } = response => response;

Suggestion 5:

Add safe default function implementation

Initialize UriMatcher with a safe default implementation instead of using null! to avoid potential null reference exceptions.

dotnet/src/webdriver/NetworkAuthenticationHandler.cs [35]

-public Func<Uri, bool> UriMatcher { get; set; } = null!;
+public Func<Uri, bool> UriMatcher { get; set; } = _ => false;

Pattern 4: Improve error message clarity by providing specific details about the error context, including the actual values that caused the error and clear instructions for resolution.

Example code before:

throw new Error(`Params must be an instance of PartitionDescriptor. Received:'${partition}'`);

Example code after:

throw new Error(`Params must be an instance of BrowsingContextPartitionDescriptor or StorageKeyPartitionDescriptor. Received:'${partition}'`);

Relevant past accepted suggestions:

Suggestion 1:

Improve error message clarity

The error message should be more specific about the expected types. Currently it mentions "PartitionDescriptor" which is a parent class, but the check is specifically for BrowsingContextPartitionDescriptor or StorageKeyPartitionDescriptor.

javascript/node/selenium-webdriver/bidi/storage.js [56-61]

 if (
   partition !== undefined &&
   !(partition instanceof BrowsingContextPartitionDescriptor || partition instanceof StorageKeyPartitionDescriptor)
 ) {
-  throw new Error(`Params must be an instance of PartitionDescriptor. Received:'${partition}'`)
+  throw new Error(`Params must be an instance of BrowsingContextPartitionDescriptor or StorageKeyPartitionDescriptor. Received:'${partition}'`)
 }

Suggestion 2:

Fix incorrect docstring description

The docstring for web_socket_url incorrectly states it's for accepting insecure certificates. It should describe WebSocket URL functionality instead.

py/selenium/webdriver/common/options.py [287-288]

 web_socket_url = _BaseOptionsDescriptor("webSocketUrl")
-"""Gets and Set whether the session accepts insecure certificates.
+"""Gets and Sets WebSocket URL.

Suggestion 3:

Improve error message clarity

Add a more descriptive error message that includes the context of why the value was expected to be non-null, to help with debugging.

dotnet/src/webdriver/Response.cs [225]

-return Value ?? throw new WebDriverException("Expected not-null response");
+return Value ?? throw new WebDriverException("Response.Value was null when a non-null value was expected");

Suggestion 4:

Enhance error messages with actual values for better debugging experience

Include the actual value in the exception message to help with debugging when an invalid cookie name is provided.

dotnet/src/webdriver/CookieJar.cs [82]

-throw new ArgumentException("Cookie name cannot be empty", nameof(name));
+throw new ArgumentException($"Cookie name cannot be empty. Provided value: '{name}'", nameof(name));

Suggestion 5:

Provide accurate error messages that reflect the actual system locale instead of assuming Arabic

The error message incorrectly assumes the system language is Arabic when it's any non-English locale. Update the message to be more accurate and include the actual locale in the error message.

java/src/org/openqa/selenium/chrome/ChromeDriverService.java [289]

-throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
+throw new NumberFormatException("Couldn't format the port numbers due to non-English system locale '" + Locale.getDefault(Locale.Category.FORMAT) + "': \"" + String.format("--port=%d", getPort()) +

Pattern 5: Use locale-independent string formatting for numeric values to ensure consistent behavior across different system locales, especially when formatting command-line arguments.

Example code before:

args.add(String.format("--port=%d", getPort()));

Example code after:

args.add("--port=" + String.valueOf(getPort()));

Relevant past accepted suggestions:

Suggestion 1:

Use locale-independent number formatting to handle port numbers consistently across all system languages

Instead of checking only for non-English locale, implement proper number formatting using a Locale-independent approach. Use String.valueOf() or Integer.toString() for port number conversion to avoid locale-specific formatting issues.

java/src/org/openqa/selenium/chrome/ChromeDriverService.java [287-291]

-args.add(String.format("--port=%d", getPort()));
-if(!Locale.getDefault(Locale.Category.FORMAT).getLanguage().equals("en")) {
-  throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
-  "\", please make sure to add the required arguments \"-Duser.language=en -Duser.region=US\" to your JVM, for more info please visit :" + "\n  https://www.selenium.dev/documentation/webdriver/browsers/");
-}
+args.add("--port=" + String.valueOf(getPort()));

Suggestion 2:

Ensure consistent locale-independent number formatting across all port number handling

The websocket port formatting is inconsistent with the main port handling and could face the same locale issues. Apply the same locale-independent formatting to the websocket port.

java/src/org/openqa/selenium/firefox/GeckoDriverService.java [223-230]

-args.add(String.format("--port=%d", getPort()));
-if(!Locale.getDefault(Locale.Category.FORMAT).getLanguage().equals("en")) {
-  throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
-  "\", please make sure to add the required arguments \"-Duser.language=en -Duser.region=US\" to your JVM, for more info please visit :" + "\n  https://www.selenium.dev/documentation/webdriver/browsers/");
-}
+args.add("--port=" + String.valueOf(getPort()));
+int wsPort = PortProber.findFreePort();
+args.add("--websocket-port=" + String.valueOf(wsPort));
 
-int wsPort = PortProber.findFreePort();
-args.add(String.format("--websocket-port=%d", wsPort));
-

Suggestion 3:

Provide accurate error messages that reflect the actual system locale instead of assuming Arabic

The error message incorrectly assumes the system language is Arabic when it's any non-English locale. Update the message to be more accurate and include the actual locale in the error message.

java/src/org/openqa/selenium/chrome/ChromeDriverService.java [289]

-throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
+throw new NumberFormatException("Couldn't format the port numbers due to non-English system locale '" + Locale.getDefault(Locale.Category.FORMAT) + "': \"" + String.format("--port=%d", getPort()) +

[Auto-generated best practices - 2025-03-29]