You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by ingvarc <gi...@git.apache.org> on 2018/06/20 06:42:43 UTC
[GitHub] commons-lang pull request #334: Code refactoring and cleaning
GitHub user ingvarc opened a pull request:
https://github.com/apache/commons-lang/pull/334
Code refactoring and cleaning
- **removes unchecked exceptions declared in 'throws' clause**;
Effective Java: "Use the Javadoc @throws tag to document each exception that a method
can throw, but do not use the throws keyword on unchecked exceptions." (3e, item 74).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ingvarc/commons-lang master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-lang/pull/334.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 #334
----
commit e767af7e7eb8ff7724d5f72709ee4bb7a72d2284
Author: Igor Curdvanovschi <in...@...>
Date: 2018-06-20T06:00:24Z
removes unchecked exceptions declared in 'throws' clause
----
---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Posted by PascalSchumacher <gi...@git.apache.org>.
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
Posted by kinow <gi...@git.apache.org>.
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
Posted by ingvarc <gi...@git.apache.org>.
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
Posted by PascalSchumacher <gi...@git.apache.org>.
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
Posted by kinow <gi...@git.apache.org>.
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
Posted by ingvarc <gi...@git.apache.org>.
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 pull request #334: Code refactoring and cleaning
Posted by ingvarc <gi...@git.apache.org>.
Github user ingvarc commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/334#discussion_r197346902
--- Diff: src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java ---
@@ -600,10 +600,10 @@ public CompareToBuilder append(final boolean lhs, final boolean rhs) {
if (lhs == rhs) {
return this;
}
- if (!lhs) {
- comparison = -1;
- } else {
+ if (lhs) {
comparison = +1;
--- End diff --
Actually, the plus sign is redundant here. I've tried to link commits with only one type of changes and therefore haven't removed it.
---
[GitHub] commons-lang pull request #334: Code refactoring and cleaning
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/commons-lang/pull/334
---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Posted by stokito <gi...@git.apache.org>.
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
Posted by coveralls <gi...@git.apache.org>.
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**.
---
[GitHub] commons-lang pull request #334: Code refactoring and cleaning
Posted by aaabramov <gi...@git.apache.org>.
Github user aaabramov commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/334#discussion_r197231903
--- Diff: src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java ---
@@ -600,10 +600,10 @@ public CompareToBuilder append(final boolean lhs, final boolean rhs) {
if (lhs == rhs) {
return this;
}
- if (!lhs) {
- comparison = -1;
- } else {
+ if (lhs) {
comparison = +1;
--- End diff --
Why do we need `+` here?
---