Feature/code refactoring #81

YauheniPo · 2019-07-27T15:44:49Z

No description provided.

Update fork branch

php-coder · 2019-07-27T21:55:04Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Cookie.java

 				.getCurrentWebDriver().manage().getCookies());
 		for (int i = 0; i < cookies.size(); i++) {
-			ret.append(cookies.get(i).getName() + "=" + cookies.get(i).getValue());
+			ret.append(cookies.get(i).getName()).append("=").append(cookies.get(i).getValue());


How about

-.append("=") +.append('=')

? :)

php-coder · 2019-07-27T21:57:14Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/JavaScript.java

@@ -165,8 +165,7 @@ public Object executeAsyncJavascript(String... code) {
 	public String getAlertMessage() {
 		try {
 			Alert alert = browserManagement.getCurrentWebDriver().switchTo().alert();
-			String text = alert.getText().replace("\n", "");
-			return text;
+			return alert.getText().replace("\n", "");


Can we use .replace('\n', '') instead?

it is not working. only alert.getText().replace("\n", "");

Yes, I see it now. Thanks for correcting me!

php-coder · 2019-07-27T22:01:20Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/SelectElement.java

@@ -222,7 +222,6 @@ public void selectFromList(String locator, String... items) {
 				} catch (NoSuchElementException e2) {
 					nonExistingItems.add(item);
 					lastItemFound = false;
-					continue;


I'd rather keep it. IMHO.

php-coder · 2019-07-27T22:04:11Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/SelectElement.java

-		for (String index : indexes) {
-			tmp.add(index);
-		}
+		List<String> tmp = new ArrayList<>(Arrays.asList(indexes));


How about using Collections.addAll() instead?

php-coder · 2019-07-27T22:06:14Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Waiting.java

-			}
-		});
+		waitUntil(timeout, message, () -> Boolean.TRUE.equals(((JavascriptExecutor) browserManagement.getCurrentWebDriver())
+				.executeScript(condition)));


Could you, please, put waitUntil() argument to the separate lines to make code a little bit easier to read?

php-coder · 2019-07-27T22:10:15Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/keywords/Waiting.java

 			}
 		}
 	}

-	protected static interface WaitUntilFunction {
+	protected interface WaitUntilFunction {


This method look like a candidate for having FunctionalInterface annotation.

php-coder · 2019-07-27T22:11:24Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/locators/ElementFinder.java

-			constraints.put("type", "file");
-		}
+		Map<String, String> constraints = new TreeMap<>();
+        switch (tag) {


There is something wrong with indentation.

php-coder · 2019-07-27T22:17:27Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/locators/TableElementFinder.java

-		for (String value : values) {
-			list.add(value);
-		}
+		List<String> list = new ArrayList<>(Arrays.asList(values));


Collections.addAll()?

Could you explain to me why this is better?

Arrays.asList() creates a new ArrayList under the hood. After that, we pass it to a ArrayList() constructor where we create an array (see toArray() there), and after that we copy this array to a destination array. So, here we've copied elements 2 times and produced unnecessary temporary objects (more work to GC). We can avoid this by using Collections.addAll(). That was my point.

ok, thanks a lot!

php-coder · 2019-07-27T22:18:11Z

src/main/java/com/github/markusbernhardt/seleniumlibrary/locators/WindowManager.java

-
-				}, "Unable to locate window with title '" + selectCoordinates.criteria + "'");
+				selectMatching(webDriver, currentWindowInfo -> currentWindowInfo.get(WINDOW_INFO_INDEX_DOCUMENT_TITLE).trim().toLowerCase()
+						.equals(selectCoordinates.criteria.toLowerCase()), "Unable to locate window with title '" + selectCoordinates.criteria + "'");


Could you split the arguments and make it more readable, please?

And for the text 2 methods also...

Hi-Fi · 2019-07-29T18:00:17Z

Thank you, refactoring has been long time on the list (and I think there's still plenty of it left).

YauheniPo added 3 commits July 27, 2019 17:31

Merge pull request #1 from Hi-Fi/develop

891dbe5

Update fork branch

Code refactoring

cf75328

Code refactoring

c7961e0

php-coder reviewed Jul 27, 2019

View reviewed changes

Corrected by review comments

9b83f26

Hi-Fi merged commit 6b85626 into MarketSquare:develop Jul 29, 2019

Feature/code refactoring #81

Feature/code refactoring #81

Uh oh!

Conversation

YauheniPo commented Jul 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hi-Fi commented Jul 29, 2019

Uh oh!

Uh oh!