GH-40 Add simple duration util #40
Conversation
WalkthroughThe pull request introduces a new utility class, The class provides two overloaded static methods named Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
eternalcode-commons-shared/src/main/java/com/eternalcode/commons/time/SimpleDurationUtil.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
eternalcode-commons-shared/src/main/java/com/eternalcode/commons/time/SimpleDurationUtil.java (2)
31-41: LGTM with a minor suggestion: format method logic is sound.The method effectively uses the removeMillis flag to determine the formatting approach, and handles the special case for durations less than one second when removeMillis is true.
Consider extracting the magic string "0s" to use the ZERO_SECONDS constant for consistency:
if (duration.toMillis() < ONE_SECOND.toMillis()) { - return "0s"; + return ZERO_SECONDS; }
1-48: LGTM: Well-implemented duration formatting utility with minor suggestions.The
SimpleDurationUtilclass provides a comprehensive and flexible solution for formattingDurationobjects. The implementation is clean, well-structured, and effectively fulfills its purpose.Consider the following suggestions for further improvement:
- Add JavaDoc comments to the class and public methods to provide clear documentation of their purpose and usage.
- Consider adding private constructor to prevent instantiation, as this is a utility class with only static methods.
Example improvements:
/** * Utility class for formatting Duration objects into human-readable strings. */ public class SimpleDurationUtil { private SimpleDurationUtil() { throw new UnsupportedOperationException("Utility class"); } // ... (other code remains the same) /** * Formats a Duration into a human-readable string. * * @param duration The Duration to format * @param removeMillis If true, milliseconds are excluded from the output * @return A formatted string representation of the Duration */ public static String format(Duration duration, boolean removeMillis) { // ... (method body remains the same) } /** * Formats a Duration into a human-readable string, including milliseconds. * * @param duration The Duration to format * @return A formatted string representation of the Duration, including milliseconds */ public static String format(Duration duration) { // ... (method body remains the same) } }These changes would enhance the usability and maintainability of the class.
eternalcode-commons-shared/test/com/eternalcode/commons/time/SimpleDurationUtilTest.java (1)
79-88: Enhance default format testing and add assertion messagesThe
testFormatDefaultmethod can be improved in the following ways:
- Add assertion messages to make debugging easier.
- Include more test cases to cover a wider range of scenarios.
Consider the following improvements:
- Add descriptive assertion messages to each assertion:
- assertEquals("10m10s", result); + assertEquals("10m10s", result, "Duration of 610 seconds should be formatted as '10m10s'"); - assertEquals("120ms", result); + assertEquals("120ms", result, "Duration of 120 milliseconds should be formatted as '120ms'");
- Add more test cases to cover different scenarios, such as:
- A duration with hours, minutes, and seconds
- A duration with days
- A duration with weeks or months
- A duration with years
For example:
duration = Duration.ofHours(25); result = SimpleDurationUtil.format(duration); assertEquals("1d1h", result, "Duration of 25 hours should be formatted as '1d1h'"); duration = Duration.ofDays(400); result = SimpleDurationUtil.format(duration); assertEquals("1y1mo5d", result, "Duration of 400 days should be formatted as '1y1mo5d'");These additional test cases will help ensure the default formatting behavior is correct across various duration ranges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- eternalcode-commons-shared/src/main/java/com/eternalcode/commons/time/SimpleDurationUtil.java (1 hunks)
- eternalcode-commons-shared/test/com/eternalcode/commons/time/SimpleDurationUtilTest.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
eternalcode-commons-shared/src/main/java/com/eternalcode/commons/time/SimpleDurationUtil.java (5)
1-6: LGTM: Class declaration and imports are appropriate.The package name, class name, and imports are well-chosen and relevant to the utility's purpose of formatting Duration objects.
8-9: LGTM: Constants are well-defined and appropriately named.The
ONE_SECONDandZERO_SECONDSconstants are clearly named and will be useful in the formatting logic.
11-19: LGTM: WITHOUT_MILLIS_FORMAT parser is well-configured.The parser is comprehensively set up to handle durations from seconds to years, rounding off to the nearest millisecond. This aligns well with its purpose of formatting durations without millisecond precision.
21-29: LGTM: STANDARD_FORMAT parser is well-configured.The STANDARD_FORMAT parser extends the capabilities of WITHOUT_MILLIS_FORMAT by including milliseconds, providing a comprehensive solution for standard duration formatting with full precision.
43-45: LGTM: Overloaded format method provides good default behavior.This overloaded method offers a convenient way to format durations with milliseconds by default, maintaining a clean and intuitive API.
| package com.eternalcode.commons.time; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import java.time.Duration; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class SimpleDurationUtilTest { | ||
|
|
||
| @Test | ||
| public void testFormatWithoutMillis() { | ||
| Duration duration = Duration.ofMillis(500); | ||
| String result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("0s", result); | ||
|
|
||
| duration = Duration.ofSeconds(30); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("30s", result); | ||
|
|
||
| duration = Duration.ofMinutes(5); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("5m", result); | ||
|
|
||
| duration = Duration.ofHours(2); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("2h", result); | ||
|
|
||
| duration = Duration.ofDays(1); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("1d", result); | ||
|
|
||
| duration = Duration.ofDays(14); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("2w", result); | ||
|
|
||
| duration = Duration.ofDays(60); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("2mo", result); | ||
|
|
||
| duration = Duration.ofDays(365 * 3); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("3y", result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testFormatWithMillis() { | ||
| Duration duration = Duration.ofMillis(500); | ||
| String result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("500ms", result); | ||
|
|
||
| duration = Duration.ofSeconds(30); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("30s", result); | ||
|
|
||
| duration = Duration.ofMinutes(5); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("5m", result); | ||
|
|
||
| duration = Duration.ofHours(2); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("2h", result); | ||
|
|
||
| duration = Duration.ofDays(1); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("1d", result); | ||
|
|
||
| duration = Duration.ofDays(14); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("2w", result); | ||
|
|
||
| duration = Duration.ofDays(60); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("2mo", result); | ||
|
|
||
| duration = Duration.ofDays(365 * 3); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("3y", result); | ||
| } | ||
|
|
||
| @Test | ||
| public void testFormatDefault() { | ||
| Duration duration = Duration.ofSeconds(610); | ||
| String result = SimpleDurationUtil.format(duration); | ||
| assertEquals("10m10s", result); | ||
|
|
||
| duration = Duration.ofMillis(120); | ||
| result = SimpleDurationUtil.format(duration); | ||
| assertEquals("120ms", result); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance overall test class structure and coverage
While the test class covers the main functionality of SimpleDurationUtil, there are several areas where it can be improved:
- Add a class-level Javadoc comment explaining the purpose of the test class.
- Include tests for edge cases and error scenarios.
- Improve the organization of test methods.
Consider the following improvements:
- Add a class-level Javadoc comment:
/**
* Test class for SimpleDurationUtil.
* This class contains unit tests to verify the correct functioning
* of the SimpleDurationUtil.format method under various scenarios.
*/
public class SimpleDurationUtilTest {
// ... existing code ...
}- Add tests for edge cases and error scenarios:
@Test
public void testFormatZeroDuration() {
Duration duration = Duration.ZERO;
String result = SimpleDurationUtil.format(duration);
assertEquals("0s", result, "Zero duration should be formatted as '0s'");
}
@Test
public void testFormatNegativeDuration() {
Duration duration = Duration.ofSeconds(-60);
String result = SimpleDurationUtil.format(duration);
assertEquals("-1m", result, "Negative duration should be formatted with a minus sign");
}
@Test
public void testFormatMaxDuration() {
Duration duration = Duration.ofSeconds(Long.MAX_VALUE);
String result = SimpleDurationUtil.format(duration);
// Assert the expected result for maximum duration
}- Improve organization using nested classes or descriptive naming:
class SimpleDurationUtilTest {
@Nested
class FormatWithoutMillis {
@Test
void shouldFormatSecondsWithoutMillis() {
// ... test case for seconds ...
}
@Test
void shouldFormatMinutesWithoutMillis() {
// ... test case for minutes ...
}
// ... other test methods ...
}
@Nested
class FormatWithMillis {
// ... similar structure for with-millis tests ...
}
@Nested
class FormatDefault {
// ... default format test methods ...
}
@Nested
class EdgeCases {
// ... edge case test methods ...
}
}These improvements will enhance the overall structure, readability, and coverage of the test class.
| @Test | ||
| public void testFormatWithoutMillis() { | ||
| Duration duration = Duration.ofMillis(500); | ||
| String result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("0s", result); | ||
|
|
||
| duration = Duration.ofSeconds(30); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("30s", result); | ||
|
|
||
| duration = Duration.ofMinutes(5); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("5m", result); | ||
|
|
||
| duration = Duration.ofHours(2); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("2h", result); | ||
|
|
||
| duration = Duration.ofDays(1); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("1d", result); | ||
|
|
||
| duration = Duration.ofDays(14); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("2w", result); | ||
|
|
||
| duration = Duration.ofDays(60); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("2mo", result); | ||
|
|
||
| duration = Duration.ofDays(365 * 3); | ||
| result = SimpleDurationUtil.format(duration, true); | ||
| assertEquals("3y", result); | ||
| } |
There was a problem hiding this comment.
Improve test method clarity and add assertion messages
The test method testFormatWithoutMillis has a few areas for improvement:
- The method name suggests testing without milliseconds, but the
includeMillisparameter is set totrue. This is counterintuitive and may lead to confusion. - The test cases lack assertion messages, which could make debugging failures more difficult.
Consider the following improvements:
- Rename the method to
testFormatWithoutMillisto accurately reflect its purpose. - Change the
SimpleDurationUtil.format(duration, true)calls toSimpleDurationUtil.format(duration, false)to match the method name. - Add descriptive assertion messages to each assertion. For example:
- assertEquals("0s", result);
+ assertEquals("0s", result, "Duration of 500ms should be formatted as '0s' without milliseconds");Apply this pattern to all assertions in the method.
Committable suggestion was skipped due to low confidence.
| @Test | ||
| public void testFormatWithMillis() { | ||
| Duration duration = Duration.ofMillis(500); | ||
| String result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("500ms", result); | ||
|
|
||
| duration = Duration.ofSeconds(30); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("30s", result); | ||
|
|
||
| duration = Duration.ofMinutes(5); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("5m", result); | ||
|
|
||
| duration = Duration.ofHours(2); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("2h", result); | ||
|
|
||
| duration = Duration.ofDays(1); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("1d", result); | ||
|
|
||
| duration = Duration.ofDays(14); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("2w", result); | ||
|
|
||
| duration = Duration.ofDays(60); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("2mo", result); | ||
|
|
||
| duration = Duration.ofDays(365 * 3); | ||
| result = SimpleDurationUtil.format(duration, false); | ||
| assertEquals("3y", result); | ||
| } |
There was a problem hiding this comment.
Improve test method clarity and add assertion messages
The test method testFormatWithMillis has similar issues to the previous method:
- The method name suggests testing with milliseconds, but the
includeMillisparameter is set tofalse. This is counterintuitive and may lead to confusion. - The test cases lack assertion messages, which could make debugging failures more difficult.
Consider the following improvements:
- Rename the method to
testFormatWithMillisto accurately reflect its purpose. - Change the
SimpleDurationUtil.format(duration, false)calls toSimpleDurationUtil.format(duration, true)to match the method name. - Add descriptive assertion messages to each assertion. For example:
- assertEquals("500ms", result);
+ assertEquals("500ms", result, "Duration of 500ms should be formatted as '500ms' with milliseconds");Apply this pattern to all assertions in the method.
Committable suggestion was skipped due to low confidence.
|
This way you avoid bugs casued by commits to DurationParser by @vLuckyyy |
Summary by CodeRabbit
New Features
SimpleDurationUtilclass for formattingDurationobjects in Java.Tests
SimpleDurationUtilto ensure accurate formatting of durations under various scenarios.