jmxtrans / embedded-jmxtrans

In process JMX metrics exporter.
MIT License
82 stars 37 forks source link

defer call to InetAddress.getLocalHost() outisde of the ResultNameStrategy constructor #125

Closed YannRobert closed 8 years ago

YannRobert commented 8 years ago

Hi Cyrille, please consider the following contribution :

Using Callable Objects in order to defer the call to InetAddress.getLocalHost() to the moment it is actually needed. Previously it was called in the ResultNameStrategy constructor, whether or not the actual expression is evaluated.

cyrille-leclerc commented 8 years ago

Thanks @YannRobert !

cyrille-leclerc commented 8 years ago

@YannRobert is there a special reason when you preferred package protected visibility rather than public or private? Is it a kind of "@VisibleForTesting" approach?

YannRobert commented 8 years ago

In the process, I extracted the previous code into a class, extracted an interface from that class, implemented the interface with the new code, wrote a test executing the 2 classes and comparing the results. Then I thought the old way didn't need to be kept (for clarity), so I removed the old code class, the interface, and the test.

I didn't make it public as it's not intended to be used directly by lib users.

Still I didn't want to made it a private inner class again or to inline the code back inside the ResultNameStrategy constructor. Just thought you could do it if you want to. Also the extracted class may be changed again a bit to make it unit-testable. I didn't do it because I wanted to keep the PR simple.

cyrille-leclerc commented 8 years ago

Thanks for the explanation @YannRobert. As I am not familiar with package protected code, I was interested in learning your reasoning.

cyrille-leclerc commented 8 years ago

@YannRobert do you need a release asap?

YannRobert commented 8 years ago

No I don't need a release, thank you.