You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "7ossam7atem1 (via GitHub)" <gi...@apache.org> on 2025/03/03 17:25:10 UTC
[PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
7ossam7atem1 opened a new pull request, #4413:
URL: https://github.com/apache/fineract/pull/4413
## Description
- Replaced all instances of `sch.duedate` with `DUE_DATE_COLUMN`
- Replaced all instances of `created_date` with `CREATED_DATE_COLUMN`
- Standardized `select` keyword usage with `SELECT_KEYWORD`
- Standardized `and` keyword usage with `AND_KEYWORD`
- Improved collection validation by replacing verbose null checks
`(entries != null && entries.size() > 0)` with more concise `!entries.isEmpty()`
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## 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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "7ossam7atem1 (via GitHub)" <gi...@apache.org>.
7ossam7atem1 commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2736528082
> I fail to see how this improves code quality. From my point of view it is harder to read after the changes... :/
This issue was suggested by SonarQube so I think it is all about that it has identified repeated string literals in code and often suggest that practice (Removing the duplicates) for better maintainability
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2740349307
@7ossam7atem1 Sorry but I am not convinced... Extracting some string literals into a variable but leave the rest AS-IS does not help maintainability.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4413: FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality
URL: https://github.com/apache/fineract/pull/4413
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2740406014
@7ossam7atem1 Usually i am not against some cases to extract string literals into variables, especially when it is used multiple times, multiple places.
However, you need to see here: extracting some part of it into variables and leave the rest does not help anything... it does not help readability, it does not help maintainability.
I dont think you should start extracting everything into variables, as the final result will still not be better...
Let me give you an example:
- Lets assume there is a class and it got a Map<String, String> variable and inside this class, many methods are fetching values from this map like:
incomingParameter.get("id")
incomingParameter.get("name")
incomingParameter.get("isActive")
etc.
These appears many places, repetitive...
We can say we are extracthing them into constants and just simply used that to avoid any typo...
Now lets take a look on an example from the PR:
```
.append("GREATEST(" + sqlGenerator.dateDiff("?", DUE_DATE_COLUMN) + ", 0) as numberofdaysoverdue," + DUE_DATE_COLUMN
+ ", pcd.category_id, pcd.provision_percentage,")
```
due date column name was extracted into a variable, but `pcd.category_id` and `pcd.provision_percentage` was not... but to be honest event if you change all of them, it does not help much on the readability.... mostly as there is column and "pcd." prefix, etc. i dont see any value to extract any part of this SQL...
```
String createdUser = rs.getString("createduser");
Date createdDate = rs.getDate("created_date");
Date createdDate = rs.getDate(CREATED_DATE_COLUMN);
Long modifiedById = rs.getLong("lastmodifiedby_id");
```
Again, 1 place it was extracted the rest left AS-IS... not helping the readability, neither maintainability... but at least here, if you decide to extract all these string literals into constants.. i kinda see some value...
```
final String sql1 = SELECT_KEYWORD + mapper1.getSchema() + " where entry." + CREATED_DATE_COLUMN + " like ? ";
```
Here, "Select " was extracted, but "where entry." was not and created date column extracted, but " like ?" not... again... harder do read, easier to miss a missing space or column or anything... i dont see any value these changes...
Kindly review and let me know if we should go into further! ;)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "7ossam7atem1 (via GitHub)" <gi...@apache.org>.
7ossam7atem1 commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2740360704
> @7ossam7atem1 Sorry but I am not convinced... Extracting some string literals into a variable but leave the rest AS-IS does not help maintainability.
I can extract the rest of them also but in case of it would be helpful to do, But if you are not convinced by the idea from the beginning, then fine.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "7ossam7atem1 (via GitHub)" <gi...@apache.org>.
7ossam7atem1 commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2740373298
> @7ossam7atem1 Sorry but I am not convinced... Extracting some string literals into a variable but leave the rest AS-IS does not help maintainability.
I just followed the SonarQube issue guide for these strings
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "7ossam7atem1 (via GitHub)" <gi...@apache.org>.
7ossam7atem1 commented on code in PR #4413:
URL: https://github.com/apache/fineract/pull/4413#discussion_r2041686571
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/provisioning/service/ProvisioningEntriesReadPlatformServiceImpl.java:
##########
@@ -43,6 +43,11 @@
@Slf4j
public class ProvisioningEntriesReadPlatformServiceImpl implements ProvisioningEntriesReadPlatformService {
+ private static final String DUE_DATE_COLUMN = "sch.duedate";
+ private static final String CREATED_DATE_COLUMN = "created_date";
+ private static final String SELECT_KEYWORD = "select ";
+ private static final String AND_KEYWORD = " and ";
Review Comment:
> Honestly I'm not sure if this refactor improves code quality at all. You just extracted standard SQL keywords into constants and in some places it makes it even worse to read the code.
>
> What would really improve the quality here is if you start rewriting the native SQL into QueryDSL/JPQL/Criteria API anything that's typed.
I was thinking about that at first but I saw that keeping the use of native SQL queries is important bc they are complex and critical for performance also they are already optimized so converting them to JPA would be a headache and might impact performance , for that I thought that It's better to organize them
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2753655467
@7ossam7atem1 You can give it a try, but like i said, i am not sure there is anything we can improve here... if you want you can extract the field names into constants, but i dont want to see string literal and variable aggregation back and forth as that does not help readability.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "7ossam7atem1 (via GitHub)" <gi...@apache.org>.
7ossam7atem1 commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2742287038
> @7ossam7atem1 Usually i am not against extracting string literals into variables, especially when it is used multiple times, multiple places. However, you need to see here: extracting some part of it into variables and leave the rest does not help anything... it does not help readability, it does not help maintainability.
>
> I dont think you should start extracting everything into variables, as the final result will still not be better...
>
> Let me give you an example:
>
> * Lets assume there is a class and it got a Map<String, String> variable and inside this class, many methods are fetching values from this map like:
> incomingParameter.get("id")
> incomingParameter.get("name")
> incomingParameter.get("isActive")
> etc.
>
> These appears many places, repetitive...
>
> We can say we are extracthing them into constants and just simply used that to avoid any typo... we can say it has some value...
>
> Now lets take a look on an example from the PR:
>
> ```
> .append("GREATEST(" + sqlGenerator.dateDiff("?", DUE_DATE_COLUMN) + ", 0) as numberofdaysoverdue," + DUE_DATE_COLUMN
> + ", pcd.category_id, pcd.provision_percentage,")
> ```
>
> due date column name was extracted into a variable, but `pcd.category_id` and `pcd.provision_percentage` was not... but to be honest even if you change all of them, it does not help much on the readability.... mostly as there is colon and "pcd." prefix, etc. i dont see any value to extract any part of this SQL...
>
> ```
> String createdUser = rs.getString("createduser");
> Date createdDate = rs.getDate("created_date");
> Date createdDate = rs.getDate(CREATED_DATE_COLUMN);
> Long modifiedById = rs.getLong("lastmodifiedby_id");
> ```
>
> Again, 1 place it was extracted the rest left AS-IS... not helping the readability, neither maintainability... but at least here, if you decide to extract all these string literals into constants.. i kinda see some value...
>
> ```
> final String sql1 = SELECT_KEYWORD + mapper1.getSchema() + " where entry." + CREATED_DATE_COLUMN + " like ? ";
> ```
>
> Here, "Select " was extracted, but "where entry." was not and created date column extracted, but " like ?" not... again... harder do read, easier to miss a missing space or colon or anything... i dont see any value of these changes...
>
> Kindly review and let me know if we should go into further! ;)
@adamsaghy
what I understood is that I could do the following:
- Extract all column names consistently, not just some of them
- keeping the related parts of sql queries together for better readability
did i get it?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2181: Improve ProvisioningEntriesReadPlatformServiceImpl code quality [fineract]
Posted by "7ossam7atem1 (via GitHub)" <gi...@apache.org>.
7ossam7atem1 commented on PR #4413:
URL: https://github.com/apache/fineract/pull/4413#issuecomment-2764945066
> @7ossam7atem1 You can give it a try, but like i said, i am not sure there is anything we can improve here... if you want you can extract the field names into constants, but i dont want to see string literal and variable aggregation back and forth as that does not help readability.
ok , gonna work on it
--
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.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org