.pr_agent_auto_best_practices

Pattern 1: Replace direct property modification with creation of new immutable instances to maintain object immutability. Instead of modifying properties directly, create new instances with the desired values.

Example code before:

response.Value = newValue;

Example code after:

response = new Response(response.SessionId, newValue, response.Status);

Relevant past accepted suggestions:

---

Suggestion 1:

Replace direct property modification with creation of new immutable instance to maintain object immutability

Instead of using the deprecated Value setter, create a new Response instance with the modified value to maintain immutability principles.

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

-response.Value = valueString.Replace("\r\n", "\n").Replace("\n", Environment.NewLine);
+response = new Response(response.SessionId, valueString.Replace("\r\n", "\n").Replace("\n", Environment.NewLine), response.Status);

---

Suggestion 2:

Add null safety checks and proper dictionary access to prevent potential runtime exceptions

The IsEnabled method could throw a NullReferenceException if _loggers is null or if the logger's issuer type is not found in the dictionary. Add null checks and fallback logic.

dotnet/src/webdriver/Internal/Logging/LogContext.cs [104]

-return Handlers != null && level >= _level && level >= _loggers?[logger.Issuer].Level;
+return Handlers != null && level >= _level && (_loggers?.TryGetValue(logger.Issuer, out var loggerEntry) != true || level >= loggerEntry.Level);

---

Suggestion 3:

Add null checks during dictionary initialization to prevent potential null reference exceptions

The logger initialization in constructor could fail if any of the source loggers has a null Issuer. Add validation to handle this case.

dotnet/src/webdriver/Internal/Logging/LogContext.cs [51]

-_loggers = new ConcurrentDictionary<Type, ILogger>(loggers.Select(l => new KeyValuePair<Type, ILogger>(l.Key, new Logger(l.Value.Issuer, level))));
+_loggers = new ConcurrentDictionary<Type, ILogger>(loggers.Where(l => l.Value?.Issuer != null).Select(l => new KeyValuePair<Type, ILogger>(l.Key, new Logger(l.Value.Issuer, level))));

---

Suggestion 4:

Make PinnedScript class immutable for better thread safety and design

Consider making the PinnedScript class immutable by using read-only properties and removing the setter for ScriptId.

dotnet/src/webdriver/PinnedScript.cs [53-83]

 public string Handle { get; }
 public string Source { get; }
-internal string ScriptId { get; set; }
+internal string ScriptId { get; }

---

Pattern 2: Enhance error messages by including actual values and specific details to help with debugging. Error messages should contain the problematic values and clear explanations.

Example code before:

throw new ArgumentException("Invalid value");

Example code after:

throw new ArgumentException($"Invalid value. Provided: '{value}'");

Relevant past accepted suggestions:

---

Suggestion 1:

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

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()) +

---

Suggestion 3:

Correct the error message by removing an unnecessary character

Remove the '=' sign from the error message as it appears to be a typo and doesn't add any meaningful information.

java/src/org/openqa/selenium/manager/SeleniumManager.java [198]

-throw new WebDriverException("Linux ARM64= is not supported by Selenium Manager");
+throw new WebDriverException("Linux ARM64 is not supported by Selenium Manager");

---

Pattern 3: Use proper null handling with null-conditional and null-coalescing operators to prevent null reference exceptions. Replace direct null checks with safer null handling operators.

Example code before:

var result = obj.ToString();

Example code after:

var result = obj?.ToString() ?? string.Empty;

Relevant past accepted suggestions:

---

Suggestion 1:

Use null-coalescing operator with null-conditional operator for safer string conversion

Consider using the null-coalescing operator (??) instead of the null-conditional operator (?.) followed by ToString(). This will provide a default empty string if the value is null, avoiding potential null reference exceptions.

dotnet/src/webdriver/ErrorResponse.cs [55]

-this.message = responseValue["message"].ToString() ?? "";
+this.message = responseValue["message"]?.ToString() ?? string.Empty;

---

Suggestion 2:

Use pattern matching for more concise type checking and casting

Consider using pattern matching with 'is' keyword for type checking and casting in a single step, which can make the code more concise and readable.

dotnet/src/webdriver/StackTraceElement.cs [62-69]

-if (elementAttributes.ContainsKey("lineNumber"))
+if (elementAttributes.TryGetValue("lineNumber", out var lineNumberObj) && lineNumberObj is int lineNumber)
 {
-    int lineNumber;
-    if (int.TryParse(elementAttributes["lineNumber"].ToString(), out lineNumber))
-    {
-        this.lineNumber = lineNumber;
-    }
+    this.lineNumber = lineNumber;
 }

---

Suggestion 3:

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: Ensure consistent and accurate naming in tests by using names that reflect the actual functionality being tested. Test names should clearly indicate what is being verified.

Example code before:

def test_chrome_driver_functionality():
  # Testing Edge driver

Example code after:

def test_edge_driver_functionality():
  # Testing Edge driver

Relevant past accepted suggestions:

---

Suggestion 1:

Correct the test function name to accurately reflect the browser being tested

Rename the test function to reflect that it's testing Edge browser instances, not Chrome instances.

py/test/selenium/webdriver/edge/edge_launcher_tests.py [28]

-def test_we_can_launch_multiple_chrome_instances(clean_driver, clean_service):
+def test_we_can_launch_multiple_edge_instances(clean_driver, clean_service):

---

Suggestion 2:

Update the test function name to correctly identify the driver being tested

Replace 'chromedriver' with 'msedgedriver' in the test function name to accurately reflect the driver being tested.

py/test/selenium/webdriver/edge/edge_service_tests.py [29]

-def test_uses_chromedriver_logging(clean_driver, driver_executable) -> None:
+def test_uses_msedgedriver_logging(clean_driver, driver_executable) -> None:

---

Suggestion 3:

Correct the class name to accurately represent the service being tested

Rename the class to reflect that it's testing EdgeDriverService, not ChromeDriverService.

py/test/selenium/webdriver/edge/edge_service_tests.py [106]

-class TestChromeDriverService:
+class TestEdgeDriverService:

---

Pattern 5: Add proper validation for method parameters and state to prevent invalid operations. Methods should validate their inputs and state before proceeding with operations.

Example code before:

public void ProcessValue(string value) {
  // Direct usage without validation
}

Example code after:

public void ProcessValue(string value) {
  if (value == null) throw new ArgumentNullException(nameof(value));
  // Process validated value
}

Relevant past accepted suggestions:

---

Suggestion 1:

Add parameter validation to prevent null reference exceptions

Add null check for value parameter in FromJson method to prevent potential NullReferenceException when deserializing JSON.

dotnet/src/webdriver/Response.cs [74-76]

 public static Response FromJson(string value)
 {
+    if (string.IsNullOrEmpty(value))
+    {
+        throw new ArgumentNullException(nameof(value));
+    }
     Dictionary<string, object> rawResponse = JsonSerializer.Deserialize<Dictionary<string, object>>(value, s_jsonSerializerOptions)

---

Suggestion 2:

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))

---

Suggestion 3:

Add validation for supported authentication types when setting the token

In the token setter method, consider adding a validation check for the auth_type to ensure it's one of the supported types (Bearer or X-API-Key) before setting the token.

py/selenium/webdriver/remote/client_config.py [221-227]

 @token.setter
 def token(self, value: str) -> None:
     """Sets the token used for authentication to the remote server if
     auth_type is not basic.
 
     Support values: Bearer, X-API-Key. For others, please use `extra_headers` instead.
     """
+    if self.auth_type.lower() not in ['bearer', 'x-api-key']:
+        raise ValueError("Token can only be set for 'Bearer' or 'X-API-Key' auth types.")
     self._token = value

---

[Auto-generated best practices - 2025-01-22]