naren2605 commented on PR #7641: URL: https://github.com/apache/netbeans/pull/7641#issuecomment-2274810806
> I am sorry, but it seems this patch contains way too many "drive-by" changes to allow a reasonable review. I probably could accept having constants instead of hardcoded number, but changing `lastWSOffset` to `lastWhiteSpaceOffset` does not seem very useful to me. I would suggest to cleanup the patch, removing as many of drive-by changes (changes that are not necessary for the functionality of the patch). hi @lahodaj , @mbien thanks for taking a look in to pr , i have added minor refactoring changes to improve readability of code in the same method where fix related changes exist(while i was debugging the issue), just following this [clean code principle](https://learning.oreilly.com/library/view/97-things-every/9780596809515/ch08.html). as @mbien mentioned i have added refactoring changes in one commit(where tests are passing before adding fix) and fix as head commit(with new tests). i would like to summarise refactoring changes as below. take a look and let me know which and all refactoring changes you see doesn't add value, will revert them accordingly. 1. introduced variables like `ACTION_TYPE_ADD_BLANK_LINE ` and `STATE_INITIAL_TEXT` as constants to remove code smell - *magic numbers* (as you are ok with this and @mbien was suggesting to add enums, will enumize these) 2. variable like currNWSPos, lastNWSPos , currWSPos, lastWSPos as NWS and WS here was bit cryptic i have renamed these variables accordingly to improve readability(will revert these changes as you suggested these doesn't seem to be adding value) 3. renamed variable names from `toAdd ` to `marker` , `marker` noun suited better as we are adding marked position and action to `LinkedList<Pair<Integer, Integer>> marks = new LinkedList<Pair<Integer, Integer>>()` , renamed variable`checkOffset` verb to `markedOffset` noun as we are retrieving markedOffset from `marks` list 4. few marked variables like `offset` as final , as current method is ~700 lines long and multiple variables and states are mutating, just marked some variables like these to `final` which are used as readonly values with the intent to improve readability. let me know @lahodaj if you don't see value add on point's 3,4 will go ahead and revert 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information about the NetBeans mailing lists, visit: https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
