Fix duplicate toUpperCase() calls, use Locale.ROOT #158

nyurik · 2018-04-28T22:56:16Z

No description provided.

nyurik · 2018-04-29T00:17:33Z

@Supersuz what do you mean? Your comment is a bit confusing :)

nyurik · 2018-04-29T17:08:13Z

@shoeper I am not able to merge the changes in this repo. Either you can set up so that I can self-merge after approvals, or you need to merge it yourself. Thx!

P.S. I think all toUpperCase() should be changed to toUpperCase(Locale.ROOT) because some tools may complain about locale ambiguity (even though in this case it shouldn't matter, ROOT makes it clear)

While technically this might not be needed in this specific case, it is a good practice to be explicit about the locale.

nyurik · 2018-05-05T03:50:26Z

@Supersuz or @shoeper - are there any other people that need to review this code, or can you merge it? Thx!

shoeper · 2018-05-05T09:13:27Z

Sorry, I don't really know.

I cannot merge it.

nyurik · 2018-05-05T14:12:28Z

Looking at commit history, @drinckes and @JonMcPherson merged things recently.

JonMcPherson · 2018-05-05T23:32:04Z

java/com/google/openlocationcode/OpenLocationCode.java

@@ -496,7 +498,7 @@ public static boolean isValidCode(String code) {
    if (code == null || code.length() < 2) {
      return false;
    }
-    code = code.toUpperCase();
+    code = code.toUpperCase(Locale.ROOT);


If you wanted to remove all redundant calls of toUpperCase(), it could be modified to something like this

public static boolean isValidCode(String code) { return isValidCodeUpperCase(code.toUpperCase(Locale.ROOT)); } private static boolean isValidCodeUpperCase(String code) { // do validity checks without calling code.toUpperCase() redundantly }

And the constructor would instead call !isValidCodeUpperCase(newCode).
Just a minor improvement...

Thanks for pointing out the redundant call. It's so redundant, in fact, that I think we can just remove the one in the constructor since the code will be cast to uppercase in isValidCode().

JonMcPherson · 2018-05-05T23:35:50Z

Also, @drinckes is who you want for merging

shoeper · 2018-05-29T08:12:23Z

Maybe @zongweil can help.

zongweil · 2018-05-29T17:22:30Z

I'll take a look within the next couple days.

zongweil

Thanks for the PR! Please pull the latest changes and address these comments.

zongweil · 2018-05-31T17:49:32Z

java/com/google/openlocationcode/OpenLocationCode.java

@@ -153,11 +154,12 @@ public double getEastLongitude() {
   * @constructor
  */
  public OpenLocationCode(String code) throws IllegalArgumentException {
-    if (!isValidCode(code.toUpperCase())) {


We can get rid of the code.toUpperCase() here all together, since we're calling it inside of isValidCode().

since this is public, can we make that assumption and document it the constructor comments?

I don't understand your question here, can you rephrase? What assumption are you thinking we need to document?

zongweil · 2018-05-31T17:49:36Z

java/com/google/openlocationcode/OpenLocationCode.java

@@ -496,7 +498,7 @@ public static boolean isValidCode(String code) {
    if (code == null || code.length() < 2) {
      return false;
    }
-    code = code.toUpperCase();
+    code = code.toUpperCase(Locale.ROOT);


Thanks for pointing out the redundant call. It's so redundant, in fact, that I think we can just remove the one in the constructor since the code will be cast to uppercase in isValidCode().

java/com/google/openlocationcode/OpenLocationCode.java

This updates your version of OpenLocationCode.java to the most recent upstream version while keeping your changes.

Update to recent version while keeping changes

# Conflicts: # java/src/main/java/com/google/openlocationcode/OpenLocationCode.java

nyurik · 2021-05-21T20:03:04Z

@zongweil I merged the code and applied your suggestion. See 92e2e40

nyurik · 2021-05-21T20:09:54Z

I have done another pass at minor optimizations -- the code now has a toValidCode which returns normalized code or null if it fails. This way constructor can use it directly, without doing duplicate toUpperCase.

java/src/main/java/com/google/openlocationcode/OpenLocationCode.java

…e.java Co-authored-by: Sonya Alexandrova <sonyaa@users.noreply.github.com>

phebehp · 2022-06-17T21:02:19Z

java/src/main/java/com/google/openlocationcode/OpenLocationCode.java

+   * @param code The code to check and normalize.
+   * @return Normalized code if it is a valid full or short code, null otherwise.
+   */
+  private static String toValidCode(String code) {


I think returning an Optional instead of using null to signify invalid would be slightly cleaner here, what do you think?

phebehp · 2022-06-17T21:04:12Z

java/com/google/openlocationcode/OpenLocationCode.java

@@ -153,11 +154,12 @@ public double getEastLongitude() {
   * @constructor
  */
  public OpenLocationCode(String code) throws IllegalArgumentException {
-    if (!isValidCode(code.toUpperCase())) {


I don't understand your question here, can you rephrase? What assumption are you thinking we need to document?

Remove duplicate toUpperCase() calls

85053db

shoeper approved these changes Apr 29, 2018

View reviewed changes

Use Locale.ROOT with toUpper to avoid ambiguities

803cdd5

While technically this might not be needed in this specific case, it is a good practice to be explicit about the locale.

nyurik changed the title ~~Remove duplicate toUpperCase() calls~~ Fix duplicate toUpperCase() calls, use Locale.ROOT May 5, 2018

JonMcPherson reviewed May 5, 2018

View reviewed changes

kjhgg approved these changes May 29, 2018

View reviewed changes

zongweil requested changes May 31, 2018

View reviewed changes

google deleted a comment from Sassysuz Apr 21, 2019

Update to recent version while keeping changes

1e8d0c3

This updates your version of OpenLocationCode.java to the most recent upstream version while keeping your changes.

bocops mentioned this pull request Apr 22, 2021

Update to recent version while keeping changes nyurik/open-location-code#1

Merged

Merge pull request #1 from bocops/patch-3

9c8fc6e

Update to recent version while keeping changes

google-cla bot added the cla: yes label May 21, 2021

nyurik added 2 commits May 21, 2021 15:57

Merge branch 'main' into patch-2

15e7afa

# Conflicts: # java/src/main/java/com/google/openlocationcode/OpenLocationCode.java

simplified per suggestion

92e2e40

Further optimization to remove dup toUpperCase

9101f25

nyurik requested a review from zongweil June 1, 2021 12:49

Merge branch 'main' into patch-2

f0f5dc6

sonyaa reviewed Nov 8, 2021

View reviewed changes

java/src/main/java/com/google/openlocationcode/OpenLocationCode.java Outdated Show resolved Hide resolved

nyurik and others added 2 commits November 18, 2021 16:25

Update java/src/main/java/com/google/openlocationcode/OpenLocationCod…

efe3cde

…e.java Co-authored-by: Sonya Alexandrova <sonyaa@users.noreply.github.com>

Merge branch 'main' into patch-2

868ed71

Merge branch 'main' into patch-2

731fdcd

phebehp reviewed Jun 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix duplicate toUpperCase() calls, use Locale.ROOT #158

Fix duplicate toUpperCase() calls, use Locale.ROOT #158

nyurik commented Apr 28, 2018

nyurik commented Apr 29, 2018

nyurik commented Apr 29, 2018

nyurik commented May 5, 2018

shoeper commented May 5, 2018

nyurik commented May 5, 2018

JonMcPherson May 5, 2018 •

edited

zongweil May 31, 2018

JonMcPherson commented May 5, 2018

shoeper commented May 29, 2018

zongweil commented May 29, 2018

zongweil left a comment

zongweil May 31, 2018

IDisposable Dec 11, 2018

phebehp Jun 17, 2022

zongweil May 31, 2018

nyurik commented May 21, 2021 •

edited

nyurik commented May 21, 2021

phebehp Jun 17, 2022

phebehp Jun 17, 2022

Fix duplicate toUpperCase() calls, use Locale.ROOT #158

Are you sure you want to change the base?

Fix duplicate toUpperCase() calls, use Locale.ROOT #158

Conversation

nyurik commented Apr 28, 2018

nyurik commented Apr 29, 2018

nyurik commented Apr 29, 2018

nyurik commented May 5, 2018

shoeper commented May 5, 2018

nyurik commented May 5, 2018

JonMcPherson May 5, 2018 • edited

Choose a reason for hiding this comment

zongweil May 31, 2018

Choose a reason for hiding this comment

JonMcPherson commented May 5, 2018

shoeper commented May 29, 2018

zongweil commented May 29, 2018

zongweil left a comment

Choose a reason for hiding this comment

zongweil May 31, 2018

Choose a reason for hiding this comment

IDisposable Dec 11, 2018

Choose a reason for hiding this comment

phebehp Jun 17, 2022

Choose a reason for hiding this comment

zongweil May 31, 2018

Choose a reason for hiding this comment

nyurik commented May 21, 2021 • edited

nyurik commented May 21, 2021

phebehp Jun 17, 2022

Choose a reason for hiding this comment

phebehp Jun 17, 2022

Choose a reason for hiding this comment

JonMcPherson May 5, 2018 •

edited

nyurik commented May 21, 2021 •

edited