Address review comments for Color classes

srimanachanta · Dec 1, 2023 · e259f59 · e259f59
1 parent 49af1f2
commit e259f59
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 26 deletions.
diff --git a/wpilibc/src/main/native/include/frc/util/Color.h b/wpilibc/src/main/native/include/frc/util/Color.h
@@ -7,6 +7,10 @@
 #include <algorithm>
 #include <stdexcept>
 #include <string>
+#include <string_view>
+
+#include <fmt/core.h>
+#include <wpi/StringExtras.h>
 
 namespace frc {
 
@@ -813,20 +817,28 @@ class Color {
   }
 
   /**
-   * Create a Color from a hex string. Throws an exception if the Hex String is
-   * invalid.
+   * Create a Color from a hex string.
    *
    * @param hexString a string of the format <tt>\#RRGGBB</tt>
    * @return Color object from hex string.
-   */
-  static constexpr Color FromHexString(std::string hexString) {
-    if (hexString.length() != 7 || !hexString.starts_with("#")) {
-      throw std::invalid_argument("Invalid Hex String for Color");
+   * @throws std::invalid_argument if the hex string is invalid.
+   */
+  static constexpr Color FromHexString(std::string_view hexString) {
+    if (hexString.length() != 7 || !hexString.starts_with("#") ||
+        !wpi::isHexDigit(hexString[1]) || !wpi::isHexDigit(hexString[2]) ||
+        !wpi::isHexDigit(hexString[3]) || !wpi::isHexDigit(hexString[4]) ||
+        !wpi::isHexDigit(hexString[5]) || !wpi::isHexDigit(hexString[6])) {
+      throw std::invalid_argument(
+          fmt::format("Invalid hex string for Color \"{}\"", hexString));
     }
 
-    int r, g, b;
-    std::sscanf(hexString.c_str(), "#%02x%02x%02x", &r, &g, &b);
-    return Color(r / 255, g / 255, b / 255);
+    int r = wpi::hexDigitValue(hexString[0]) * 16 +
+            wpi::hexDigitValue(hexString[1]);
+    int g = wpi::hexDigitValue(hexString[2]) * 16 +
+            wpi::hexDigitValue(hexString[3]);
+    int b = wpi::hexDigitValue(hexString[4]) * 16 +
+            wpi::hexDigitValue(hexString[5]);
+    return Color{r, g, b};
   }
 
   /**

diff --git a/wpilibc/src/main/native/include/frc/util/Color8Bit.h b/wpilibc/src/main/native/include/frc/util/Color8Bit.h
@@ -7,6 +7,10 @@
 #include <algorithm>
 #include <stdexcept>
 #include <string>
+#include <string_view>
+
+#include <fmt/core.h>
+#include <wpi/StringExtras.h>
 
 #include "Color.h"
 
@@ -46,20 +50,28 @@ class Color8Bit {
   }
 
   /**
-   * Create a Color8Bit from a hex string. Throws an exception if the Hex String
-   * is invalid.
+   * Create a Color8Bit from a hex string.
    *
    * @param hexString a string of the format <tt>\#RRGGBB</tt>
    * @return Color8Bit object from hex string.
+   * @throws std::invalid_argument if the hex string is invalid.
    */
-  static constexpr Color8Bit FromHexString(std::string hexString) {
-    if (hexString.length() != 7 || !hexString.starts_with("#")) {
-      throw std::invalid_argument("Invalid Hex String for Color");
+  static constexpr Color8Bit FromHexString(std::string_view hexString) {
+    if (hexString.length() != 7 || !hexString.starts_with("#") ||
+        !wpi::isHexDigit(hexString[1]) || !wpi::isHexDigit(hexString[2]) ||
+        !wpi::isHexDigit(hexString[3]) || !wpi::isHexDigit(hexString[4]) ||
+        !wpi::isHexDigit(hexString[5]) || !wpi::isHexDigit(hexString[6])) {
+      throw std::invalid_argument(
+          fmt::format("Invalid hex string for Color \"{}\"", hexString));
     }
 
-    int r, g, b;
-    std::sscanf(hexString.c_str(), "#%02x%02x%02x", &r, &g, &b);
-    return Color8Bit(r, g, b);
+    int r = wpi::hexDigitValue(hexString[0]) * 16 +
+            wpi::hexDigitValue(hexString[1]);
+    int g = wpi::hexDigitValue(hexString[2]) * 16 +
+            wpi::hexDigitValue(hexString[3]);
+    int b = wpi::hexDigitValue(hexString[4]) * 16 +
+            wpi::hexDigitValue(hexString[5]);
+    return Color8Bit{r, g, b};
   }
 
   constexpr bool operator==(const Color8Bit&) const = default;

diff --git a/wpilibj/src/main/java/edu/wpi/first/wpilibj/util/Color.java b/wpilibj/src/main/java/edu/wpi/first/wpilibj/util/Color.java
@@ -116,19 +116,21 @@ public static Color fromHSV(int h, int s, int v) {
   }
 
   /**
-   * Create a Color from a hex string. Throws an exception if the Hex String is invalid.
+   * Create a Color from a hex string.
    *
    * @param hexString a string of the format <code>#RRGGBB</code>
    * @return Color object from hex string.
+   * @throws IllegalArgumentException if the hex string is invalid.
    */
   public static Color fromHexString(String hexString) {
-    if (hexString.length() != 7 || !hexString.startsWith("#"))
-      throw new IllegalArgumentException("Invalid Hex String");
+    if (hexString.length() != 7 || !hexString.startsWith("#")) {
+      throw new IllegalArgumentException("Invalid hex string \"" + hexString + "\"");
+    }
 
     return new Color(
-        Integer.valueOf(hexString.substring(1, 3), 16) / 255.0,
-        Integer.valueOf(hexString.substring(3, 5), 16) / 255.0,
-        Integer.valueOf(hexString.substring(5, 7), 16) / 255.0);
+        Integer.valueOf(hexString.substring(1, 3), 16),
+        Integer.valueOf(hexString.substring(3, 5), 16),
+        Integer.valueOf(hexString.substring(5, 7), 16));
   }
 
   @Override

diff --git a/wpilibj/src/main/java/edu/wpi/first/wpilibj/util/Color8Bit.java b/wpilibj/src/main/java/edu/wpi/first/wpilibj/util/Color8Bit.java
@@ -37,14 +37,16 @@ public Color8Bit(Color color) {
   }
 
   /**
-   * Create a Color8Bit from a hex string. Throws an exception if the Hex String is invalid.
+   * Create a Color8Bit from a hex string.
    *
    * @param hexString a string of the format <code>#RRGGBB</code>
    * @return Color8Bit object from hex string.
+   * @throws IllegalArgumentException if the hex string is invalid.
    */
   public static Color8Bit fromHexString(String hexString) {
-    if (hexString.length() != 7 || !hexString.startsWith("#"))
-      throw new IllegalArgumentException("Invalid Hex String");
+    if (hexString.length() != 7 || !hexString.startsWith("#")) {
+      throw new IllegalArgumentException("Invalid hex string \"" + hexString + "\"");
+    }
 
     return new Color8Bit(
         Integer.valueOf(hexString.substring(1, 3), 16),