From 527f0e04453a52fc6d20b3f7cf1f3d17b10f02cb Mon Sep 17 00:00:00 2001
From: Zebb McAteer <zebb.mcateer@smartservice.qld.gov.au>
Date: Mon, 10 Jun 2024 10:41:01 +1000
Subject: [PATCH] Add recommended code revisions

---
 .../gov/qld/online/selenium/SeleniumHelper.java  | 16 +++++++---------
 .../gov/qld/online/selenium/WebDriverHolder.java |  6 +++---
 .../qld/online/selenium/SeleniumHelperTest.java  |  4 ++--
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/src/main/java/au/gov/qld/online/selenium/SeleniumHelper.java b/src/main/java/au/gov/qld/online/selenium/SeleniumHelper.java
index 3acc2a2..e31d793 100644
--- a/src/main/java/au/gov/qld/online/selenium/SeleniumHelper.java
+++ b/src/main/java/au/gov/qld/online/selenium/SeleniumHelper.java
@@ -45,12 +45,12 @@ public final class SeleniumHelper {
     private static DriverService chromeService;
 
     //Keep list of released browsers to reuse until max usage is hit
-    private static Map<String, WebDriverHolder> webDriverListReleased = new ConcurrentHashMap<>();
+    private static final Map<String, WebDriverHolder> webDriverListReleased = new ConcurrentHashMap<>();
     //Keep internal tabs on open browsers so when we die unexpectedly we don't leave orphaned browsers running on outside of jvm connection
-    private static List<WebDriver> webDriverListAll = new LinkedList<>();
-    private static List<DriverService> driverServiceAll = new LinkedList<>();
-    private static File screenprintFolder = new File("target/screenprints/" + new SimpleDateFormat("dd-M-yyyy", Locale.getDefault()).format(new Date()) + "/");
-    private static File screenprintCurrentFolder = new File("target/screenprints/current");
+    private static final List<WebDriver> webDriverListAll = new LinkedList<>();
+    private static final List<DriverService> driverServiceAll = new LinkedList<>();
+    private static final File screenprintFolder = new File("target/screenprints/" + new SimpleDateFormat("dd-M-yyyy", Locale.getDefault()).format(new Date()) + "/");
+    private static final File screenprintCurrentFolder = new File("target/screenprints/current");
     private static boolean doScreenPrints = false;
     private static boolean headlessEnabled = true;
 
@@ -74,12 +74,10 @@ public void run() {
             }
             for (DriverService service : driverServiceAll) {
                 if (service != null) {
-                    try {
+                    try (service) {
                         service.stop();
                     } catch (Exception e) {
                         LOGGER.error("exception on close", e);
-                    } finally {
-                        service.close();
                     }
                 }
             }
@@ -138,7 +136,7 @@ public static synchronized WebDriverHolder getWebDriver(DriverTypes driverType,
             }
         }
 
-        WebDriver webDriver = null;
+        WebDriver webDriver;
         WebDriverManager wdm;
         try {
             final Platform platform = Platform.getCurrent();
diff --git a/src/main/java/au/gov/qld/online/selenium/WebDriverHolder.java b/src/main/java/au/gov/qld/online/selenium/WebDriverHolder.java
index 3656b01..1c5b030 100644
--- a/src/main/java/au/gov/qld/online/selenium/WebDriverHolder.java
+++ b/src/main/java/au/gov/qld/online/selenium/WebDriverHolder.java
@@ -4,10 +4,10 @@
 
 public class WebDriverHolder {
 
-    private WebDriver webDriver;
-    private DriverTypes driverType;
+    private final WebDriver webDriver;
+    private final DriverTypes driverType;
     private int numberUsed = 0;
-    private String downloadDirectory;
+    private final String downloadDirectory;
 
     public WebDriverHolder(WebDriver webDriver, DriverTypes driverType, String downloadDirectory) {
         this.webDriver = webDriver;
diff --git a/src/test/java/au/gov/qld/online/selenium/SeleniumHelperTest.java b/src/test/java/au/gov/qld/online/selenium/SeleniumHelperTest.java
index 82d2aa3..bbecd29 100644
--- a/src/test/java/au/gov/qld/online/selenium/SeleniumHelperTest.java
+++ b/src/test/java/au/gov/qld/online/selenium/SeleniumHelperTest.java
@@ -13,6 +13,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.time.Duration;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
@@ -62,7 +63,6 @@ public void shouldStartFirefoxBrowserMultiTest() {
         holder3.getWebDriver().navigate().to("https://www.whirlpool.net.au");
         SeleniumHelper.performScreenPrint(holder3, testName);
         SeleniumHelper.close(holder);
-        int afterStart = SeleniumHelper.openDrivers();
     }
 
     @Test
@@ -197,7 +197,7 @@ public void shouldSetDownloadDirectoryForChromeBrowser() throws IOException, Int
         try {
             Assertions.assertThat(downloadedFile).exists();
         } catch (AssertionError e) {
-            Set<String> files = Stream.of(tempDownloadDirectory.toFile().listFiles())
+            Set<String> files = Stream.of(Objects.requireNonNull(tempDownloadDirectory.toFile().listFiles()))
                     .filter(file -> !file.isDirectory())
                     .map(File::getName)
                     .collect(Collectors.toSet());