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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
