Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction #4307

a7med3del1973 · 2025-02-06T05:10:01Z

Description

Replaced the chain of if statements in calcColumnDisplayType
Introduced a helper method getJdbcType(JdbcJavaType columnType) to encapsulate the logic for mapping a JdbcJavaType to a string identifier.
Updated calcColumnDisplayType to switch on the result of getJdbcType and return the corresponding DisplayType.
Added Lombok’s Getter annotation at the class level to remove boilerplate getter methods.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

oluexpert99 · 2025-02-06T10:09:00Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

+        return ResultsetColumnHeaderData.builder()
+                .columnName(columnName)
+                .columnType(JdbcJavaType.getByTypeName(dialect, adjustColumnType(columnType), true))
+                .columnLength(null)


why set null? we could omit this in the builder rather than set null

Same goes for primitive boolean types. It is nice to explicitly set, but in the other hand it is unnecessary

adamsaghy · 2025-02-06T10:09:39Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

@@ -6,9 +6,9 @@
 * to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance
 * with the License. You may obtain a copy of the License at
- *
+ * <p>


Please do not change the licence header

oluexpert99 · 2025-02-06T10:11:43Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        }
+        if (columnType.isTextType()) return DisplayType.TEXT;
+        if (columnType.isStringType()) return DisplayType.STRING;
+        if (columnType.isAnyIntegerType()) return DisplayType.INTEGER;


a switch or a map will be better in this case

@oluexpert99
Thanks for your response .. i noted your comments then refactored the code according to yours and waiting for feedback

adamsaghy · 2025-02-06T10:11:57Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return columnValues != null && !columnValues.isEmpty();
-    }
-
-    public boolean isColumnValueAllowed(final String match) {


This is used in the SeachUtil class... i am not sure why you removed it....

adamsaghy · 2025-02-06T10:12:09Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return false;
-    }
-
-    public boolean isColumnCodeAllowed(final Integer match) {


Same here...

adamsaghy · 2025-02-06T10:13:06Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return this.columnName;
-    }
-
-    public boolean isNamed(final String columnName) {


This methid is used in many classes.

adamsaghy

Kindly review the comments!

a7med3del1973 · 2025-02-09T07:00:20Z

Kindly review the comments!

@adamsaghy
Thanks for your review .. your comments are noted then refactored the code now waiting your feedback

adamsaghy · 2025-02-09T11:11:27Z

@a7med3del1973 please squash your commits!

adamsaghy · 2025-02-09T11:11:43Z

@a7med3del1973 please squash your commits!

a7med3del1973 · 2025-02-10T11:27:39Z

@adamsaghy squash done

a7med3del1973 · 2025-02-10T13:29:26Z

@adamsaghy
@oluexpert99
if your think something should be modified please let me know.

adamsaghy · 2025-02-11T18:35:49Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

 public final class ResultsetColumnHeaderData implements Serializable {

    private final String columnName;
    private JdbcJavaType columnType;
    private final Long columnLength;
    private final DisplayType columnDisplayType;
+    @Getter(AccessLevel.NONE)


We dont need to restricted getter for these fields...

adamsaghy · 2025-02-11T18:36:30Z

...main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java

-        return null;
+        Map<Predicate<JdbcJavaType>, DisplayType> typeMap = new LinkedHashMap<>();
+
+        typeMap.put(JdbcJavaType::isTextType, DisplayType.TEXT);


Using switch-case would be better...

galovics · 2025-02-12T14:37:35Z

@a7med3del1973 what's the purpose here? are you familiar with Java and have an IDE set up properly? I really feel like you're just starting out hence the "used in SearchUtil" comments in the codebase. Also, the PR builds are failing, probably due to formatting issues (haven't checked).

a7med3del1973 · 2025-02-12T16:01:39Z

@galovics Thanks for your review
Added this comments just for clearity. Your comment noted and working on it.

…atures

adamsaghy · 2025-02-24T09:52:14Z

@a7med3del1973 Kindly fix the failing checks:

> Task :fineract-core:modernizer
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:225:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:226:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:227:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:228:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:229:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:230:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:231:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:232:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:233:9: 'if' construct must use '{}'s. [NeedBraces]
Error: eckstyle] [ERROR] /home/runner/work/fineract/fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/ResultsetColumnHeaderData.java:234:9: 'if' construct must use '{}'s. [NeedBraces]

Execution failed for task ':fineract-core:checkstyleMain'.
> A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction
   > Checkstyle rule violations were found. See the report at: file:///home/runner/work/fineract/fineract/fineract-core/build/reports/checkstyle/main.html
     Checkstyle files with violations: 1
     Checkstyle violations by severity: [error:10]

oluexpert99 reviewed Feb 6, 2025

View reviewed changes

adamsaghy reviewed Feb 6, 2025

View reviewed changes

oluexpert99 reviewed Feb 6, 2025

View reviewed changes

adamsaghy reviewed Feb 6, 2025

View reviewed changes

adamsaghy requested changes Feb 6, 2025

View reviewed changes

a7med3del1973 requested a review from oluexpert99 February 9, 2025 06:56

a7med3del1973 requested a review from adamsaghy February 9, 2025 07:00

a7med3del1973 force-pushed the lombok-integration branch from b68b45b to e384bfa Compare February 10, 2025 11:17

a7med3del1973 force-pushed the lombok-integration branch 3 times, most recently from 2b77e08 to 41b4859 Compare February 10, 2025 13:25

adamsaghy reviewed Feb 11, 2025

View reviewed changes

a7med3del1973 force-pushed the lombok-integration branch from 41b4859 to c78a15c Compare February 12, 2025 10:42

a7med3del1973 requested a review from adamsaghy February 12, 2025 10:43

Refactored ResultColumnHeaderData using Lombok and modern Java new fe…

616da39

…atures

a7med3del1973 force-pushed the lombok-integration branch from c78a15c to 616da39 Compare February 19, 2025 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction #4307

Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction #4307

a7med3del1973 commented Feb 6, 2025 •

edited

Loading

oluexpert99 Feb 6, 2025

adamsaghy Feb 6, 2025

adamsaghy Feb 6, 2025

oluexpert99 Feb 6, 2025

a7med3del1973 Feb 9, 2025

adamsaghy Feb 6, 2025

adamsaghy Feb 6, 2025

adamsaghy Feb 6, 2025

adamsaghy left a comment

a7med3del1973 commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

a7med3del1973 commented Feb 10, 2025 •

edited

Loading

a7med3del1973 commented Feb 10, 2025

adamsaghy Feb 11, 2025

adamsaghy Feb 11, 2025

galovics commented Feb 12, 2025

a7med3del1973 commented Feb 12, 2025

adamsaghy commented Feb 24, 2025

Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction #4307

Are you sure you want to change the base?

Refactor ResultsetColumnHeaderData to Use Lombok Annotations for Boilerplate Reduction #4307

Conversation

a7med3del1973 commented Feb 6, 2025 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

a7med3del1973 commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

adamsaghy commented Feb 9, 2025

a7med3del1973 commented Feb 10, 2025 • edited Loading

a7med3del1973 commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galovics commented Feb 12, 2025

a7med3del1973 commented Feb 12, 2025

adamsaghy commented Feb 24, 2025

a7med3del1973 commented Feb 6, 2025 •

edited

Loading

a7med3del1973 commented Feb 10, 2025 •

edited

Loading