You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/06/21 08:28:45 UTC
[GitHub] [fineract] percyashu opened a new pull request #1099: FINERACT-822 Enforce UndefinedEquals check
percyashu opened a new pull request #1099:
URL: https://github.com/apache/fineract/pull/1099
FINERACT-822
https://errorprone.info/bugpattern/UndefinedEquals
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r444145876
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java
##########
@@ -100,8 +100,9 @@ public boolean equals(Object obj) {
}
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId) && Objects.equals(other.glAccountId, glAccountId)
- && Objects.equals(other.amount, amount) && Objects.equals(other.entryDate, entryDate)
- && Objects.equals(other.transactionDate, transactionDate) && Objects.equals(other.closingBalance, closingBalance);
+ && Objects.equals(other.amount, amount) && other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE
Review comment:
Error Prone kept complaining java.util.Date equals() doesn't seem to work as expected. This is because from [this](https://docs.oracle.com/javase/6/docs/api/java/util/Date.html#equals(java.lang.Object)) two Date objects are equal if and only if the getTime method returns the same long value for both.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1099:
URL: https://github.com/apache/fineract/pull/1099
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r444493839
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java
##########
@@ -100,8 +100,9 @@ public boolean equals(Object obj) {
}
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId) && Objects.equals(other.glAccountId, glAccountId)
- && Objects.equals(other.amount, amount) && Objects.equals(other.entryDate, entryDate)
- && Objects.equals(other.transactionDate, transactionDate) && Objects.equals(other.closingBalance, closingBalance);
+ && Objects.equals(other.amount, amount) && other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE
+ : Boolean.FALSE && other.transactionDate.compareTo(transactionDate) == 0 ? Boolean.TRUE
Review comment:
@percyashu what @thesmallstar is pointing out here, and I agree with, is that `? Boolean.TRUE: Boolean.FALSE` is not required - as he said, just `other.entryDate.compareTo(entryDate) == 0` yields exactly the same value. The difference between primitive VS non-primitive is hidden by "auto boxing". Do you want to raise a follow-up PR to clean this up, or ignore this?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r444034118
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java
##########
@@ -100,8 +100,9 @@ public boolean equals(Object obj) {
}
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId) && Objects.equals(other.glAccountId, glAccountId)
- && Objects.equals(other.amount, amount) && Objects.equals(other.entryDate, entryDate)
- && Objects.equals(other.transactionDate, transactionDate) && Objects.equals(other.closingBalance, closingBalance);
+ && Objects.equals(other.amount, amount) && other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE
Review comment:
@percyashu I don't understand this... given that `entryDate` and `transactionDate` are (both) `java.util.Date`, why is `Objects.equals()` not OK? That does not match what https://errorprone.info/bugpattern/UndefinedEquals seems to be about... what am I missing?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r443744286
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/shareaccounts/domain/ShareAccount.java
##########
@@ -224,7 +224,7 @@ public boolean setSubmittedDate(final Date submittedDate) {
public boolean setApprovedDate(final Date approvedDate) {
boolean toReturn = false;
- if (!this.approvedDate.equals(approvedDate)) {
+ if (this.approvedDate.compareTo(approvedDate) == 0 ? Boolean.FALSE : Boolean.TRUE) {
this.approvedDate = approvedDate;
Review comment:
very very minor suggestion, can we not keep it just !=0 (this makes it little confusing IMO).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r443741923
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java
##########
@@ -100,8 +100,9 @@ public boolean equals(Object obj) {
}
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId) && Objects.equals(other.glAccountId, glAccountId)
- && Objects.equals(other.amount, amount) && Objects.equals(other.entryDate, entryDate)
- && Objects.equals(other.transactionDate, transactionDate) && Objects.equals(other.closingBalance, closingBalance);
+ && Objects.equals(other.amount, amount) && other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE
+ : Boolean.FALSE && other.transactionDate.compareTo(transactionDate) == 0 ? Boolean.TRUE
Review comment:
Just curious about this:
How is `other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE: Boolean.FALSE`
dIfferent from just `other.entryDate.compareTo(entryDate) == 0 ` (In terms of end result) of the boolean?
Is it because .equals returns Boolean.TRUE and not primitive data-type boolean?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r444152233
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java
##########
@@ -100,8 +100,9 @@ public boolean equals(Object obj) {
}
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId) && Objects.equals(other.glAccountId, glAccountId)
- && Objects.equals(other.amount, amount) && Objects.equals(other.entryDate, entryDate)
- && Objects.equals(other.transactionDate, transactionDate) && Objects.equals(other.closingBalance, closingBalance);
+ && Objects.equals(other.amount, amount) && other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE
+ : Boolean.FALSE && other.transactionDate.compareTo(transactionDate) == 0 ? Boolean.TRUE
Review comment:
This is just like a Java If statement where other.entryDate.compareTo(entryDate) == 0 is the condition, the first value Boolean.TRUE is return if the condition is true and the second value Boolean.FALSE if it is false. Could have used an If statement but this is shorter. See [this](https://www.baeldung.com/java-ternary-operator)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
percyashu commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r443937448
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java
##########
@@ -100,8 +100,9 @@ public boolean equals(Object obj) {
}
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId) && Objects.equals(other.glAccountId, glAccountId)
- && Objects.equals(other.amount, amount) && Objects.equals(other.entryDate, entryDate)
- && Objects.equals(other.transactionDate, transactionDate) && Objects.equals(other.closingBalance, closingBalance);
+ && Objects.equals(other.amount, amount) && other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE
+ : Boolean.FALSE && other.transactionDate.compareTo(transactionDate) == 0 ? Boolean.TRUE
Review comment:
compareTo returns an int. If values to be compared are equal, the value returned is zero.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/shareaccounts/domain/ShareAccount.java
##########
@@ -224,7 +224,7 @@ public boolean setSubmittedDate(final Date submittedDate) {
public boolean setApprovedDate(final Date approvedDate) {
boolean toReturn = false;
- if (!this.approvedDate.equals(approvedDate)) {
+ if (this.approvedDate.compareTo(approvedDate) == 0 ? Boolean.FALSE : Boolean.TRUE) {
this.approvedDate = approvedDate;
Review comment:
Yeah, but I think this way is clearer.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] percyashu commented on pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
percyashu commented on pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#issuecomment-647109766
@awasum @vorburger LGTY?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [fineract] thesmallstar commented on a change in pull request #1099: FINERACT-822 Enforce UndefinedEquals check
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1099:
URL: https://github.com/apache/fineract/pull/1099#discussion_r444114733
##########
File path: fineract-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java
##########
@@ -100,8 +100,9 @@ public boolean equals(Object obj) {
}
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId) && Objects.equals(other.glAccountId, glAccountId)
- && Objects.equals(other.amount, amount) && Objects.equals(other.entryDate, entryDate)
- && Objects.equals(other.transactionDate, transactionDate) && Objects.equals(other.closingBalance, closingBalance);
+ && Objects.equals(other.amount, amount) && other.entryDate.compareTo(entryDate) == 0 ? Boolean.TRUE
+ : Boolean.FALSE && other.transactionDate.compareTo(transactionDate) == 0 ? Boolean.TRUE
Review comment:
Agreed, so if other.entryDate.compareTo(entryDate) == 0 is executed, then 0 == 0 returns true, which is boolean.
Sorry if I am confusing, I am not able to understand the need for the
```?Boolean.TRUE: Boolean.FALSE ``` part.
----------------------------------------------------------------
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:
users@infra.apache.org