[GitHub] [fineract] vorburger commented on a change in pull request #1019: [WIP] FINERACT-821 Added and Enforced HideUtilityClassConstructor checkstyle

[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