[GitHub] [commons-lang] XenoAmess opened a new pull request #577: refine StringUtils.center

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess opened a new pull request #577: refine StringUtils.center

GitBox

XenoAmess opened a new pull request #577:
URL: https://github.com/apache/commons-lang/pull/577


   as title.


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] coveralls commented on pull request #577: [LANG-1588] refine StringUtils.center

GitBox

coveralls commented on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-651297946


   
   [![Coverage Status](https://coveralls.io/builds/31754329/badge)](https://coveralls.io/builds/31754329)
   
   Coverage decreased (-0.02%) to 94.66% when pulling **9b5a87efdfacf75943cb08ed659fd8cd9265c4a2 on XenoAmess:refine_center** into **da0f6a8051cc60d1bebe703b4e390ce3035497db on apache:master**.
   


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] garydgregory commented on pull request #577: [LANG-1588] refine StringUtils.center

GitBox
In reply to this post by GitBox

garydgregory commented on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-654825246


   I do not understand what this PR is about and you'd be correct in assuming that I did not parse out the new code in my head or run it in the debugger. What are you "refining" here? If you change the algorithm, please use the commit comment to explain why implementing this method in this manner is better. The commit comment and code comment are only way for anyone looking at the code and commit log to make sense of what your intent it is and why it happened. TY.


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess commented on pull request #577: [LANG-1588] refine StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess commented on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-654971110


   OK I will add some jmh test for this pr.
   will also try refine this pr.


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess edited a comment on pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess edited a comment on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-654971110


   @garydgregory
   This pr is a performance refine pr.
   I add some jmh test for this pr after you asking it.


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess commented on pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess commented on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-655519535


   jmh result:
   ```
   [INFO] --- exec-maven-plugin:1.6.0:exec (benchmark) @ commons-lang3 ---
   WARNING: An illegal reflective access operation has occurred
   WARNING: Illegal reflective access by org.openjdk.jmh.util.Utils (file:/C:/Users/xenoa/.m2/repository/org/openjdk/jmh/jmh-core/1
   .21/jmh-core-1.21.jar) to field java.io.PrintStream.charOut
   WARNING: Please consider reporting this to the maintainers of org.openjdk.jmh.util.Utils
   WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
   WARNING: All illegal access operations will be denied in a future release
   # JMH version: 1.21
   # VM version: JDK 13.0.2, OpenJDK 64-Bit Server VM, 13.0.2+8
   # VM invoker: C:\jdk-13.0.2+8\bin\java.exe
   # VM options: <none>
   # Warmup: 5 iterations, 10 s each
   # Measurement: 5 iterations, 10 s each
   # Timeout: 10 min per iteration
   # Threads: 1 thread, will synchronize iterations
   # Benchmark mode: Average time, time/op
   # Benchmark: org.apache.commons.lang3.StringUtilCenterTest.test0New
   
   # Run progress: 0.00% complete, ETA 00:33:20
   # Fork: 1 of 5
   # Warmup Iteration   1: 73.970 ns/op
   # Warmup Iteration   2: 69.102 ns/op
   # Warmup Iteration   3: 69.798 ns/op
   # Warmup Iteration   4: 66.163 ns/op
   # Warmup Iteration   5: 63.440 ns/op
   Iteration   1: 64.715 ns/op
   Iteration   2: 64.716 ns/op
   Iteration   3: 74.180 ns/op
   Iteration   4: 72.618 ns/op
   Iteration   5: 75.182 ns/op
   
   # Run progress: 5.00% complete, ETA 00:31:54
   # Fork: 2 of 5
   # Warmup Iteration   1: 67.586 ns/op
   # Warmup Iteration   2: 65.819 ns/op
   # Warmup Iteration   3: 63.447 ns/op
   # Warmup Iteration   4: 65.497 ns/op
   # Warmup Iteration   5: 63.994 ns/op
   Iteration   1: 63.189 ns/op
   Iteration   2: 65.755 ns/op
   Iteration   3: 65.839 ns/op
   Iteration   4: 65.091 ns/op
   Iteration   5: 63.715 ns/op
   
   # Run progress: 10.00% complete, ETA 00:30:12
   # Fork: 3 of 5
   # Warmup Iteration   1: 64.871 ns/op
   # Warmup Iteration   2: 65.010 ns/op
   # Warmup Iteration   3: 65.238 ns/op
   # Warmup Iteration   4: 63.760 ns/op
   # Warmup Iteration   5: 64.467 ns/op
   Iteration   1: 65.498 ns/op
   Iteration   2: 64.989 ns/op
   Iteration   3: 70.976 ns/op
   Iteration   4: 66.951 ns/op
   Iteration   5: 67.054 ns/op
   
   # Run progress: 15.00% complete, ETA 00:28:31
   # Fork: 4 of 5
   # Warmup Iteration   1: 66.880 ns/op
   # Warmup Iteration   2: 64.242 ns/op
   # Warmup Iteration   3: 64.745 ns/op
   # Warmup Iteration   4: 64.326 ns/op
   # Warmup Iteration   5: 64.886 ns/op
   Iteration   1: 63.967 ns/op
   Iteration   2: 63.959 ns/op
   Iteration   3: 65.041 ns/op
   Iteration   4: 63.518 ns/op
   Iteration   5: 64.119 ns/op
   
   # Run progress: 20.00% complete, ETA 00:26:50
   # Fork: 5 of 5
   # Warmup Iteration   1: 74.978 ns/op
   # Warmup Iteration   2: 73.869 ns/op
   # Warmup Iteration   3: 70.176 ns/op
   # Warmup Iteration   4: 64.718 ns/op
   # Warmup Iteration   5: 62.849 ns/op
   Iteration   1: 63.283 ns/op
   Iteration   2: 63.399 ns/op
   Iteration   3: 64.567 ns/op
   Iteration   4: 61.981 ns/op
   Iteration   5: 63.795 ns/op
   
   
   Result "org.apache.commons.lang3.StringUtilCenterTest.test0New":
     65.924 ?99.9%) 2.632 ns/op [Average]
   
     (min, avg, max) = (61.981, 65.924, 75.182), stdev = 3.514
     CI (99.9%): [63.291, 68.556] (assumes normal distribution)
   
   
   # JMH version: 1.21
   # VM version: JDK 13.0.2, OpenJDK 64-Bit Server VM, 13.0.2+8
   # VM invoker: C:\jdk-13.0.2+8\bin\java.exe
   # VM options: <none>
   # Warmup: 5 iterations, 10 s each
   # Measurement: 5 iterations, 10 s each
   # Timeout: 10 min per iteration
   # Threads: 1 thread, will synchronize iterations
   # Benchmark mode: Average time, time/op
   # Benchmark: org.apache.commons.lang3.StringUtilCenterTest.test0Old
   
   # Run progress: 25.00% complete, ETA 00:25:09
   # Fork: 1 of 5
   # Warmup Iteration   1: 67.660 ns/op
   # Warmup Iteration   2: 64.947 ns/op
   # Warmup Iteration   3: 64.171 ns/op
   # Warmup Iteration   4: 63.782 ns/op
   # Warmup Iteration   5: 63.521 ns/op
   Iteration   1: 64.632 ns/op
   Iteration   2: 63.704 ns/op
   Iteration   3: 63.486 ns/op
   Iteration   4: 64.546 ns/op
   Iteration   5: 64.219 ns/op
   
   # Run progress: 30.00% complete, ETA 00:23:29
   # Fork: 2 of 5
   # Warmup Iteration   1: 68.962 ns/op
   # Warmup Iteration   2: 64.948 ns/op
   # Warmup Iteration   3: 63.874 ns/op
   # Warmup Iteration   4: 66.906 ns/op
   # Warmup Iteration   5: 66.031 ns/op
   Iteration   1: 65.594 ns/op
   Iteration   2: 70.326 ns/op
   Iteration   3: 66.059 ns/op
   Iteration   4: 65.465 ns/op
   Iteration   5: 63.751 ns/op
   
   # Run progress: 35.00% complete, ETA 00:21:48
   # Fork: 3 of 5
   # Warmup Iteration   1: 75.793 ns/op
   # Warmup Iteration   2: 71.874 ns/op
   # Warmup Iteration   3: 76.445 ns/op
   # Warmup Iteration   4: 80.507 ns/op
   # Warmup Iteration   5: 80.730 ns/op
   Iteration   1: 75.477 ns/op
   Iteration   2: 72.640 ns/op
   Iteration   3: 86.545 ns/op
   Iteration   4: 72.018 ns/op
   Iteration   5: 72.236 ns/op
   
   # Run progress: 40.00% complete, ETA 00:20:07
   # Fork: 4 of 5
   # Warmup Iteration   1: 67.321 ns/op
   # Warmup Iteration   2: 65.013 ns/op
   # Warmup Iteration   3: 64.822 ns/op
   # Warmup Iteration   4: 64.609 ns/op
   # Warmup Iteration   5: 64.005 ns/op
   Iteration   1: 64.883 ns/op
   Iteration   2: 63.384 ns/op
   Iteration   3: 64.585 ns/op
   Iteration   4: 65.865 ns/op
   Iteration   5: 65.136 ns/op
   
   # Run progress: 45.00% complete, ETA 00:18:27
   # Fork: 5 of 5
   # Warmup Iteration   1: 72.468 ns/op
   # Warmup Iteration   2: 71.567 ns/op
   # Warmup Iteration   3: 70.487 ns/op
   # Warmup Iteration   4: 69.775 ns/op
   # Warmup Iteration   5: 72.405 ns/op
   Iteration   1: 69.492 ns/op
   Iteration   2: 70.049 ns/op
   Iteration   3: 70.439 ns/op
   Iteration   4: 70.214 ns/op
   Iteration   5: 69.640 ns/op
   
   
   Result "org.apache.commons.lang3.StringUtilCenterTest.test0Old":
     68.175 ?99.9%) 3.886 ns/op [Average]
   
     (min, avg, max) = (63.384, 68.175, 86.545), stdev = 5.188
     CI (99.9%): [64.289, 72.061] (assumes normal distribution)
   
   
   # JMH version: 1.21
   # VM version: JDK 13.0.2, OpenJDK 64-Bit Server VM, 13.0.2+8
   # VM invoker: C:\jdk-13.0.2+8\bin\java.exe
   # VM options: <none>
   # Warmup: 5 iterations, 10 s each
   # Measurement: 5 iterations, 10 s each
   # Timeout: 10 min per iteration
   # Threads: 1 thread, will synchronize iterations
   # Benchmark mode: Average time, time/op
   # Benchmark: org.apache.commons.lang3.StringUtilCenterTest.test1New
   
   # Run progress: 50.00% complete, ETA 00:16:46
   # Fork: 1 of 5
   # Warmup Iteration   1: 26326250.789 ns/op
   # Warmup Iteration   2: 25239957.683 ns/op
   # Warmup Iteration   3: 25217874.811 ns/op
   # Warmup Iteration   4: 29799395.536 ns/op
   # Warmup Iteration   5: 25481418.575 ns/op
   Iteration   1: 27424505.205 ns/op
   Iteration   2: 32866042.295 ns/op
   Iteration   3: 25211274.811 ns/op
   Iteration   4: 26647165.957 ns/op
   Iteration   5: 28455071.023 ns/op
   
   # Run progress: 55.00% complete, ETA 00:15:06
   # Fork: 2 of 5
   # Warmup Iteration   1: 27791962.222 ns/op
   # Warmup Iteration   2: 25545446.173 ns/op
   # Warmup Iteration   3: 25595086.701 ns/op
   # Warmup Iteration   4: 29474667.353 ns/op
   # Warmup Iteration   5: 23791697.150 ns/op
   Iteration   1: 25600432.481 ns/op
   Iteration   2: 28094228.933 ns/op
   Iteration   3: 21998013.187 ns/op
   Iteration   4: 30368663.333 ns/op
   Iteration   5: 24707792.840 ns/op
   
   # Run progress: 60.00% complete, ETA 00:13:26
   # Fork: 3 of 5
   # Warmup Iteration   1: 26658096.809 ns/op
   # Warmup Iteration   2: 25738426.992 ns/op
   # Warmup Iteration   3: 25444187.310 ns/op
   # Warmup Iteration   4: 29603887.870 ns/op
   # Warmup Iteration   5: 23952392.823 ns/op
   Iteration   1: 26057157.813 ns/op
   Iteration   2: 29099787.791 ns/op
   Iteration   3: 22069017.621 ns/op
   Iteration   4: 28992393.623 ns/op
   Iteration   5: 25628914.066 ns/op
   
   # Run progress: 65.00% complete, ETA 00:11:45
   # Fork: 4 of 5
   # Warmup Iteration   1: 26624345.745 ns/op
   # Warmup Iteration   2: 25712105.141 ns/op
   # Warmup Iteration   3: 25732705.913 ns/op
   # Warmup Iteration   4: 29031242.609 ns/op
   # Warmup Iteration   5: 23822108.571 ns/op
   Iteration   1: 26038135.065 ns/op
   Iteration   2: 28183252.958 ns/op
   Iteration   3: 23823259.286 ns/op
   Iteration   4: 33591714.094 ns/op
   Iteration   5: 33456041.806 ns/op
   
   # Run progress: 70.00% complete, ETA 00:10:05
   # Fork: 5 of 5
   # Warmup Iteration   1: 29365897.947 ns/op
   # Warmup Iteration   2: 27275929.700 ns/op
   # Warmup Iteration   3: 28926793.642 ns/op
   # Warmup Iteration   4: 29033741.739 ns/op
   # Warmup Iteration   5: 23825106.905 ns/op
   Iteration   1: 32482081.818 ns/op
   Iteration   2: 26602738.830 ns/op
   Iteration   3: 25330494.697 ns/op
   Iteration   4: 31557474.448 ns/op
   Iteration   5: 25133722.613 ns/op
   
   
   Result "org.apache.commons.lang3.StringUtilCenterTest.test1New":
     27576775.064 ?99.9%) 2497347.881 ns/op [Average]
   
     (min, avg, max) = (21998013.187, 27576775.064, 33591714.094), stdev = 3333887.972
     CI (99.9%): [25079427.182, 30074122.945] (assumes normal distribution)
   
   
   # JMH version: 1.21
   # VM version: JDK 13.0.2, OpenJDK 64-Bit Server VM, 13.0.2+8
   # VM invoker: C:\jdk-13.0.2+8\bin\java.exe
   # VM options: <none>
   # Warmup: 5 iterations, 10 s each
   # Measurement: 5 iterations, 10 s each
   # Timeout: 10 min per iteration
   # Threads: 1 thread, will synchronize iterations
   # Benchmark mode: Average time, time/op
   # Benchmark: org.apache.commons.lang3.StringUtilCenterTest.test1Old
   
   # Run progress: 75.00% complete, ETA 00:08:24
   # Fork: 1 of 5
   # Warmup Iteration   1: 42567488.936 ns/op
   # Warmup Iteration   2: 55084039.011 ns/op
   # Warmup Iteration   3: 40206120.800 ns/op
   # Warmup Iteration   4: 57288193.143 ns/op
   # Warmup Iteration   5: 40978082.857 ns/op
   Iteration   1: 57269681.143 ns/op
   Iteration   2: 42055589.076 ns/op
   Iteration   3: 51496538.462 ns/op
   Iteration   4: 44500411.111 ns/op
   Iteration   5: 52760451.579 ns/op
   
   # Run progress: 80.00% complete, ETA 00:06:43
   # Fork: 2 of 5
   # Warmup Iteration   1: 50089586.000 ns/op
   # Warmup Iteration   2: 40182082.731 ns/op
   # Warmup Iteration   3: 55439843.094 ns/op
   # Warmup Iteration   4: 38485115.769 ns/op
   # Warmup Iteration   5: 57708808.621 ns/op
   Iteration   1: 43127754.741 ns/op
   Iteration   2: 57522725.862 ns/op
   Iteration   3: 43491734.632 ns/op
   Iteration   4: 53273010.638 ns/op
   Iteration   5: 46896654.673 ns/op
   
   # Run progress: 85.00% complete, ETA 00:05:02
   # Fork: 3 of 5
   # Warmup Iteration   1: 57863772.832 ns/op
   # Warmup Iteration   2: 51218231.633 ns/op
   # Warmup Iteration   3: 80835640.476 ns/op
   # Warmup Iteration   4: 104292427.083 ns/op
   # Warmup Iteration   5: 108862696.739 ns/op
   Iteration   1: 47689726.190 ns/op
   Iteration   2: 99487373.267 ns/op
   Iteration   3: 46848762.150 ns/op
   Iteration   4: 62012329.630 ns/op
   Iteration   5: 59998337.725 ns/op
   
   # Run progress: 90.00% complete, ETA 00:03:22
   # Fork: 4 of 5
   # Warmup Iteration   1: 64110367.516 ns/op
   # Warmup Iteration   2: 51091306.122 ns/op
   # Warmup Iteration   3: 48142773.077 ns/op
   # Warmup Iteration   4: 48012818.660 ns/op
   # Warmup Iteration   5: 48532850.725 ns/op
   Iteration   1: 47408419.905 ns/op
   Iteration   2: 47231877.358 ns/op
   Iteration   3: 54101611.892 ns/op
   Iteration   4: 56448726.966 ns/op
   Iteration   5: 73093223.358 ns/op
   
   # Run progress: 95.00% complete, ETA 00:01:41
   # Fork: 5 of 5
   # Warmup Iteration   1: 58022134.682 ns/op
   # Warmup Iteration   2: 51917492.746 ns/op
   # Warmup Iteration   3: 50538762.626 ns/op
   # Warmup Iteration   4: 47698360.476 ns/op
   # Warmup Iteration   5: 47873885.646 ns/op
   Iteration   1: 46033436.239 ns/op
   Iteration   2: 44170253.744 ns/op
   Iteration   3: 54631745.109 ns/op
   Iteration   4: 45260548.649 ns/op
   Iteration   5: 48970378.537 ns/op
   
   
   Result "org.apache.commons.lang3.StringUtilCenterTest.test1Old":
     53031252.105 ?99.9%) 9048821.218 ns/op [Average]
   
     (min, avg, max) = (42055589.076, 53031252.105, 99487373.267), stdev = 12079917.438
     CI (99.9%): [43982430.887, 62080073.324] (assumes normal distribution)
   
   
   # Run complete. Total time: 00:33:42
   
   REMEMBER: The numbers below are just data. To gain reusable insights, you need to follow up on
   why the numbers are the way they are. Use profilers (see -prof, -lprof), design factorial
   experiments, perform baseline and negative tests that provide experimental control, make sure
   the benchmarking environment is safe on JVM/OS/HW level, ask for reviews from the domain experts.
   Do not assume the numbers tell you what you want them to tell.
   
   Benchmark                      Mode  Cnt         Score         Error  Units
   StringUtilCenterTest.test0New  avgt   25        65.924 ?      2.632  ns/op
   
   StringUtilCenterTest.test0Old  avgt   25        68.175 ?      3.886  ns/op
   
   StringUtilCenterTest.test1New  avgt   25  27576775.064 ?2497347.881  ns/op
   
   StringUtilCenterTest.test1Old  avgt   25  53031252.105 ?9048821.218  ns/op
   
   
   Benchmark result is saved to target/jmh-result.org.apache.json
   [INFO] ------------------------------------------------------------------------
   [INFO] BUILD SUCCESS
   [INFO] ------------------------------------------------------------------------
   [INFO] Total time:  34:25 min
   [INFO] Finished at: 2020-07-08T21:20:48+08:00
   [INFO] ------------------------------------------------------------------------
   ```


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] coveralls edited a comment on pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

coveralls edited a comment on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-651297946


   
   [![Coverage Status](https://coveralls.io/builds/31938939/badge)](https://coveralls.io/builds/31938939)
   
   Coverage increased (+0.002%) to 94.681% when pulling **e75a0fb9845c8a6befed653ddaad05e3eed72d64 on XenoAmess:refine_center** into **32a9a350c67f1418784fff266d7e7881ca6346ad on apache:master**.
   


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] garydgregory commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

garydgregory commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452264665



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6201,6 +6203,9 @@ public static String removeStartIgnoreCase(final String str, final String remove
         return str;
     }
 
+    // Padding

Review comment:
       Let's not use these kind of separator. I usually prefer to keep the methods sorted in AB order so its simple and easy to find methods.




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] garydgregory commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

garydgregory commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452264665



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6201,6 +6203,9 @@ public static String removeStartIgnoreCase(final String str, final String remove
         return str;
     }
 
+    // Padding

Review comment:
       Let's not add these kinds of separator. I usually prefer to keep the methods sorted in AB order so its simple and easy to find methods.




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] garydgregory commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

garydgregory commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452268209



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6234,8 +6239,34 @@ public static String repeat(final char ch, final int repeat) {
         return new String(buf);
     }
 
-    // Padding
-    //-----------------------------------------------------------------------
+    /**
+     * <p>Returns padding using the specified delimiter repeated
+     * to a given length.</p>
+     *
+     * <pre>
+     * StringUtils.repeat('e', 0)  = ""
+     * StringUtils.repeat('e', 3)  = "eee"
+     * StringUtils.repeat('e', -2) = ""
+     * </pre>
+     *
+     * <p>Note: this method does not support padding with
+     * <a href="http://www.unicode.org/glossary/#supplementary_character">Unicode Supplementary Characters</a>
+     * as they require a pair of {@code char}s to be represented.
+     * If you are needing to support full I18N of your applications
+     * consider using {@link #repeat(String, int)} instead.
+     * </p>
+     *
+     * @param stringBuilder  stringBuilder to fill
+     * @param ch  character to repeat
+     * @param repeat  number of times to repeat char, negative treated as zero
+     * @see #repeat(String, int)
+     */
+    public static void repeat(StringBuilder stringBuilder, final char ch, final int repeat) {

Review comment:
       Why is this API typed to a `StringBuilder` instead of a `Appendable`?




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] garydgregory commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

garydgregory commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452268209



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6234,8 +6239,34 @@ public static String repeat(final char ch, final int repeat) {
         return new String(buf);
     }
 
-    // Padding
-    //-----------------------------------------------------------------------
+    /**
+     * <p>Returns padding using the specified delimiter repeated
+     * to a given length.</p>
+     *
+     * <pre>
+     * StringUtils.repeat('e', 0)  = ""
+     * StringUtils.repeat('e', 3)  = "eee"
+     * StringUtils.repeat('e', -2) = ""
+     * </pre>
+     *
+     * <p>Note: this method does not support padding with
+     * <a href="http://www.unicode.org/glossary/#supplementary_character">Unicode Supplementary Characters</a>
+     * as they require a pair of {@code char}s to be represented.
+     * If you are needing to support full I18N of your applications
+     * consider using {@link #repeat(String, int)} instead.
+     * </p>
+     *
+     * @param stringBuilder  stringBuilder to fill
+     * @param ch  character to repeat
+     * @param repeat  number of times to repeat char, negative treated as zero
+     * @see #repeat(String, int)
+     */
+    public static void repeat(StringBuilder stringBuilder, final char ch, final int repeat) {

Review comment:
       Why is this API typed to a `StringBuilder` instead of a `Appendable`?
   Do we need an `AppendableUtils`?




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452272462



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6201,6 +6203,9 @@ public static String removeStartIgnoreCase(final String str, final String remove
         return str;
     }
 
+    // Padding

Review comment:
       @garydgregory
   Hi.
   I didn't ADD this seperator, but MOVE it to a better place.
   you can see it append in a weird place in the original codes.
   If you think it is better to delete it, then I'll be glad to delete it.
   I don't like ie either :)




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452277354



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6234,8 +6239,34 @@ public static String repeat(final char ch, final int repeat) {
         return new String(buf);
     }
 
-    // Padding
-    //-----------------------------------------------------------------------
+    /**
+     * <p>Returns padding using the specified delimiter repeated
+     * to a given length.</p>
+     *
+     * <pre>
+     * StringUtils.repeat('e', 0)  = ""
+     * StringUtils.repeat('e', 3)  = "eee"
+     * StringUtils.repeat('e', -2) = ""
+     * </pre>
+     *
+     * <p>Note: this method does not support padding with
+     * <a href="http://www.unicode.org/glossary/#supplementary_character">Unicode Supplementary Characters</a>
+     * as they require a pair of {@code char}s to be represented.
+     * If you are needing to support full I18N of your applications
+     * consider using {@link #repeat(String, int)} instead.
+     * </p>
+     *
+     * @param stringBuilder  stringBuilder to fill
+     * @param ch  character to repeat
+     * @param repeat  number of times to repeat char, negative treated as zero
+     * @see #repeat(String, int)
+     */
+    public static void repeat(StringBuilder stringBuilder, final char ch, final int repeat) {

Review comment:
       @garydgregory
   > Why is this API typed to a `StringBuilder` instead of a `Appendable`?
   Yeah you are right. will do this change.
   will aslo refine the javadoc of this function.
   
   > Do we need an `AppendableUtils`?
   Don't think so but I do think quite some of functions should be changed from build a string, and using it to build another string, and build another string, to build the whole string using StringBuilder, this one named `center` is a good example I think.
   I create this repeat function to reduce String creation.
   And I don't actually know if people really often use functions like this in real work.
   
   

##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6234,8 +6239,34 @@ public static String repeat(final char ch, final int repeat) {
         return new String(buf);
     }
 
-    // Padding
-    //-----------------------------------------------------------------------
+    /**
+     * <p>Returns padding using the specified delimiter repeated
+     * to a given length.</p>
+     *
+     * <pre>
+     * StringUtils.repeat('e', 0)  = ""
+     * StringUtils.repeat('e', 3)  = "eee"
+     * StringUtils.repeat('e', -2) = ""
+     * </pre>
+     *
+     * <p>Note: this method does not support padding with
+     * <a href="http://www.unicode.org/glossary/#supplementary_character">Unicode Supplementary Characters</a>
+     * as they require a pair of {@code char}s to be represented.
+     * If you are needing to support full I18N of your applications
+     * consider using {@link #repeat(String, int)} instead.
+     * </p>
+     *
+     * @param stringBuilder  stringBuilder to fill
+     * @param ch  character to repeat
+     * @param repeat  number of times to repeat char, negative treated as zero
+     * @see #repeat(String, int)
+     */
+    public static void repeat(StringBuilder stringBuilder, final char ch, final int repeat) {

Review comment:
       @garydgregory
   > Why is this API typed to a `StringBuilder` instead of a `Appendable`?
   
   Yeah you are right. will do this change.
   will aslo refine the javadoc of this function.
   
   > Do we need an `AppendableUtils`?
   
   Don't think so but I do think quite some of functions should be changed from build a string, and using it to build another string, and build another string, to build the whole string using StringBuilder, this one named `center` is a good example I think.
   I create this repeat function to reduce String creation.
   And I don't actually know if people really often use functions like this in real work.
   
   




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452279363



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6234,8 +6239,34 @@ public static String repeat(final char ch, final int repeat) {
         return new String(buf);
     }
 
-    // Padding
-    //-----------------------------------------------------------------------
+    /**
+     * <p>Returns padding using the specified delimiter repeated
+     * to a given length.</p>
+     *
+     * <pre>
+     * StringUtils.repeat('e', 0)  = ""
+     * StringUtils.repeat('e', 3)  = "eee"
+     * StringUtils.repeat('e', -2) = ""
+     * </pre>
+     *
+     * <p>Note: this method does not support padding with
+     * <a href="http://www.unicode.org/glossary/#supplementary_character">Unicode Supplementary Characters</a>
+     * as they require a pair of {@code char}s to be represented.
+     * If you are needing to support full I18N of your applications
+     * consider using {@link #repeat(String, int)} instead.
+     * </p>
+     *
+     * @param stringBuilder  stringBuilder to fill
+     * @param ch  character to repeat
+     * @param repeat  number of times to repeat char, negative treated as zero
+     * @see #repeat(String, int)
+     */
+    public static void repeat(StringBuilder stringBuilder, final char ch, final int repeat) {

Review comment:
       > Why is this API typed to a `StringBuilder` instead of a `Appendable`?
   
   @garydgregory Oh I guess I found out why I didn;t use Appendable when create this function...
   Appendable.append can throw IOException, and we must catch it.
   That might cause performance issue, so I just used StringBuilder instead.
   
   




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452279363



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6234,8 +6239,34 @@ public static String repeat(final char ch, final int repeat) {
         return new String(buf);
     }
 
-    // Padding
-    //-----------------------------------------------------------------------
+    /**
+     * <p>Returns padding using the specified delimiter repeated
+     * to a given length.</p>
+     *
+     * <pre>
+     * StringUtils.repeat('e', 0)  = ""
+     * StringUtils.repeat('e', 3)  = "eee"
+     * StringUtils.repeat('e', -2) = ""
+     * </pre>
+     *
+     * <p>Note: this method does not support padding with
+     * <a href="http://www.unicode.org/glossary/#supplementary_character">Unicode Supplementary Characters</a>
+     * as they require a pair of {@code char}s to be represented.
+     * If you are needing to support full I18N of your applications
+     * consider using {@link #repeat(String, int)} instead.
+     * </p>
+     *
+     * @param stringBuilder  stringBuilder to fill
+     * @param ch  character to repeat
+     * @param repeat  number of times to repeat char, negative treated as zero
+     * @see #repeat(String, int)
+     */
+    public static void repeat(StringBuilder stringBuilder, final char ch, final int repeat) {

Review comment:
       > Why is this API typed to a `StringBuilder` instead of a `Appendable`?
   
   @garydgregory Oh I guess I found out why I didn;t use Appendable when create this function...
   Appendable.append can throw IOException, and we must catch it.
   That might cause performance issue, so I just used StringBuilder instead.
   Maybe it is better to make this function private instead?
   
   




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] XenoAmess commented on a change in pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

XenoAmess commented on a change in pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#discussion_r452285077



##########
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##########
@@ -6201,6 +6203,9 @@ public static String removeStartIgnoreCase(final String str, final String remove
         return str;
     }
 
+    // Padding

Review comment:
       @garydgregory resolved as deleted.




----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] coveralls edited a comment on pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

coveralls edited a comment on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-651297946


   
   [![Coverage Status](https://coveralls.io/builds/31969016/badge)](https://coveralls.io/builds/31969016)
   
   Coverage decreased (-0.08%) to 94.6% when pulling **3cba9db920b57c38f5f4b53cdb0d8d5f8473af7d on XenoAmess:refine_center** into **32a9a350c67f1418784fff266d7e7881ca6346ad on apache:master**.
   


----------------------------------------------------------------
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.

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


Reply | Threaded
Open this post in threaded view
|

[GitHub] [commons-lang] coveralls edited a comment on pull request #577: [LANG-1588] performance refine for StringUtils.center

GitBox
In reply to this post by GitBox

coveralls edited a comment on pull request #577:
URL: https://github.com/apache/commons-lang/pull/577#issuecomment-651297946


   
   [![Coverage Status](https://coveralls.io/builds/31969166/badge)](https://coveralls.io/builds/31969166)
   
   Coverage decreased (-0.08%) to 94.6% when pulling **3cba9db920b57c38f5f4b53cdb0d8d5f8473af7d on XenoAmess:refine_center** into **32a9a350c67f1418784fff266d7e7881ca6346ad on apache:master**.
   


----------------------------------------------------------------
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.

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