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