[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/334 Thanks @PascalSchumacher for confirming it's actually backward compatible (#TodayILearned - and hopefully will remember it). @ingvarc , I agree on @aaabramov regarding the `+1`. When I read it, I first thought that was a `+= 1`, and then re-read it and understood his concern. Might be worth simplifying it. If you'd like to keep the commits simpler (which is great initiative, thanks!) you could try rebasing commits and push+force to your branch. Good points @stokito. Not sure about branch prediction after the spectre bug. I think one of the mitigations involved disabling branch prediction. Though I think micro-benchmarking isn't priority in lang. >Here developer sees a "happy path" first and only then he or she sees exception situation handling. This is more natural to understand. As I remember this style was mentioned in Complete Code book. Code Complete is one of my favourite books. For me, that code could be: ```java private static void addProcessor(final String key, final Processor processor) { if (ARCH_TO_PROCESSOR.containsKey(key)) { throw new IllegalStateException("Key " + key + " already exists in processor map"); } ARCH_TO_PROCESSOR.put(key, processor); } ``` I think maybe developers would read the first `if` as a validation block. Then the normal behaviour, without the else (I think omitting the else in cases like this is also in Code Complete... or Pragmatic Programmer? Can't recall). I think the other changes like removing class name, inverting some `if` conditions, and removing the exceptions not thrown look good, and the code readability keeps OK. Great contribution @ingvarc , thanks! ---
[GitHub] commons-lang issue #318: clean code
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/318 @JiangYongKang, thanks for your contribution. Even though the behaviour is the same, in my opinion, the other code is easier to read, and fewer code doesn't necessarily mean better or more clean code. Checking if a number if even or odd, for example, can be done with bit operation, or with a `if/else`. While the former is shorter, not all developers immediately understand it, nor know how to maintain/change it. ---
[GitHub] commons-lang issue #318: clean code
Github user JiangYongKang commented on the issue: https://github.com/apache/commons-lang/pull/318 @stokito thinks ---
[jira] [Commented] (LANG-967) Code should never catch and ignore all Throwables
[ https://issues.apache.org/jira/browse/LANG-967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16521312#comment-16521312 ] Sergey Ponomarev commented on LANG-967: --- As it was mentioned you shouldn't catch Throwable until you understood what are you doing and even you you need then catch Error explicitly and even better to catch specific error. For example if you received OutOfMemoryError then it can be so low memory that you even can't write to logs or wrap OOM into another exception. But you can try to recover an application by clearing some in-memory caches or temporary stop processing. See Apache Derby [LowMemory|http://people.apache.org/~kristwaa/jacoco/org.apache.derby.iapi.services.memory/LowMemory.java.html] class and it's usage as example of recovery. The mentioned Tomcat's ExceptionUtils.handleThrowable() looks quite weird. It rethrowth VirtualMachineError and ThreadDeath while all others are silently eaten. Maybe Tomcat's developers know what they are doing but this is definitely very custom logic. I think we have nothing to do here and the ticket can be closed > Code should never catch and ignore all Throwables > - > > Key: LANG-967 > URL: https://issues.apache.org/jira/browse/LANG-967 > Project: Commons Lang > Issue Type: New Feature > Components: lang.exception.* >Reporter: Sebb >Priority: Major > Fix For: Patch Needed > > > Code should never catch and ignore all Throwables - ThreadDeath and > VirtualMachineError must be rethrown. > It would be useful to provide a method to enforce this behaviour. > For example,. as is done by the Tomcat method \[1] with source here \[2] > Sample usage: > {code} > try { > reader.close(); > } catch (Throwable u) { > ExceptionUtils.handleThrowable(u); > } > {code} > \[1] org.apache.tomcat.util.ExceptionUtils#handleThrowable() > \[2] > https://svn.apache.org/repos/asf/tomcat/trunk/java/org/apache/tomcat/util/ExceptionUtils.java -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Updated] (LANG-1400) StringUtils: Add method for masking strings
[ https://issues.apache.org/jira/browse/LANG-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Sergey Ponomarev updated LANG-1400: --- Description: It would be great to have a function which allows to mask the original str by replacing it's characters with the specified character-mask. For eaxmple to mask credit card: {code} mask("3566002020360505", 4, 4, '*') = "35660505" {code} Thus the number was hidden by asterisks while first and last four digits are unmasked and seen. Common use case is to hide sensitive information from logs, by using it in toString() of classes or in inputs to log calls. I think this is "must have" functionality for Commons Lang library because from my experience in almost all bit projects what I saw was their home grown masking function. I think this is very important also because this is required for masking credit card numbers by PCI compliance. Also new GDPR rules requires to hide personal info as much as possible so masking of First and Last names now is required by laws. To make the world safer place the utility for masking should be already existing in platform to avoid situations when developer think "this is better to mask but I don't have enough time now so let's do this latter" until leak happens. IMHO this should be implemented even in `String` class itself. >From my experience what I saw was usually few masking usages and styles: 1. masking of passwords and names: only first and last symbols are shown, mask char is `*` 2. masking of credit cards: only first and last 4 or 6 symbols are shown, mask char is `*`. 3. credit card number shortest masking of last symbols i.e. `mask("4242424242424242") == " *4242"` but it's not so often used, I just wanted to mention. 4. not masking but showing a length just so see that value was passed. This can be easily achieved by usual `String.legth()` method. There is already some pull request [https://github.com/apache/commons-lang/pull/332] but I decided to create the ticket because we also need to support masking for ToStringBuilder and I would like to propose [PR with my own implementation of mask() function|https://github.com/apache/commons-lang/pull/335]. If you accept my PR then I'll send another one with a new annotation @ToStringMasked in accordance to [@ToStringExclude|https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/ToStringExclude.html] which will mark that fields should be masked. Possible usage I see like: {code} @ToStringMasked(unmaskedStart = 4, unmaskedEnd = 4) String creditCardNumber; @ToStringMasked(unmaskedStart = 1, unmaskedEnd = 1) String password; {code} was: It would be great to have a function which allows to mask the original str by replacing it's characters with the specified character-mask. For eaxmple to mask credit card: {code} mask("3566002020360505", 4, 4, '*') = "35660505" {code} Thus the number was hidden by asterisks while first and last four digits are unmasked and seen. Common use case is to hide sensitive information from logs, by using it in toString() of classes or in inputs to log calls. I think this is "must have" functionality for Commons Lang library because from my experience in almost all bit projects what I saw was their home grown masking function. I think this is very important also because this is required for masking credit card numbers by PCI compliance. Also new GDPR rules requires to hide personal info as much as possible so masking of First and Last names now is required by laws. To make the world safer place the utility for masking should be already existing in platform to avoid situations when developer think "this is better to mask but I don't have enough time now so let's do this latter" until leak happens. IMHO this should be implemented even in `String` class itself. >From my experience what I saw was usually few masking usages and styles: 1. masking of passwords and names: only first and last symbols are shown, mask char is `*` 2. masking of credit cards: only first and last 4 or 6 symbols are shown, mask char is `*`. 3. credit card number shortest masking of last symbols i.e. `mask("4242424242424242") == " *4242"` but it's not so often used, I just wanted to mention. 4. not masking but showing a length just so see that value was passed. This can be easily achieved by usual `String.legth()` method. There is already some pull request [https://github.com/apache/commons-lang/pull/332] but I decided to create the ticket because we also need to support masking for ToStringBuilder and I would like to propose [PR with my own implementation of mask() function|https://github.com/apache/commons-lang/pull/335]. If you accept my PR then I'll send another one with a new annotation @ToStringMasked in accordance to [@ToStringExclude|https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/l
[jira] [Updated] (LANG-1400) StringUtils: Add method for masking strings
[ https://issues.apache.org/jira/browse/LANG-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Sergey Ponomarev updated LANG-1400: --- Description: It would be great to have a function which allows to mask the original str by replacing it's characters with the specified character-mask. For eaxmple to mask credit card: {code} mask("3566002020360505", 4, 4, '*') = "35660505" {code} Thus the number was hidden by asterisks while first and last four digits are unmasked and seen. Common use case is to hide sensitive information from logs, by using it in toString() of classes or in inputs to log calls. I think this is "must have" functionality for Commons Lang library because from my experience in almost all bit projects what I saw was their home grown masking function. I think this is very important also because this is required for masking credit card numbers by PCI compliance. Also new GDPR rules requires to hide personal info as much as possible so masking of First and Last names now is required by laws. To make the world safer place the utility for masking should be already existing in platform to avoid situations when developer think "this is better to mask but I don't have enough time now so let's do this latter" until leak happens. IMHO this should be implemented even in `String` class itself. >From my experience what I saw was usually few masking usages and styles: 1. masking of passwords and names: only first and last symbols are shown, mask char is `*` 2. masking of credit cards: only first and last 4 or 6 symbols are shown, mask char is `*`. 3. credit card number shortest masking of last symbols i.e. `mask("4242424242424242") == " *4242"` but it's not so often used, I just wanted to mention. 4. not masking but showing a length just so see that value was passed. This can be easily achieved by usual `String.legth()` method. There is already some pull request [https://github.com/apache/commons-lang/pull/332] but I decided to create the ticket because we also need to support masking for ToStringBuilder and I would like to propose [PR with my own implementation of mask() function|https://github.com/apache/commons-lang/pull/335]. If you accept my PR then I'll send another one with a new annotation @ToStringMasked in accordance to [@ToStringExclude|https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/ToStringExclude.html] which will mark that fields should be masked. Possible usage I see like: {code} @ToStringMasked(unmaskedStart = 4, unmaskedEnd = 4) String creditCardNumber; {code} was: It would be great to have a function which allows to mask the original str by replacing it's characters with the specified character-mask. For eaxmple to mask credit card: {code} mask("3566002020360505", 4, 4, '*') = "35660505" {code} Thus the number was hidden by asterisks while first and last four digits are unmasked and seen. Common use case is to hide sensitive information from logs, by using it in toString() of classes or in inputs to log calls. I think this is "must have" functionality for Commons Lang library because from my experience in almost all bit projects what I saw was their home grown masking function. I think this is very important also because this is required for masking credit card numbers by PCI compliance. Also new GDPR rules requires to hide personal info as much as possible so masking of First and Last names now is required by laws. To make the world safer place the utility for masking should be already existing in platform to avoid situations when developer think "this is better to mask but I don't have enough time now so let's do this latter" until leak happens. IMHO this should be implemented even in `String` class itself. >From my experience what I saw was usually few masking usages and styles: 1. masking of passwords and names: only first and last symbols are shown, mask char is `*` 2. masking of credit cards: only first and last 4 or 6 symbols are shown, mask char is `*`. 3. credit card number shortest masking of last symbols i.e. `mask("4242424242424242") == " *4242"` but it's not so often used, I just wanted to mention. 4. not masking but showing a length just so see that value was passed. This can be easily achieved by usual `String.legth()` method. There is already some pull request [https://github.com/apache/commons-lang/pull/332] but I decided to create the ticket because we also need to support masking for @ToString annotation and I would like to propose [PR with my own implementation of mask() function|https://github.com/apache/commons-lang/pull/335]. > StringUtils: Add method for masking strings > --- > > Key: LANG-1400 > URL: https://issues.apache.org/jira/browse/LANG-1400 > Project: Commons Lang > Issue Type:
[jira] [Commented] (LANG-1400) StringUtils: Add method for masking strings
[ https://issues.apache.org/jira/browse/LANG-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16521300#comment-16521300 ] ASF GitHub Bot commented on LANG-1400: -- Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/332 Hi @greenman18523 thank for your contribution - from my experience this is "must have" functionality for Commons Lang library because in almost all bit projects what I saw was their home grown masking function. But what I would like to propose is to simplify the api and use one function `mask()` instead of two `maskStart()` and `maskEnd()`. This will simplify code of the functions especially both of them are quite similar. I created a pull request with my alternative implementation #335 Also I created a JIRA ticket https://issues.apache.org/jira/browse/LANG-1400 Please give your feedback, Thank you > StringUtils: Add method for masking strings > --- > > Key: LANG-1400 > URL: https://issues.apache.org/jira/browse/LANG-1400 > Project: Commons Lang > Issue Type: Improvement > Components: lang.* >Reporter: Sergey Ponomarev >Priority: Minor > > It would be great to have a function which allows to mask the original str by > replacing it's > characters with the specified character-mask. For eaxmple to mask credit card: > {code} > mask("3566002020360505", 4, 4, '*') = "35660505" > {code} > Thus the number was hidden by asterisks while first and last four digits are > unmasked and seen. > Common use case is to hide sensitive information from logs, by using it in > toString() of classes or in inputs to log calls. > I think this is "must have" functionality for Commons Lang library because > from my experience in almost all bit projects what I saw was their home grown > masking function. > I think this is very important also because this is required for masking > credit card numbers by PCI compliance. Also new GDPR rules requires to hide > personal info as much as possible so masking of First and Last names now is > required by laws. > To make the world safer place the utility for masking should be already > existing in platform to avoid situations when developer think "this is > better to mask but I don't have enough time now so let's do this latter" > until leak happens. > IMHO this should be implemented even in `String` class itself. > From my experience what I saw was usually few masking usages and styles: > 1. masking of passwords and names: only first and last symbols are shown, > mask char is `*` > 2. masking of credit cards: only first and last 4 or 6 symbols are shown, > mask char is `*`. > 3. credit card number shortest masking of last symbols i.e. > `mask("4242424242424242") == " *4242"` but it's not so often used, I just > wanted to mention. > 4. not masking but showing a length just so see that value was passed. This > can be easily achieved by usual `String.legth()` method. > There is already some pull request > [https://github.com/apache/commons-lang/pull/332] but I decided to create the > ticket because we also need to support masking for @ToString annotation and I > would like to propose [PR with my own implementation of mask() > function|https://github.com/apache/commons-lang/pull/335]. > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] commons-lang issue #332: Add methods allowing masking of Strings
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/332 Hi @greenman18523 thank for your contribution - from my experience this is "must have" functionality for Commons Lang library because in almost all bit projects what I saw was their home grown masking function. But what I would like to propose is to simplify the api and use one function `mask()` instead of two `maskStart()` and `maskEnd()`. This will simplify code of the functions especially both of them are quite similar. I created a pull request with my alternative implementation #335 Also I created a JIRA ticket https://issues.apache.org/jira/browse/LANG-1400 Please give your feedback, Thank you ---
[jira] [Updated] (LANG-1400) StringUtils: Add method for masking strings
[ https://issues.apache.org/jira/browse/LANG-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Sergey Ponomarev updated LANG-1400: --- Description: It would be great to have a function which allows to mask the original str by replacing it's characters with the specified character-mask. For eaxmple to mask credit card: {code} mask("3566002020360505", 4, 4, '*') = "35660505" {code} Thus the number was hidden by asterisks while first and last four digits are unmasked and seen. Common use case is to hide sensitive information from logs, by using it in toString() of classes or in inputs to log calls. I think this is "must have" functionality for Commons Lang library because from my experience in almost all bit projects what I saw was their home grown masking function. I think this is very important also because this is required for masking credit card numbers by PCI compliance. Also new GDPR rules requires to hide personal info as much as possible so masking of First and Last names now is required by laws. To make the world safer place the utility for masking should be already existing in platform to avoid situations when developer think "this is better to mask but I don't have enough time now so let's do this latter" until leak happens. IMHO this should be implemented even in `String` class itself. >From my experience what I saw was usually few masking usages and styles: 1. masking of passwords and names: only first and last symbols are shown, mask char is `*` 2. masking of credit cards: only first and last 4 or 6 symbols are shown, mask char is `*`. 3. credit card number shortest masking of last symbols i.e. `mask("4242424242424242") == " *4242"` but it's not so often used, I just wanted to mention. 4. not masking but showing a length just so see that value was passed. This can be easily achieved by usual `String.legth()` method. There is already some pull request [https://github.com/apache/commons-lang/pull/332] but I decided to create the ticket because we also need to support masking for @ToString annotation and I would like to propose [PR with my own implementation of mask() function|https://github.com/apache/commons-lang/pull/335]. was: It would be great to have a function which allows to mask the original str by replacing it's characters with the specified character-mask. For eaxmple to mask credit card: {code} mask("3566002020360505", 4, 4, '*') = "35660505" {code} Thus the number was hidden by asterisks while first and last four digits are unmasked and seen. Common use case is to hide sensitive information from logs, by using it in toString() of classes or in inputs to log calls. I think this is "must have" functionality for Commons Lang library because from my experience in almost all bit projects what I saw was their home grown masking function. I think this is very important also because this is required for masking credit card numbers by PCI compliance. Also new GDPR rules requires to hide personal info as much as possible so masking of First and Last names now is required by laws. To make the world safer place the utility for masking should be already existing in platform to avoid situations when developer think "this is better to mask but I don't have enough time now so let's do this latter" until leak happens. IMHO this should be implemented even in `String` class itself. >From my experience what I saw was usually few masking usages and styles: 1. masking of passwords and names: only first and last symbols are shown, mask char is `*` 2. masking of credit cards: only first and last 4 or 6 symbols are shown, mask char is `*`. 3. credit card number shortest masking of last symbols i.e. `mask("4242424242424242") == " *4242"` but it's not so often used, I just wanted to mention. 4. not masking but showing a length just so see that value was passed. This can be easily achieved by usual `String.legth()` method. There is already some pull request [https://github.com/apache/commons-lang/pull/332] but I decided to create the ticket because we also need to support masking for @ToString annotation and I would like to propose my own implementation of mask() function. > StringUtils: Add method for masking strings > --- > > Key: LANG-1400 > URL: https://issues.apache.org/jira/browse/LANG-1400 > Project: Commons Lang > Issue Type: Improvement > Components: lang.* >Reporter: Sergey Ponomarev >Priority: Minor > > It would be great to have a function which allows to mask the original str by > replacing it's > characters with the specified character-mask. For eaxmple to mask credit card: > {code} > mask("3566002020360505", 4, 4, '*') = "35660505" > {code} > Thus the number was hidden by asterisks while first and last four digits are >
[jira] [Commented] (LANG-1400) StringUtils: Add method for masking strings
[ https://issues.apache.org/jira/browse/LANG-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16521296#comment-16521296 ] ASF GitHub Bot commented on LANG-1400: -- GitHub user stokito opened a pull request: https://github.com/apache/commons-lang/pull/335 LANG-1400: Add StringUtils.mask() function My alternative implementation for #332 See https://issues.apache.org/jira/browse/LANG-1400 for details You can merge this pull request into a Git repository by running: $ git pull https://github.com/stokito/commons-lang LANG-1400 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/commons-lang/pull/335.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #335 commit 001b769fab526f2034c155dab422dfe10bf48cff Author: Sergey Ponomarev Date: 2018-06-23T22:47:33Z LANG-1400: Add StringUtils.mask() function > StringUtils: Add method for masking strings > --- > > Key: LANG-1400 > URL: https://issues.apache.org/jira/browse/LANG-1400 > Project: Commons Lang > Issue Type: Improvement > Components: lang.* >Reporter: Sergey Ponomarev >Priority: Minor > > It would be great to have a function which allows to mask the original str by > replacing it's > characters with the specified character-mask. For eaxmple to mask credit card: > {code} > mask("3566002020360505", 4, 4, '*') = "35660505" > {code} > Thus the number was hidden by asterisks while first and last four digits are > unmasked and seen. > Common use case is to hide sensitive information from logs, by using it in > toString() of classes or in inputs to log calls. > I think this is "must have" functionality for Commons Lang library because > from my experience in almost all bit projects what I saw was their home grown > masking function. > I think this is very important also because this is required for masking > credit card numbers by PCI compliance. Also new GDPR rules requires to hide > personal info as much as possible so masking of First and Last names now is > required by laws. > To make the world safer place the utility for masking should be already > existing in platform to avoid situations when developer think "this is > better to mask but I don't have enough time now so let's do this latter" > until leak happens. > IMHO this should be implemented even in `String` class itself. > From my experience what I saw was usually few masking usages and styles: > 1. masking of passwords and names: only first and last symbols are shown, > mask char is `*` > 2. masking of credit cards: only first and last 4 or 6 symbols are shown, > mask char is `*`. > 3. credit card number shortest masking of last symbols i.e. > `mask("4242424242424242") == " *4242"` but it's not so often used, I just > wanted to mention. > 4. not masking but showing a length just so see that value was passed. This > can be easily achieved by usual `String.legth()` method. > There is already some pull request > [https://github.com/apache/commons-lang/pull/332] but I decided to create the > ticket because we also need to support masking for @ToString annotation and I > would like to propose my own implementation of mask() function. > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] commons-lang pull request #335: LANG-1400: Add StringUtils.mask() function
GitHub user stokito opened a pull request: https://github.com/apache/commons-lang/pull/335 LANG-1400: Add StringUtils.mask() function My alternative implementation for #332 See https://issues.apache.org/jira/browse/LANG-1400 for details You can merge this pull request into a Git repository by running: $ git pull https://github.com/stokito/commons-lang LANG-1400 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/commons-lang/pull/335.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #335 commit 001b769fab526f2034c155dab422dfe10bf48cff Author: Sergey Ponomarev Date: 2018-06-23T22:47:33Z LANG-1400: Add StringUtils.mask() function ---
[jira] [Created] (LANG-1400) StringUtils: Add method for masking strings
Sergey Ponomarev created LANG-1400: -- Summary: StringUtils: Add method for masking strings Key: LANG-1400 URL: https://issues.apache.org/jira/browse/LANG-1400 Project: Commons Lang Issue Type: Improvement Components: lang.* Reporter: Sergey Ponomarev It would be great to have a function which allows to mask the original str by replacing it's characters with the specified character-mask. For eaxmple to mask credit card: {code} mask("3566002020360505", 4, 4, '*') = "35660505" {code} Thus the number was hidden by asterisks while first and last four digits are unmasked and seen. Common use case is to hide sensitive information from logs, by using it in toString() of classes or in inputs to log calls. I think this is "must have" functionality for Commons Lang library because from my experience in almost all bit projects what I saw was their home grown masking function. I think this is very important also because this is required for masking credit card numbers by PCI compliance. Also new GDPR rules requires to hide personal info as much as possible so masking of First and Last names now is required by laws. To make the world safer place the utility for masking should be already existing in platform to avoid situations when developer think "this is better to mask but I don't have enough time now so let's do this latter" until leak happens. IMHO this should be implemented even in `String` class itself. >From my experience what I saw was usually few masking usages and styles: 1. masking of passwords and names: only first and last symbols are shown, mask char is `*` 2. masking of credit cards: only first and last 4 or 6 symbols are shown, mask char is `*`. 3. credit card number shortest masking of last symbols i.e. `mask("4242424242424242") == " *4242"` but it's not so often used, I just wanted to mention. 4. not masking but showing a length just so see that value was passed. This can be easily achieved by usual `String.legth()` method. There is already some pull request [https://github.com/apache/commons-lang/pull/332] but I decided to create the ticket because we also need to support masking for @ToString annotation and I would like to propose my own implementation of mask() function. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] commons-lang issue #318: clean code
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/318 Changes are ok ---
[GitHub] commons-lang issue #278: Lang-1345 Enhance non-empty strings
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/278 For me is not clear importance of the new methods. I can't remember situations when I needed for something like this. And even if I need something like this I'll write the function myself and I'll not even think about to search something in Commons Lang library. Time spent to looking for the method will be ten time more then write myself. Maybe commons lang itself uses the function internally then it can make sense. It would be great if you can give us an examples of usage and how they are important. ---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/334 I checked and everything is ok in terms of backward compatibility. But the commit 7f8571a506c7081bae0cf27bd93295e0344160bf "flips the order of conditional expressions and 'if' statements whose conditions were negated." may be not so good in terms of code readability. First of all, there is always some natural flow when we expect that something is more likely happens. Let's take a look on the method `addProcessor()`: ```java private static void addProcessor(final String key, final Processor processor) { if (!ARCH_TO_PROCESSOR.containsKey(key)) { ARCH_TO_PROCESSOR.put(key, processor); } else { final String msg = "Key " + key + " already exists in processor map"; throw new IllegalStateException(msg); } } ``` Here developer sees a "happy path" first and only then he or she sees exception situation handling. This is more natural to understand. As I remember this style was mentioned in Complete Code book. Also this style has slightly better performance because in most situations processor will go forward to prefetched instructions or even it can be already executed by [branch prediction](https://en.wikipedia.org/wiki/Branch_predictor). Meanwhile removing of negation usually doesn't have any performance impact because processors have already negated instructions. I thinks it's ok to merge this commit but I just wan't you to know that sometimes it is better to make unnecessary negation. ---