garydgregory commented on code in PR #1473:
URL: https://github.com/apache/commons-lang/pull/1473#discussion_r2467107041


##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -326,6 +335,16 @@ public String getMessage() {
         return message;
     }
 
+    /**
+     * Gets the split history list.
+     *
+     * @return the list of splits.
+     * @since 3.19.0
+     */
+    public List<Split> getSplitHistory() {

Review Comment:
   "getSplits()" is better. It's a list of splits with the order implied by the 
context (a stopwatch).



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -746,4 +784,45 @@ public void unsplit() {
         splitState = SplitState.UNSPLIT;
     }
 
+    /**
+     * Class to store details of each split.

Review Comment:
   This is a bit of a circular definition IMO: a Split is ... a split. Just say 
what a split is.



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -746,4 +784,45 @@ public void unsplit() {
         splitState = SplitState.UNSPLIT;
     }
 
+    /**
+     * Class to store details of each split.
+     * @since 3.19.0
+     */
+    public static final class Split extends ImmutablePair<String, Long> {
+
+        /**
+         * Constructor with label and duration.
+         * @param label Label for this split.
+         * @param timeNanos time in ns.
+         */
+        public Split(String label, Long timeNanos) {
+            super(label, timeNanos);
+        }
+
+        /**
+         * Get the label of this split.

Review Comment:
   Javadoc: A getter "Gets ..."



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -746,4 +784,45 @@ public void unsplit() {
         splitState = SplitState.UNSPLIT;
     }
 
+    /**
+     * Class to store details of each split.
+     * @since 3.19.0
+     */
+    public static final class Split extends ImmutablePair<String, Long> {
+
+        /**
+         * Constructor with label and duration.
+         * @param label Label for this split.
+         * @param timeNanos time in ns.
+         */
+        public Split(String label, Long timeNanos) {
+            super(label, timeNanos);
+        }
+
+        /**
+         * Get the label of this split.
+         * @return label.
+         */
+        public String getLabel() {
+            return getLeft();
+        }
+
+        /**
+         * Get the time in nanoseconds.

Review Comment:
   Javadoc: A getter "Gets ..."



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -248,6 +252,11 @@ public static StopWatch createStarted() {
      */
     private long stopTimeNanos;
 
+    /**
+     * The split history list.
+     */
+    private List<Split> splitHistory;

Review Comment:
   "split" is enough IMO. 



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -746,4 +784,45 @@ public void unsplit() {
         splitState = SplitState.UNSPLIT;
     }
 
+    /**
+     * Class to store details of each split.
+     * @since 3.19.0
+     */
+    public static final class Split extends ImmutablePair<String, Long> {
+
+        /**
+         * Constructor with label and duration.
+         * @param label Label for this split.
+         * @param timeNanos time in ns.
+         */
+        public Split(String label, Long timeNanos) {
+            super(label, timeNanos);
+        }
+
+        /**
+         * Get the label of this split.
+         * @return label.
+         */
+        public String getLabel() {
+            return getLeft();
+        }
+
+        /**
+         * Get the time in nanoseconds.
+         * @return time in ns.
+         */
+        public Long getTimeNanos() {
+            return getRight();
+        }
+
+        /**
+         * Converts this instance to a handy string.
+         * @return this instance as a string.
+         */
+        @Override
+        public String toString() {
+            return String.format("Split [label=%s, timeNanos=%d])", 
getLabel(), getTimeNanos());

Review Comment:
   With a Duration, you can simplify: `"Split [%s: %s]"`



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -626,6 +646,23 @@ public void split() {
         splitState = SplitState.SPLIT;
     }
 
+    /**
+     * <p>
+     * Captures the time in ns and records in a history list.
+     * The label specified is used to identify the current split.
+     * </p>
+     *
+     * @param label A message for string presentation.
+     * @throws IllegalStateException if the StopWatch is not running.
+     * @since 3.19.0
+     */
+    public void recordSplit(final String label) {

Review Comment:
   "split(String)" is simpler. The use case implies that splits are tracked by 
its stopwatch.



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -746,4 +784,45 @@ public void unsplit() {
         splitState = SplitState.UNSPLIT;
     }
 
+    /**
+     * Class to store details of each split.
+     * @since 3.19.0
+     */
+    public static final class Split extends ImmutablePair<String, Long> {

Review Comment:
   As mentioned before, this is a Duration, which is more flexible than a long 
hardcoded as nanoseconds. Using a Duration will display much more nicely in a 
toString() as well for simple "reports". Using a Duration offers more 
flexibility for users who want to perform operations on those time quantities.



##########
src/main/java/org/apache/commons/lang3/time/StopWatch.java:
##########
@@ -746,4 +784,45 @@ public void unsplit() {
         splitState = SplitState.UNSPLIT;
     }
 
+    /**
+     * Class to store details of each split.
+     * @since 3.19.0
+     */
+    public static final class Split extends ImmutablePair<String, Long> {
+
+        /**
+         * Constructor with label and duration.

Review Comment:
   Javadoc: A constructor "Constructs ..."



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to