[GitHub] [accumulo] dlmarion commented on pull request #2432: Modified ThreadPools to conditionally register metrics

[GitBox <gi...@apache.org>]

dlmarion commented on pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#issuecomment-1023167707


   > Boolean parameters can make code harder to read. Instead of boolean parameters, you could have a separate method name, like .action() and .actionWithMetrics(), or you could use a builder, like builder.withMetrics().build(), or you could have enums for flags, like .action(EnumSet.of(WITH_METRICS, OTHER_FLAG))
   
   I have no issue with modifying ThreadPools to use a builder pattern, but I think that should be done in a different issue. IDEs make it pretty easy to see the formal parameter name for the method and the formal parameter name is descriptive in this case.
   
   > Another option would be to replace the last boolean argument in the method with a new enum type. The single boolean here is not too bad, an enum would be more readable. We have had methods in the past w/ multiple boolean arguments and that was awful to read.
   
   I have no issue with modifying this PR to use an enum for the boolean parameter, but I'm not sure that it's necessary if there is going to be a follow-on issue created to modify the class to use a builder pattern.


-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org