Implement size unit formatter and converter 84#115
Conversation
…verting between units, parsing size strings, and formatting network rates
WalkthroughThe changes introduce a utility class ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Formatter as SizeFormatter
participant Unit as SizeUnit
Client->>Formatter: autoFormat(bytes, useBinaryBase, decimalPlaces)
Formatter->>Unit: Retrieve unit list (binary/decimal)
Unit-->>Formatter: Return unit list
Formatter->>Formatter: Compute formatted string
Formatter-->>Client: Return human-readable size
Client->>Formatter: convert(value, fromUnit, toUnit)
Formatter->>Unit: Convert quantity to bytes via fromBytes()
Unit-->>Formatter: Return byte value
Formatter->>Unit: Convert bytes to target unit via fromBytes()
Unit-->>Formatter: Return converted value
Formatter-->>Client: Return converted result
Client->>Formatter: parse(sizeString)
Formatter->>Formatter: Validate and parse string
Formatter-->>Client: Return byte value or throw error
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/org/fungover/breeze/util/size/SizeUnit.java (1)
51-61: Consider adding validation for negative quantities.The conversion methods handle precision well using BigDecimal, but they should validate input parameters to prevent negative quantities.
public BigDecimal toBytes(BigDecimal quantity) { + if (quantity.compareTo(BigDecimal.ZERO) < 0) { + throw new IllegalArgumentException("Quantity cannot be negative"); + } BigDecimal baseFactor = BigDecimal.valueOf(base).pow(exponent); BigDecimal value = quantity.multiply(baseFactor); return isBit ? value.divide(BigDecimal.valueOf(8), MathContext.DECIMAL128) : value; } public BigDecimal fromBytes(BigDecimal bytes) { + if (bytes.compareTo(BigDecimal.ZERO) < 0) { + throw new IllegalArgumentException("Bytes cannot be negative"); + } BigDecimal baseFactor = BigDecimal.valueOf(base).pow(exponent); BigDecimal value = isBit ? bytes.multiply(BigDecimal.valueOf(8)) : bytes; return value.divide(baseFactor, MathContext.DECIMAL128); }src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (3)
15-26: Consider caching the unit lists.The
getBinaryByteUnits()andgetDecimalByteUnits()are called frequently but return new lists each time. Consider caching these lists as static final fields.public class SizeFormatter { + private static final List<SizeUnit> BINARY_BYTE_UNITS = Arrays.asList( + SizeUnit.TEBIBYTES, + SizeUnit.GIBIBYTES, + SizeUnit.MEBIBYTES, + SizeUnit.KIBIBYTES, + SizeUnit.BYTES + ); + + private static final List<SizeUnit> DECIMAL_BYTE_UNITS = Arrays.asList( + SizeUnit.TERABYTES, + SizeUnit.GIGABYTES, + SizeUnit.MEGABYTES, + SizeUnit.KILOBYTES, + SizeUnit.BYTES + ); public static String autoFormat(long bytes, boolean useBinaryBase, int decimalPlaces) { - List<SizeUnit> units = useBinaryBase ? getBinaryByteUnits() : getDecimalByteUnits(); + List<SizeUnit> units = useBinaryBase ? BINARY_BYTE_UNITS : DECIMAL_BYTE_UNITS;
42-45: Consider using a constant for the regex pattern.The regex pattern should be compiled once and reused for better performance.
public class SizeFormatter { + private static final Pattern SIZE_PATTERN = Pattern.compile("([\\d.]+)\\s*([a-zA-Z]+)"); public static long parse(String sizeString) { - Matcher matcher = Pattern.compile("([\\d.]+)\\s*([a-zA-Z]+)").matcher(sizeString.trim()); + Matcher matcher = SIZE_PATTERN.matcher(sizeString.trim());
62-69: Consider caching the rate units list.Similar to the byte units, the rate units list should be cached as a static final field.
public class SizeFormatter { + private static final List<SizeUnit> RATE_UNITS = Arrays.asList( + SizeUnit.TERABITS, + SizeUnit.GIGABITS, + SizeUnit.MEGABITS, + SizeUnit.KILOBITS, + SizeUnit.BITS + ); public static String formatRate(long bits, TimeUnit timeUnit, int decimalPlaces) { BigDecimal seconds = BigDecimal.valueOf(timeUnit.toSeconds(1)); BigDecimal bitsPerSecond = BigDecimal.valueOf(bits).divide(seconds, MathContext.DECIMAL128); - List<SizeUnit> rateUnits = Arrays.asList( - SizeUnit.TERABITS, - SizeUnit.GIGABITS, - SizeUnit.MEGABITS, - SizeUnit.KILOBITS, - SizeUnit.BITS - ); - - for (SizeUnit unit : rateUnits) { + for (SizeUnit unit : RATE_UNITS) {src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (1)
22-27: Add more test cases for parse method.The parse method should be tested with more edge cases:
- Empty string
- Null input
- Invalid number format
- Invalid unit format
@Test void parseValidFormats() { assertEquals(1_500_000_000L, SizeFormatter.parse("1.5GB")); assertEquals(1024, SizeFormatter.parse("1KiB")); assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("invalid")); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("")); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse(null)); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("1.2.3GB")); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("1.5XB")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(1 hunks)src/main/java/org/fungover/breeze/util/size/SizeUnit.java(1 hunks)src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java(1 hunks)
🔇 Additional comments (1)
src/main/java/org/fungover/breeze/util/size/SizeUnit.java (1)
6-37: LGTM! Well-structured enum with clear categorization of units.The enum is well-organized with clear separation between decimal and binary units. The constructor parameters are appropriately chosen for handling different unit types.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (5)
9-14: Add test cases for larger units and zero bytes.Consider adding test cases for:
- Larger units (GB, TB)
- Zero bytes
@Test void autoFormatUsesCorrectUnits() { assertEquals("1.46 KiB", SizeFormatter.autoFormat(1500, true, 2)); assertEquals("1.50 KB", SizeFormatter.autoFormat(1500, false, 2)); assertEquals("2.00 MB", SizeFormatter.autoFormat(2_000_000, false, 2)); + assertEquals("1.50 GB", SizeFormatter.autoFormat(1_500_000_000, false, 2)); + assertEquals("1.00 TB", SizeFormatter.autoFormat(1_000_000_000_000L, false, 2)); + assertEquals("0 B", SizeFormatter.autoFormat(0, false, 2)); }
16-20: Add test cases for decimal and binary unit conversions.Consider adding test cases for:
- Conversion between decimal units (e.g., KB to MB)
- Conversion between binary units (e.g., MiB to GiB)
@Test void convertBetweenUnits() { assertEquals(1.5, SizeFormatter.convert(1_500_000_000L, SizeUnit.BYTES, SizeUnit.GIGABYTES), 0.001); assertEquals(1024, SizeFormatter.convert(1, SizeUnit.KIBIBYTES, SizeUnit.BYTES), 0.001); + assertEquals(1.5, SizeFormatter.convert(1500, SizeUnit.KILOBYTES, SizeUnit.MEGABYTES), 0.001); + assertEquals(2.0, SizeFormatter.convert(2048, SizeUnit.MEBIBYTES, SizeUnit.GIBIBYTES), 0.001); }
22-27: Add test cases for whitespace and case sensitivity.Consider adding test cases for:
- Parsing with whitespace (e.g., "1.5 GB")
- Case sensitivity (e.g., "1.5gb", "1.5GB", "1.5Gb")
@Test void parseValidFormats() { assertEquals(1_500_000_000L, SizeFormatter.parse("1.5GB")); assertEquals(1024, SizeFormatter.parse("1KiB")); assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("invalid")); + assertEquals(1_500_000_000L, SizeFormatter.parse("1.5 GB")); + assertEquals(1_500_000_000L, SizeFormatter.parse("1.5gb")); + assertEquals(1_500_000_000L, SizeFormatter.parse("1.5Gb")); }
29-34: Add test cases for different time units.Consider adding test cases for different time units:
- Minutes (e.g., bits per minute)
- Hours (e.g., bits per hour)
@Test void formatNetworkRates() { assertEquals("1.00 Mbps", SizeFormatter.formatRate(1_000_000, TimeUnit.SECONDS, 2)); assertEquals("8.00 Kbps", SizeFormatter.formatRate(8_000, TimeUnit.SECONDS, 2)); assertEquals("1.50 Gbps", SizeFormatter.formatRate(1_500_000_000, TimeUnit.SECONDS, 2)); + assertEquals("60.00 Mbps", SizeFormatter.formatRate(1_000_000, TimeUnit.MINUTES, 2)); + assertEquals("3600.00 Mbps", SizeFormatter.formatRate(1_000_000, TimeUnit.HOURS, 2)); }
47-51: Add test cases for different decimal places.Consider adding test cases for different decimal places:
- 0 decimal places
- 1 decimal place
- 2 decimal places
@Test void testNetworkRatePrecision() { assertEquals("1.500 Gbps", SizeFormatter.formatRate(1_500_000_000, TimeUnit.SECONDS, 3)); + assertEquals("2 Gbps", + SizeFormatter.formatRate(1_500_000_000, TimeUnit.SECONDS, 0)); + assertEquals("1.5 Gbps", + SizeFormatter.formatRate(1_500_000_000, TimeUnit.SECONDS, 1)); + assertEquals("1.50 Gbps", + SizeFormatter.formatRate(1_500_000_000, TimeUnit.SECONDS, 2)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(1 hunks)src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java(1 hunks)
🔇 Additional comments (2)
src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (1)
36-45: Remove duplicate test case.The
handleEdgeCasesmethod duplicates the tests fromtestEdgeCases. Consider removing the duplicate or adding different test cases.src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (1)
28-30: LGTM!The convenience method provides sensible defaults.
| public static double convert(long value, SizeUnit fromUnit, SizeUnit toUnit) { | ||
| BigDecimal bytes = fromUnit.toBytes(BigDecimal.valueOf(value)); | ||
| return toUnit.fromBytes(bytes).setScale(4, RoundingMode.HALF_UP).doubleValue(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks for unit parameters.
The method should validate that fromUnit and toUnit are not null to prevent NullPointerException.
public static double convert(long value, SizeUnit fromUnit, SizeUnit toUnit) {
+ if (fromUnit == null || toUnit == null) {
+ throw new IllegalArgumentException("Units cannot be null");
+ }
BigDecimal bytes = fromUnit.toBytes(BigDecimal.valueOf(value));
return toUnit.fromBytes(bytes).setScale(4, RoundingMode.HALF_UP).doubleValue();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static double convert(long value, SizeUnit fromUnit, SizeUnit toUnit) { | |
| BigDecimal bytes = fromUnit.toBytes(BigDecimal.valueOf(value)); | |
| return toUnit.fromBytes(bytes).setScale(4, RoundingMode.HALF_UP).doubleValue(); | |
| } | |
| public static double convert(long value, SizeUnit fromUnit, SizeUnit toUnit) { | |
| if (fromUnit == null || toUnit == null) { | |
| throw new IllegalArgumentException("Units cannot be null"); | |
| } | |
| BigDecimal bytes = fromUnit.toBytes(BigDecimal.valueOf(value)); | |
| return toUnit.fromBytes(bytes).setScale(4, RoundingMode.HALF_UP).doubleValue(); | |
| } |
| public static String autoFormat(long bytes, boolean useBinaryBase, int decimalPlaces) { | ||
| List<SizeUnit> units = useBinaryBase ? getBinaryByteUnits() : getDecimalByteUnits(); | ||
| BigDecimal size = BigDecimal.valueOf(bytes); | ||
|
|
||
| for (SizeUnit unit : units) { | ||
| BigDecimal converted = unit.fromBytes(size); | ||
| if (converted.abs().compareTo(BigDecimal.ONE) >= 0) { | ||
| return format(converted, unit, decimalPlaces); | ||
| } | ||
| } | ||
| return format(size, SizeUnit.BYTES, decimalPlaces); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for decimal places parameter.
The method should validate that decimalPlaces is non-negative to prevent IllegalArgumentException from BigDecimal.setScale.
public static String autoFormat(long bytes, boolean useBinaryBase, int decimalPlaces) {
+ if (decimalPlaces < 0) {
+ throw new IllegalArgumentException("Decimal places must be non-negative");
+ }
List<SizeUnit> units = useBinaryBase ? getBinaryByteUnits() : getDecimalByteUnits();
BigDecimal size = BigDecimal.valueOf(bytes);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static String autoFormat(long bytes, boolean useBinaryBase, int decimalPlaces) { | |
| List<SizeUnit> units = useBinaryBase ? getBinaryByteUnits() : getDecimalByteUnits(); | |
| BigDecimal size = BigDecimal.valueOf(bytes); | |
| for (SizeUnit unit : units) { | |
| BigDecimal converted = unit.fromBytes(size); | |
| if (converted.abs().compareTo(BigDecimal.ONE) >= 0) { | |
| return format(converted, unit, decimalPlaces); | |
| } | |
| } | |
| return format(size, SizeUnit.BYTES, decimalPlaces); | |
| } | |
| public static String autoFormat(long bytes, boolean useBinaryBase, int decimalPlaces) { | |
| if (decimalPlaces < 0) { | |
| throw new IllegalArgumentException("Decimal places must be non-negative"); | |
| } | |
| List<SizeUnit> units = useBinaryBase ? getBinaryByteUnits() : getDecimalByteUnits(); | |
| BigDecimal size = BigDecimal.valueOf(bytes); | |
| for (SizeUnit unit : units) { | |
| BigDecimal converted = unit.fromBytes(size); | |
| if (converted.abs().compareTo(BigDecimal.ONE) >= 0) { | |
| return format(converted, unit, decimalPlaces); | |
| } | |
| } | |
| return format(size, SizeUnit.BYTES, decimalPlaces); | |
| } |
| public static String formatRate(long bits, TimeUnit timeUnit, int decimalPlaces) { | ||
| BigDecimal seconds = BigDecimal.valueOf(timeUnit.toSeconds(1)); | ||
| BigDecimal bitsPerSecond = BigDecimal.valueOf(bits).divide(seconds, MathContext.DECIMAL128); | ||
|
|
||
| List<SizeUnit> rateUnits = Arrays.asList( | ||
| SizeUnit.TERABITS, | ||
| SizeUnit.GIGABITS, | ||
| SizeUnit.MEGABITS, | ||
| SizeUnit.KILOBITS, | ||
| SizeUnit.BITS | ||
| ); | ||
|
|
||
| for (SizeUnit unit : rateUnits) { | ||
| BigDecimal divisor = BigDecimal.valueOf(unit.getBase()).pow(unit.getExponent()); | ||
| BigDecimal converted = bitsPerSecond.divide(divisor, MathContext.DECIMAL128); | ||
| if (converted.compareTo(BigDecimal.ONE) >= 0) { | ||
| return String.format(Locale.US, | ||
| "%." + decimalPlaces + "f %sps", | ||
| converted.setScale(decimalPlaces, RoundingMode.HALF_UP), | ||
| unit.getSuffix()); | ||
| } | ||
| } | ||
| return String.format(Locale.US, | ||
| "%." + decimalPlaces + "f bps", | ||
| bitsPerSecond.setScale(decimalPlaces, RoundingMode.HALF_UP)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for time unit and decimal places.
The method should validate:
timeUnitis not null to preventNullPointerExceptiondecimalPlacesis non-negative to preventIllegalArgumentException
public static String formatRate(long bits, TimeUnit timeUnit, int decimalPlaces) {
+ if (timeUnit == null) {
+ throw new IllegalArgumentException("Time unit cannot be null");
+ }
+ if (decimalPlaces < 0) {
+ throw new IllegalArgumentException("Decimal places must be non-negative");
+ }
BigDecimal seconds = BigDecimal.valueOf(timeUnit.toSeconds(1));
BigDecimal bitsPerSecond = BigDecimal.valueOf(bits).divide(seconds, MathContext.DECIMAL128);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static String formatRate(long bits, TimeUnit timeUnit, int decimalPlaces) { | |
| BigDecimal seconds = BigDecimal.valueOf(timeUnit.toSeconds(1)); | |
| BigDecimal bitsPerSecond = BigDecimal.valueOf(bits).divide(seconds, MathContext.DECIMAL128); | |
| List<SizeUnit> rateUnits = Arrays.asList( | |
| SizeUnit.TERABITS, | |
| SizeUnit.GIGABITS, | |
| SizeUnit.MEGABITS, | |
| SizeUnit.KILOBITS, | |
| SizeUnit.BITS | |
| ); | |
| for (SizeUnit unit : rateUnits) { | |
| BigDecimal divisor = BigDecimal.valueOf(unit.getBase()).pow(unit.getExponent()); | |
| BigDecimal converted = bitsPerSecond.divide(divisor, MathContext.DECIMAL128); | |
| if (converted.compareTo(BigDecimal.ONE) >= 0) { | |
| return String.format(Locale.US, | |
| "%." + decimalPlaces + "f %sps", | |
| converted.setScale(decimalPlaces, RoundingMode.HALF_UP), | |
| unit.getSuffix()); | |
| } | |
| } | |
| return String.format(Locale.US, | |
| "%." + decimalPlaces + "f bps", | |
| bitsPerSecond.setScale(decimalPlaces, RoundingMode.HALF_UP)); | |
| } | |
| public static String formatRate(long bits, TimeUnit timeUnit, int decimalPlaces) { | |
| if (timeUnit == null) { | |
| throw new IllegalArgumentException("Time unit cannot be null"); | |
| } | |
| if (decimalPlaces < 0) { | |
| throw new IllegalArgumentException("Decimal places must be non-negative"); | |
| } | |
| BigDecimal seconds = BigDecimal.valueOf(timeUnit.toSeconds(1)); | |
| BigDecimal bitsPerSecond = BigDecimal.valueOf(bits).divide(seconds, MathContext.DECIMAL128); | |
| List<SizeUnit> rateUnits = Arrays.asList( | |
| SizeUnit.TERABITS, | |
| SizeUnit.GIGABITS, | |
| SizeUnit.MEGABITS, | |
| SizeUnit.KILOBITS, | |
| SizeUnit.BITS | |
| ); | |
| for (SizeUnit unit : rateUnits) { | |
| BigDecimal divisor = BigDecimal.valueOf(unit.getBase()).pow(unit.getExponent()); | |
| BigDecimal converted = bitsPerSecond.divide(divisor, MathContext.DECIMAL128); | |
| if (converted.compareTo(BigDecimal.ONE) >= 0) { | |
| return String.format(Locale.US, | |
| "%." + decimalPlaces + "f %sps", | |
| converted.setScale(decimalPlaces, RoundingMode.HALF_UP), | |
| unit.getSuffix()); | |
| } | |
| } | |
| return String.format(Locale.US, | |
| "%." + decimalPlaces + "f bps", | |
| bitsPerSecond.setScale(decimalPlaces, RoundingMode.HALF_UP)); | |
| } |
| BigDecimal bytes = unit.toBytes(number); | ||
|
|
||
| if (bytes.compareTo(BigDecimal.valueOf(Long.MAX_VALUE)) > 0) { | ||
| throw new ArithmeticException("Size exceeds maximum Long value"); |
There was a problem hiding this comment.
I recommend to provide more context in the message to help users understand the issue better.
for example:
throw new ArithmeticException("Parsed size exceeds maximum Long value: " + bytes + ". Ensure the size is within the acceptable range.");
| return unit; | ||
| } | ||
| } | ||
| throw new IllegalArgumentException("Unrecognized unit: " + unitPart); |
There was a problem hiding this comment.
same here, to be more clear for the user, would be good to add extra information for the user.
For example:
throw new IllegalArgumentException("Unrecognized unit: '" + unitPart + "'. Supported units are: B, KB, MB, GB, TB, KiB, MiB, GiB, TiB.");
Stelle83
left a comment
Stelle83
left a comment
There was a problem hiding this comment.
Generally looks good!
Some comments:
*Code Comments: Add more detailed comments explaining the logic, especially within the autoFormat and parse methods.
*Unit Tests: Confirm comprehensive test coverage for all methods, including edge cases for parsing and formatting.
There was a problem hiding this comment.
Generally looks good!
Some comments:
*Code Comments: Add more detailed comments explaining the logic, especially within the autoFormat and parse methods.
*Unit Tests: Confirm comprehensive test coverage for all methods, including edge cases for parsing and formatting.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (3)
38-42: Add more test cases for invalid formats.The
parseValidFormatstest method could be more comprehensive. Consider adding test cases for:
- Empty strings
- Strings with invalid characters
- Strings with multiple decimal points
- Strings with spaces between number and unit
@Test void parseValidFormats() { assertEquals(1_500_000_000L, SizeFormatter.parse("1.5GB")); assertEquals(1024, SizeFormatter.parse("1KiB")); assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("invalid")); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("")); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("1.2.3GB")); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("1.5 GB")); + assertThrows(IllegalArgumentException.class, () -> SizeFormatter.parse("abc123KB")); }
48-52: Add test cases for more bit rate units.The
formatNetworkRatestest method should include test cases for all bit rate units, including Tbps.@Test void formatNetworkRates() { assertEquals("1.00 Mbps", SizeFormatter.formatRate(1_000_000, TimeUnit.SECONDS, 2)); assertEquals("8.00 Kbps", SizeFormatter.formatRate(8_000, TimeUnit.SECONDS, 2)); assertEquals("1.50 Gbps", SizeFormatter.formatRate(1_500_000_000, TimeUnit.SECONDS, 2)); + assertEquals("1.00 Tbps", SizeFormatter.formatRate(1_000_000_000_000L, TimeUnit.SECONDS, 2)); + assertEquals("1.00 bps", SizeFormatter.formatRate(1, TimeUnit.SECONDS, 2)); }
57-66: Add test cases for more edge cases.The
handleEdgeCasestest method should include additional edge cases:
- Zero values
- Values close to Long.MAX_VALUE/MIN_VALUE
- Values with different time units
@Test void handleEdgeCases() { assertThrows(ArithmeticException.class, () -> SizeFormatter.parse("9223372036854775808B")); // Exceeds Long.MAX_VALUE assertEquals("-1.46 KiB", SizeFormatter.autoFormat(-1500, true, 2)); assertDoesNotThrow(() -> SizeFormatter.parse("9223372036854775807B")); // Valid large value + + assertEquals("0 B", SizeFormatter.autoFormat(0, true, 2)); + assertEquals("8.00 Gbps", SizeFormatter.formatRate(8_000, TimeUnit.MILLISECONDS, 2)); + assertThrows(ArithmeticException.class, () -> + SizeFormatter.parse("-9223372036854775809B")); // Exceeds Long.MIN_VALUE }src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (1)
21-24: Consider making the regex pattern more restrictive.The current pattern allows for some edge cases that might be undesirable:
- Leading zeros in the decimal part
- Very long decimal parts
private static final Pattern SIZE_PATTERN = Pattern.compile( - "^([-+]?(?:\\d{1,19}(?:\\.\\d{1,6})?|\\.\\d{1,6}))(\\s*)([a-zA-Z]+)$", + "^([-+]?(?:\\d{1,19}(?:\\.\\d{1,3})?|\\.\\d{1,3}))(\\s*)([a-zA-Z]+)$", Pattern.CASE_INSENSITIVE );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(1 hunks)src/main/java/org/fungover/breeze/util/size/SizeUnit.java(1 hunks)src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java(1 hunks)
🔇 Additional comments (2)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (2)
110-111: Improve error messages for arithmetic exceptions.Following previous feedback, enhance the error messages to provide more context.
- throw new ArithmeticException("Size exceeds maximum Long value"); + throw new ArithmeticException(String.format("Parsed size exceeds maximum Long value: %s bytes. Ensure the size is within the acceptable range.", bytes)); - throw new ArithmeticException("Size exceeds minimum Long value"); + throw new ArithmeticException(String.format("Parsed size exceeds minimum Long value: %s bytes. Ensure the size is within the acceptable range.", bytes));Also applies to: 113-114
214-215: Enhance error message for unrecognized units.Following previous feedback, improve the error message to list supported units.
- throw new IllegalArgumentException(String.format("Unrecognized unit: %s", unitPart)); + throw new IllegalArgumentException(String.format("Unrecognized unit: '%s'. Supported units are: B, KB, MB, GB, TB, KiB, MiB, GiB, TiB.", unitPart));
| public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) { | ||
| if (useBinaryUnits) { | ||
| if (size < 1024) return BYTES; | ||
| if (size < 1024 * 1024) return KIBIBYTES; | ||
| if (size < 1024L * 1024 * 1024) return MEBIBYTES; | ||
| return GIBIBYTES; | ||
| } else { | ||
| if (size < 1_000) return BYTES; | ||
| if (size < 1_000_000) return KILOBYTES; | ||
| if (size < 1_000_000_000) return MEGABYTES; | ||
| return GIGABYTES; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding overflow protection in chooseUnit method.
The method should handle potential overflow when calculating size thresholds.
public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) {
if (useBinaryUnits) {
if (size < 1024) return BYTES;
- if (size < 1024 * 1024) return KIBIBYTES;
- if (size < 1024L * 1024 * 1024) return MEBIBYTES;
+ if (size < 1024L * 1024L) return KIBIBYTES;
+ if (size < 1024L * 1024L * 1024L) return MEBIBYTES;
return GIBIBYTES;
} else {
if (size < 1_000) return BYTES;
- if (size < 1_000_000) return KILOBYTES;
- if (size < 1_000_000_000) return MEGABYTES;
+ if (size < 1_000_000L) return KILOBYTES;
+ if (size < 1_000_000_000L) return MEGABYTES;
return GIGABYTES;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) { | |
| if (useBinaryUnits) { | |
| if (size < 1024) return BYTES; | |
| if (size < 1024 * 1024) return KIBIBYTES; | |
| if (size < 1024L * 1024 * 1024) return MEBIBYTES; | |
| return GIBIBYTES; | |
| } else { | |
| if (size < 1_000) return BYTES; | |
| if (size < 1_000_000) return KILOBYTES; | |
| if (size < 1_000_000_000) return MEGABYTES; | |
| return GIGABYTES; | |
| } | |
| } | |
| public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) { | |
| if (useBinaryUnits) { | |
| if (size < 1024) return BYTES; | |
| if (size < 1024L * 1024L) return KIBIBYTES; | |
| if (size < 1024L * 1024L * 1024L) return MEBIBYTES; | |
| return GIBIBYTES; | |
| } else { | |
| if (size < 1_000) return BYTES; | |
| if (size < 1_000_000L) return KILOBYTES; | |
| if (size < 1_000_000_000L) return MEGABYTES; | |
| return GIGABYTES; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (1)
108-113: Consider enhancing the exception messages for clarity.
Including the numeric value in the error messages would help developers quickly identify the problematic input. For example:- throw new ArithmeticException("Size exceeds maximum Long value"); + throw new ArithmeticException( + "Parsed size exceeds maximum Long value: " + bytes + ". Ensure the size is within the acceptable range." + );Likewise for the minimum value check.
src/main/java/org/fungover/breeze/util/size/SizeUnit.java (1)
116-116: Enhance the error message to guide users toward supported suffixes.
Explicitly listing valid suffixes helps resolve confusion for the end user:- throw new IllegalArgumentException("Unknown size unit: " + suffix); + throw new IllegalArgumentException("Unknown size unit: " + suffix + + ". Valid units include B, KB, MB, GB, TB, KiB, MiB, GiB, TiB, and bit-based variants.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(1 hunks)src/main/java/org/fungover/breeze/util/size/SizeUnit.java(1 hunks)
🔇 Additional comments (2)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (2)
39-41: Thank you for validating the decimalPlaces parameter.
This check for negative decimal places fulfills a common input validation requirement and aligns with the previous feedback.
74-75: Good job implementing null checks for the units here.
This ensures thatfromUnitandtoUnitcan never cause aNullPointerException, reflecting the past recommendations.
| private static SizeUnit parseUnit(String unitPart) { | ||
| for (SizeUnit unit : SizeUnit.values()) { | ||
| if (unit.getSuffix().equalsIgnoreCase(unitPart)) { | ||
| return unit; | ||
| } | ||
| } | ||
| throw new IllegalArgumentException(String.format("Unrecognized unit: %s", unitPart)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicating logic by reusing SizeUnit.fromSuffix(...).
The parseUnit method replicates the same matching logic implemented in SizeUnit.fromSuffix. You could simplify maintenance by delegating calls to SizeUnit.fromSuffix:
-private static SizeUnit parseUnit(String unitPart) {
- for (SizeUnit unit : SizeUnit.values()) {
- if (unit.getSuffix().equalsIgnoreCase(unitPart)) {
- return unit;
- }
- }
- throw new IllegalArgumentException(String.format("Unrecognized unit: %s", unitPart));
-}
+private static SizeUnit parseUnit(String unitPart) {
+ return SizeUnit.fromSuffix(unitPart);
+}Additionally, consider listing recognized units in the error message for better clarity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static SizeUnit parseUnit(String unitPart) { | |
| for (SizeUnit unit : SizeUnit.values()) { | |
| if (unit.getSuffix().equalsIgnoreCase(unitPart)) { | |
| return unit; | |
| } | |
| } | |
| throw new IllegalArgumentException(String.format("Unrecognized unit: %s", unitPart)); | |
| } | |
| private static SizeUnit parseUnit(String unitPart) { | |
| return SizeUnit.fromSuffix(unitPart); | |
| } |
| if (useBinaryUnits) { | ||
| if (size < 1024) return BYTES; | ||
| if (size < 1024 * 1024) return KIBIBYTES; | ||
| if (size < 1024L * 1024 * 1024) return MEBIBYTES; | ||
| if (size < 1024L * 1024L) return KIBIBYTES; | ||
| if (size < 1024L * 1024L * 1024L) return MEBIBYTES; | ||
| return GIBIBYTES; | ||
| } else { | ||
| if (size < 1_000) return BYTES; | ||
| if (size < 1_000_000) return KILOBYTES; | ||
| if (size < 1_000_000_000) return MEGABYTES; | ||
| if (size < 1_000_000L) return KILOBYTES; | ||
| if (size < 1_000_000_000L) return MEGABYTES; | ||
| return GIGABYTES; | ||
| } | ||
| } |
There was a problem hiding this comment.
Eliminate duplicated conditions and include missing thresholds for TEBIBYTES and TERABYTES.
The binary path repeats checks for KiB and MiB, leaving no path for TEBIBYTES, while the decimal path repeats checks for KB and MB, leaving no path for TERABYTES. This likely causes incorrect unit selection and is a major logic bug. Consider a unified approach such as:
public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) {
if (useBinaryUnits) {
- if (size < 1024) return BYTES;
- if (size < 1024 * 1024) return KIBIBYTES;
- if (size < 1024L * 1024 * 1024) return MEBIBYTES;
- if (size < 1024L * 1024L) return KIBIBYTES;
- if (size < 1024L * 1024L * 1024L) return MEBIBYTES;
- return GIBIBYTES;
+ if (size < 1024) return BYTES;
+ else if (size < 1024L * 1024L) return KIBIBYTES;
+ else if (size < 1024L * 1024L * 1024L) return MEBIBYTES;
+ else if (size < 1024L * 1024L * 1024L * 1024L) return GIBIBYTES;
+ return TEBIBYTES;
} else {
- if (size < 1_000) return BYTES;
- if (size < 1_000_000) return KILOBYTES;
- if (size < 1_000_000_000) return MEGABYTES;
- if (size < 1_000_000L) return KILOBYTES;
- if (size < 1_000_000_000L) return MEGABYTES;
- return GIGABYTES;
+ if (size < 1_000) return BYTES;
+ else if (size < 1_000_000L) return KILOBYTES;
+ else if (size < 1_000_000_000L) return MEGABYTES;
+ else if (size < 1_000_000_000_000L) return GIGABYTES;
+ return TERABYTES;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (useBinaryUnits) { | |
| if (size < 1024) return BYTES; | |
| if (size < 1024 * 1024) return KIBIBYTES; | |
| if (size < 1024L * 1024 * 1024) return MEBIBYTES; | |
| if (size < 1024L * 1024L) return KIBIBYTES; | |
| if (size < 1024L * 1024L * 1024L) return MEBIBYTES; | |
| return GIBIBYTES; | |
| } else { | |
| if (size < 1_000) return BYTES; | |
| if (size < 1_000_000) return KILOBYTES; | |
| if (size < 1_000_000_000) return MEGABYTES; | |
| if (size < 1_000_000L) return KILOBYTES; | |
| if (size < 1_000_000_000L) return MEGABYTES; | |
| return GIGABYTES; | |
| } | |
| } | |
| if (useBinaryUnits) { | |
| if (size < 1024) return BYTES; | |
| else if (size < 1024L * 1024L) return KIBIBYTES; | |
| else if (size < 1024L * 1024L * 1024L) return MEBIBYTES; | |
| else if (size < 1024L * 1024L * 1024L * 1024L) return GIBIBYTES; | |
| return TEBIBYTES; | |
| } else { | |
| if (size < 1_000) return BYTES; | |
| else if (size < 1_000_000L) return KILOBYTES; | |
| else if (size < 1_000_000_000L) return MEGABYTES; | |
| else if (size < 1_000_000_000_000L) return GIGABYTES; | |
| return TERABYTES; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/fungover/breeze/util/size/SizeUnit.java (2)
87-91: Consider clarifying negative quantity behavior.When using
toByteson a negative BigDecimal, the result is negative bytes. Ensure downstream code can handle negative values properly (e.g., invalid input or meaningful scenario?).
126-140: Check handling of negative sizes inchooseUnit.Currently, a negative
sizedefaults toBYTESifuseBinaryUnitsis true and is also handled asBYTESifuseBinaryUnitsis false andsizeis beyond the threshold. If negative sizes are valid, consider clarifying or explicitly documenting this approach.src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (1)
88-116: Parsing logic is clear and robust, but consider extra context in errors.The exception messages are sufficient. Optionally, including recognized units in error messages can help guide users to valid suffixes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(1 hunks)src/main/java/org/fungover/breeze/util/size/SizeUnit.java(1 hunks)
🔇 Additional comments (6)
src/main/java/org/fungover/breeze/util/size/SizeUnit.java (3)
12-30: Enums look comprehensive and consistent for decimal and binary units.No issues found with the definitions and initialization for decimal (byte-based, bit-based) and binary units. Good job covering a wide range of units up to Tera/Tebi.
38-52: Constructor logic is well-structured.Storing the
baseFactoronce in the constructor for repeated use is efficient.
148-154: Comprehensive TERA bit rate coverage.Including
TERABITSensures large bit rates are represented accurately. Nicely done.src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (3)
30-53: Good iteration strategy for auto-formatting sizes.Sequentially checking from larger to smaller units is a neat approach. This ensures you only pick the first unit whose magnitude reaches at least 1.
65-80: Nice conversion method with null checks and rounding.The method properly checks for null units, preventing
NullPointerException.
126-155:formatRateprovides a flexible approach for bit rates.The logic for enumerating from Tera down to bits is straightforward. This is well-structured.
| public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) { | ||
| if (useBinaryUnits) { | ||
| if (size < 1024) return SizeUnit.BYTES; | ||
| if (size < 1024L * 1024L) return SizeUnit.KIBIBYTES; | ||
| if (size < 1024L * 1024L * 1024L) return SizeUnit.MEBIBYTES; | ||
| return SizeUnit.GIBIBYTES; | ||
| } else { | ||
| if (size < 1_000) return SizeUnit.BYTES; | ||
| if (size < 1_000_000L) return SizeUnit.KILOBYTES; | ||
| if (size < 1_000_000_000L) return SizeUnit.MEGABYTES; | ||
| return SizeUnit.GIGABYTES; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent unit range compared to SizeUnit.chooseUnit.
SizeFormatter.chooseUnit stops at gigabytes/gibibytes. Meanwhile, SizeUnit.chooseUnit includes terabytes/tebibytes. If you intend to cover sizes ≥ 1 TB/TiB, update SizeFormatter.chooseUnit for consistency.
- if (size < 1024L * 1024L * 1024L) return SizeUnit.MEBIBYTES;
- return SizeUnit.GIBIBYTES;
+ if (size < 1024L * 1024L * 1024L) return SizeUnit.MEBIBYTES;
+ if (size < 1024L * 1024L * 1024L * 1024L) return SizeUnit.GIBIBYTES;
+ return SizeUnit.TEBIBYTES;Similarly extend the decimal path to support TERABYTES.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) { | |
| if (useBinaryUnits) { | |
| if (size < 1024) return SizeUnit.BYTES; | |
| if (size < 1024L * 1024L) return SizeUnit.KIBIBYTES; | |
| if (size < 1024L * 1024L * 1024L) return SizeUnit.MEBIBYTES; | |
| return SizeUnit.GIBIBYTES; | |
| } else { | |
| if (size < 1_000) return SizeUnit.BYTES; | |
| if (size < 1_000_000L) return SizeUnit.KILOBYTES; | |
| if (size < 1_000_000_000L) return SizeUnit.MEGABYTES; | |
| return SizeUnit.GIGABYTES; | |
| } | |
| } | |
| public static SizeUnit chooseUnit(long size, boolean useBinaryUnits) { | |
| if (useBinaryUnits) { | |
| if (size < 1024) return SizeUnit.BYTES; | |
| if (size < 1024L * 1024L) return SizeUnit.KIBIBYTES; | |
| if (size < 1024L * 1024L * 1024L) return SizeUnit.MEBIBYTES; | |
| if (size < 1024L * 1024L * 1024L * 1024L) return SizeUnit.GIBIBYTES; | |
| return SizeUnit.TEBIBYTES; | |
| } else { | |
| if (size < 1_000) return SizeUnit.BYTES; | |
| if (size < 1_000_000L) return SizeUnit.KILOBYTES; | |
| if (size < 1_000_000_000L) return SizeUnit.MEGABYTES; | |
| if (size < 1_000_000_000_000L) return SizeUnit.GIGABYTES; | |
| return SizeUnit.TERABYTES; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (2)
46-53: Potential improved fallback logic inautoFormat.
The current loop picks the first unit whoseconverted.abs()is ≥ 1. For extremely small non-zero values (e.g., 0.5 bytes) or extremely large values beyond TB/TiB, this might affect user readability. You may want to either continue iterating for a more appropriate unit or handle sub-byte or multi-terabyte data in a separate branch.
88-93: Clarify numeric value error messages for user guidance.
When aNumberFormatExceptionoccurs, consider adding text clarifying valid number formats accepted by this API (e.g., integer, decimal, leading “.”) to guide the user in correctly formatting input.- throw new IllegalArgumentException(String.format("Invalid numeric value: %s", matcher.group(1)), e); + throw new IllegalArgumentException(String.format( + "Invalid numeric value: '%s'. Accepted formats include integers or decimal values like 1.5 or .75.", + matcher.group(1)), e);src/main/java/org/fungover/breeze/util/size/SizeUnit.java (2)
26-30: Confirm consistent suffix usage for bits.
"Kb", "Mb", "Gb", "Tb" may cause confusion with typical uppercase/lowercase conventions. If you want to strictly adhere to widespread naming, consider changing these suffixes (e.g., "kb", "Mb", etc.) or clearly documenting the chosen style.
111-117: Improve the exception message by listing valid units.
Users might not know which suffixes are valid. Including them in the error message can reduce confusion.- throw new IllegalArgumentException("Unknown size unit: " + suffix); + throw new IllegalArgumentException("Unknown size unit: " + suffix + + ". Valid suffixes are: B, KB, MB, GB, TB, KiB, MiB, GiB, TiB, b, Kb, Mb, Gb, Tb.");src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (1)
132-136: Add tests for even larger sizes if your domain includes PB/EiB.
If your application expects data sizes beyond terabytes, you may want to confirm that no overflow or unexpected behavior occurs when dealing with larger orders of magnitude like petabytes (PB/EiB).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(1 hunks)src/main/java/org/fungover/breeze/util/size/SizeUnit.java(1 hunks)src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java(1 hunks)
🔇 Additional comments (6)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java (3)
64-66: Appreciation for null-checking units.
The decision to immediately throw iffromUnitortoUnitis null helps avoid silent failures and clarifies correct usage of this method.
139-147:Details
❓ Verification inconclusive
Consider adding negative bit rate handling.
Currently, negative bit rates are not explicitly addressed. If negative rates are valid in your domain (e.g., flows in reverse directions), you might want to handle or reject them consistently.Would you like me to generate a script to see if any of your production code passes negative bit values into
formatRate?
🏁 Script executed:
#!/bin/bash rg -A 5 'formatRate\([^,]+-[0-9]'Length of output: 35
Negative Bit Rate Handling – Please Verify
- The code in
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(lines 139–147) currently does not include explicit handling for negative bit rate values.- A grep for literal negative values in calls to
formatRatedid not return any results. However, this only confirms that negative literals are not directly passed—it does not exclude the possibility of negative values being computed or passed via variables.- Recommendation: Determine if negative bit rates are valid in your domain:
- If they are, augment the logic to correctly format and display them.
- If not, consider adding a guard clause that rejects negative values with a clear exception or message.
Please review whether negative bit rates can occur in practice and adjust the method accordingly.
20-23:Details
❓ Verification inconclusive
Consider extending valid characters for unit suffixes.
Right now, the regex enforces[a-z]+withCASE_INSENSITIVEmode, which works for many cases. However, users might type suffixes with symbols (e.g., "MB/s"), or might expect uppercase/lowercase mixing with non-alphabetic characters (like "MiB/s"). If you plan to accept such inputs in the future, consider refining the regex pattern.Would you like to check your codebase and tests for any references to alternative suffix conventions (e.g., "MB/s" or trailing symbols)?
🏁 Script executed:
#!/bin/bash # Look for references to "MB/s", "MiB/s", or other pattern usage in the test or code. rg -A 5 'MB/s|MiB/s'Length of output: 22
Regex Unit Suffix Extension Suggestion
It appears that no references to alternative unit suffix formats (such as "MB/s" or "MiB/s") were found in the codebase or tests. While the current regex (
[a-z]+withCASE_INSENSITIVE) meets existing usage, if you plan to support inputs containing symbols or mixed-case non-alphabetic characters in the future, the regex will need to be refined accordingly.
- Confirm that no existing tests or documentation depend on the current strict unit suffix format.
- If extended suffix formats are anticipated, update the regex pattern in
src/main/java/org/fungover/breeze/util/size/SizeFormatter.javato cover symbols (e.g.,/) and potentially other characters.src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (3)
18-23: Excellent coverage of decimal vs. binary auto-format.
Testing bothuseBinaryBase = trueandfalseensures that theautoFormatlogic is exercised fully.
57-66: Good inclusion of boundary checks for very large values.
Catching exceptions when parsing values aboveLong.MAX_VALUEhelps maintain robust behavior and clarifies expected usage limits.
108-112: Solid coverage for null checks in conversions.
Verifying that converting withnullunits throws an exception prevents silent usage errors.
|
Your code is clear and understandable, but you could add Javadoc to make it even more readable. Also, I noticed that some of your commits are not signed, so it might be a good idea to fix that as well. |
Stelle83
left a comment
Stelle83
left a comment
There was a problem hiding this comment.
The changes improve the clarity and maintainability of the code by simplifying patterns, removing unused methods, and enhancing exception messages.
Overall, the refactoring and new tests look good and contribute positively to the codebase.
JesperSandklef
left a comment
JesperSandklef
left a comment
There was a problem hiding this comment.
Code looks good after the changes. It has improved the maintainability and looks cleaner with the unused methods removed. Approved
|
To get all your commits signed you can use rebase in intellij. |
…rn values, and exceptions Signed-off-by: Patti88 <patricia.nilsson@iths.se>
Patti88
dismissed stale reviews from JesperSandklef and Stelle83
via
c8b5f3a
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (4)
77-90: Consider harmonizing test method naming conventions.There's an inconsistency in the test method naming convention. Some methods use camelCase (e.g.,
autoFormatUsesCorrectUnits), while others use underscores (e.g.,parseInvalidNumbers_throwsProperExceptions). Standardizing on a single naming convention would improve readability.Also applies to: 95-100
15-23: Consider testing with different decimal place values.The
autoFormatmethod is currently only tested with 2 decimal places. Consider adding test cases with different values (0, 1, 3, etc.) to ensure the functionality works correctly with various precision settings.
136-139: Add test for zero value.The tests cover large values, negative values, and exact thresholds, but there's no explicit test for a zero byte value. Consider adding a test case to verify the correct behavior when formatting zero bytes.
@Test void testLargeSizes() { assertEquals("1.50 TB", SizeFormatter.autoFormat(1_500_000_000_000L, false, 2)); assertEquals("1.00 TiB", SizeFormatter.autoFormat(1_099_511_627_776L, true, 2)); + assertEquals("0.00 B", SizeFormatter.autoFormat(0, true, 2)); + assertEquals("0.00 B", SizeFormatter.autoFormat(0, false, 2)); }
34-42: Enhance test coverage for parsing.While the basic parsing functionality is tested, consider adding more test cases to cover additional units (MB, GB, TB, etc.) and different formats (with/without spaces, uppercase/lowercase units).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/fungover/breeze/util/size/SizeFormatter.java(1 hunks)src/main/java/org/fungover/breeze/util/size/SizeUnit.java(1 hunks)src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/fungover/breeze/util/size/SizeUnit.java
- src/main/java/org/fungover/breeze/util/size/SizeFormatter.java
🔇 Additional comments (2)
src/test/java/org/fungover/breeze/util/size/SizeFormatterTest.java (2)
1-152: Well-structured test class with comprehensive coverage.The test class is well-designed with good organization, clear test methods, and thorough Javadoc documentation. The tests cover a wide range of scenarios including both binary and decimal units, edge cases, error conditions, and precision handling.
58-66: Well-handled edge cases.The edge case handling is thorough, testing for values that exceed Long.MAX_VALUE, negative values, and validating that the parser can handle legitimate large values.
AfagMamedova
left a comment
AfagMamedova
left a comment
There was a problem hiding this comment.
Hej!
Det här var en smart funktion som gör det lättare att läsa storlekar, till exempel när man jobbar med filer.
Bra att det går att visa storlekar i t.ex. MB och GB.
Ser ut att fungera bra och koden är lätt att förstå.
Det skulle vara bra om man kan välja mellan 1000 och 1024 som bas.
Kanske lägga till ett felmeddelande om man skriver in något konstigt.
5 participants
Summary by CodeRabbit
New Features
Tests