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/09 16:42:56 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
thesmallstar opened a new pull request #1019:
URL: https://github.com/apache/fineract/pull/1019
HideUtilityClassConstructor ->
Utility Classes like Helperand Constants are forced to have all methods static, and constructor non-public and non-default
If we keep constructor protected we cannot create an object of that class but support inheritance if required. The other option was to use a private constructor(which won't support inheritance). I have decided to go with protected constructor any contradicting views?
The problems->
1. Some Utility classes don't have all the methods static and also use "this" keyword inside methods.
Solution-> remove this keywords and make the method static.
2. The constructor is used to set values in a utility class
Solution -> Make another method to set values and make constructor protected.
3. Some methods return "this"
Solution-> Very suspicious on this, why is a utility class returning its instance? Still looking into how to fix this.
There are about 150-200 changes so can anyone please verify this approach? Do you see any unintended side effects?
@vorburger @awasum @ptuomola @xurror @percyashu
----------------------------------------------------------------
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 #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#discussion_r444105628
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/bulkimport/populator/client/ClientEntityWorkbookPopulatorTest.java
##########
@@ -60,9 +60,8 @@ public void setup(){
@Test
public void testClientEntityWorkbookPopulate() throws IOException {
//in order to populate helper sheets
- StaffHelper staffHelper=new StaffHelper();
requestSpec.header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON);
- Integer outcome_staff_creation =staffHelper.createStaff(requestSpec,responseSpec);
+ Integer outcome_staff_creation =StaffHelper.createStaff(requestSpec,responseSpec);
Review comment:
Nope, it is all manual clean-up. Just that my IDE(vscode) does give a suggestion for this while writing code which makes it easier to spot.
In this case, since I am removing the object I have "no other option" that making it static it throws out an error.
----------------------------------------------------------------
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 pull request #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#issuecomment-670992135
@vorburger probably some work was done in this space recently? The checkstyle was much simpler (although boring :( ) to implement now.
Just FYI, the only suppressed checks used are for the classes which were extended, and hence we could not make them follow the utility class constructor rules.
----------------------------------------------------------------
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] ptuomola commented on pull request #1019: FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#issuecomment-674465641
@thesmallstar LGTM, and as far as I can see, you've addressed the feedback from @vorburger. So unless @vorburger has further comments, I'm happy to merge 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 #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#discussion_r443114391
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientStatusChecker.java
##########
@@ -25,6 +25,12 @@
import org.slf4j.LoggerFactory;
public class ClientStatusChecker {
+
+ protected ClientStatusChecker() {
+ // prevents calls from subclass(if any)
+ throw new UnsupportedOperationException();
+ }
Review comment:
This is weird / un-usual... I think the typical solution for this is to make the constructor private, and/or (?) declare the class to be `final` so that there can be no subclasses.
```suggestion
private ClientStatusChecker() {
}
```
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/bulkimport/populator/client/ClientEntityWorkbookPopulatorTest.java
##########
@@ -60,9 +60,8 @@ public void setup(){
@Test
public void testClientEntityWorkbookPopulate() throws IOException {
//in order to populate helper sheets
- StaffHelper staffHelper=new StaffHelper();
requestSpec.header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON);
- Integer outcome_staff_creation =staffHelper.createStaff(requestSpec,responseSpec);
+ Integer outcome_staff_creation =StaffHelper.createStaff(requestSpec,responseSpec);
Review comment:
Question: Is this (static invocation of static methods) something you have found a way to make your IDE do automatically? If yes, then this would even allow us to enable Error Prone's `MethodCanBeStatic` in FINERACT-822... but doing it manually for 301 cases (according to @percyashu) is probably not "worth it" (https://en.wikipedia.org/wiki/Diminishing_returns).
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/bulkimport/populator/loan/LoanWorkbookPopulatorTest.java
##########
@@ -64,18 +64,15 @@ public void testLoanWorkbookPopulate() throws IOException {
Assert.assertNotNull("Could not create office" ,outcome_office_creation);
//in order to populate helper sheets
- ClientHelper clientHelper=new ClientHelper(requestSpec,responseSpec);
- Integer outcome_client_creation=clientHelper.createClient(requestSpec,responseSpec);
Review comment:
this is actually a very nice clean up - as it makes absolutely no sense to pass requestSpec, responseSpec twice
----------------------------------------------------------------
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] awasum commented on pull request #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#issuecomment-659526403
Is this still important?
----------------------------------------------------------------
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 #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#discussion_r444105769
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/bulkimport/populator/loan/LoanWorkbookPopulatorTest.java
##########
@@ -64,18 +64,15 @@ public void testLoanWorkbookPopulate() throws IOException {
Assert.assertNotNull("Could not create office" ,outcome_office_creation);
//in order to populate helper sheets
- ClientHelper clientHelper=new ClientHelper(requestSpec,responseSpec);
- Integer outcome_client_creation=clientHelper.createClient(requestSpec,responseSpec);
Review comment:
Yes!
----------------------------------------------------------------
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 #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#discussion_r444104732
##########
File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/ClientStatusChecker.java
##########
@@ -25,6 +25,12 @@
import org.slf4j.LoggerFactory;
public class ClientStatusChecker {
+
+ protected ClientStatusChecker() {
+ // prevents calls from subclass(if any)
+ throw new UnsupportedOperationException();
+ }
Review comment:
Agreed! Making the changes
----------------------------------------------------------------
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 pull request #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#issuecomment-662704563
@thesmallstar do you want to finish this one up?
----------------------------------------------------------------
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 pull request #1019: FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1019:
URL: https://github.com/apache/fineract/pull/1019#issuecomment-674449245
@ptuomola merge 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] ptuomola merged pull request #1019: FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle
Posted by GitBox <gi...@apache.org>.
ptuomola merged pull request #1019:
URL: https://github.com/apache/fineract/pull/1019
----------------------------------------------------------------
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