[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/334 Thanks! ð ---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user ingvarc commented on the issue: https://github.com/apache/commons-lang/pull/334 @stokito Thanks for pointing that out, I'll keep it in mind in future. @kinow I like the idea of a validation block and the normal behaviour. It seems to me that in this case it is more readable and clear. I'll rebase commits and make force push asap. @kinow, @PascalSchumacher, @aaabramov ans @stokito Since it's one of my first open-source contributions I really appreciate your observations and comments. ---
[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 #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.
---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/334 @kinow Changes to throws declaration are binary compatible (see: https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.21). Removing checked exceptions from a throws declaration is source incompatible, but removing unchecked exceptions is source compatible. This confirmed by this pull request passing the clirr check (see: https://travis-ci.org/apache/commons-lang/jobs/394527557#L3097) ---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user ingvarc commented on the issue: https://github.com/apache/commons-lang/pull/334 @kinow Since the pull request aims to refactor and to clean the code I think it makes sense. Speaking of unchecked exceptions in 'throws' clause they are backward compatible since the developers don't need either catch or declare them. I may have missed something in terms of backward compatibility and if you don't you mind pointing it out to me I would appreciate it. ---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/334 I'm fine either way (I believe even some classes in the JDK have some [unchecked exceptions](http://hg.openjdk.java.net/jdk7/jdk7/jdk/file/9b8c96f96a0f/src/share/classes/java/util/LinkedList.java) declared?) But I suspect this change is not backward compatible. Can't locate that documentation from Oracle explaining what's a backward change or not, but quite sure this one is. ---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/334 [](https://coveralls.io/builds/17585970) Coverage increased (+0.01%) to 95.241% when pulling **e767af7e7eb8ff7724d5f72709ee4bb7a72d2284 on ingvarc:master** into **8e8b8e05e4eb9aa009444c2fea3552d28b57aa98 on apache:master**. ---
