From commits-return-10520-archive-asf-public=cust-asf.ponee.io@fineract.apache.org Sat Jun 20 09:02:32 2020 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id DF7AB180638 for ; Sat, 20 Jun 2020 11:02:31 +0200 (CEST) Received: (qmail 34591 invoked by uid 500); 20 Jun 2020 09:02:31 -0000 Mailing-List: contact commits-help@fineract.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@fineract.apache.org Delivered-To: mailing list commits@fineract.apache.org Received: (qmail 34582 invoked by uid 99); 20 Jun 2020 09:02:31 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 20 Jun 2020 09:02:31 +0000 From: =?utf-8?q?GitBox?= To: commits@fineract.apache.org Subject: =?utf-8?q?=5BGitHub=5D_=5Bfineract=5D_vorburger_commented_on_a_change_in_pul?= =?utf-8?q?l_request_=231019=3A_=5BWIP=5D_FINERACT-821_Added_and_Enforced_Hi?= =?utf-8?q?deUtilityClassConstructor_checkstyle?= Message-ID: <159264375116.8807.7854999316192895784.asfpy@gitbox.apache.org> Date: Sat, 20 Jun 2020 09:02:31 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit In-Reply-To: References: 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