You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by Bob Paulin <bo...@bobpaulin.com> on 2014/11/20 18:39:27 UTC
Refactoring possibilities in the BundleWiringImpl for Weaving
All,
Wanted to get some thoughts on some things in the BundleWiringImpl.
There seems to be some very large methods in this class which makes it
very difficult to write tests. I wanted to get some opinions on
breaking some of the methods up to improve readability and testability.
The findClass method within the BundleClassLoader is a good example of a
large method that could benefit from this. I'd like to break down the
method to better fit the specification around Weaving so that there are
specific methods around Transform and Define phases of the weaving.
So for example it could be something like:
try
{
transformClass(felix, hooks, wovenClassListeners,
name, bytes);
}
catch (Exception e)
{
wci.setState(WovenClass.TRANSFORMING_FAILED);
callWovenClassListeners(felix,
wovenClassListeners, wci);
}
/* Do locking code - omitted for breivity */
try {
clazz = defineClass(felix, wovenClassListeners,
wci, name,
clazz, bytes, content, pkgName, lock);
} catch (ClassFormatError e) {
if(wci != null){
wci.setState(WovenClass.DEFINE_FAILED);
callWovenClassListeners(felix,
wovenClassListeners, wci);
}
}
Are there other arrangements that make more sense for this class or
reasons just to keep it all in the same method? I'm a big fan of
smaller methods where possible since it helps with readability and
testing. I might be interesting to run the code through a static
analysis tool such as SonarQube (I know some other projects build this
into their Jenkins processes) for no other than to identify some spots
that might benefit from some refactoring. Also I realized that I added a
logger to the constructor of the BundleClassLoader only to realize that
a logger was being exposed through the BundleWiringImpl class that's
passed in the constructor. Any thoughts on exposing the logger through
a constructor instead? Looking forward to input on this.
- Bob
Re: Refactoring possibilities in the BundleWiringImpl for Weaving
Posted by Bob Paulin <bo...@bobpaulin.com>.
Hi David,
Thanks for the reply. I think the unit test first approach makes a lot
of sense in making sure the refactoring doesn't break the existing
method. It will be painful with the size of the method but worth it in
the end.
Appreciate the thoughts on the logger. I felt a bit silly making the
change after I saw it was injected through another object but I had to
ask myself why I missed it. I'm used to the log4j approach where the
logger is typically statically added to the class at the top of the
file. It appears that it's the logger object that gets instantiated in
the Felix class is what's getting passed around to all these other
classes so we're being efficient with the heap. With respect to your
point on memory we're adding another reference to the existing logger in
exchange for readability. Based on what I've seen in the class there
are a few places the spec calls for logging where it was left out
perhaps because the developer was not aware that a logger was available
in the class. So I think in this case the trade off will be worth it in
the long term. Perhaps there are some other places we can find to
reduce a reference to make the delta on the memory footprint zero.
Thanks!
- Bob
On 11/21/2014 2:31 AM, David Bosschaert wrote:
> Hi Bob,
>
> I'm personally a big fan of smaller methods too. As you say, it makes
> it easier to write unit tests for them and also makes it easier to
> understand them.
> It might be good to write a unit test for the big method as a start
> and make sure that it's still passing with the refactored methods.
>
> Wrt to the logger. Generally the central logger as defined in the
> Felix.java class is used throughout other classes in the framework. A
> common pattern is to pass the logger via the constructor so that
> sounds good to me. I have seen cases where the Felix object was
> available and Felix.getLogger() was used instead. Personally I think
> passing it explicitly is a little bit nicer, although it does have the
> cost of being stored in a field, so that's something to keep in mind.
>
> Cheers,
>
> David
>
> On 20 November 2014 17:39, Bob Paulin <bo...@bobpaulin.com> wrote:
>> All,
>>
>> Wanted to get some thoughts on some things in the BundleWiringImpl. There
>> seems to be some very large methods in this class which makes it very
>> difficult to write tests. I wanted to get some opinions on breaking some of
>> the methods up to improve readability and testability. The findClass method
>> within the BundleClassLoader is a good example of a large method that could
>> benefit from this. I'd like to break down the method to better fit the
>> specification around Weaving so that there are specific methods around
>> Transform and Define phases of the weaving.
>>
>> So for example it could be something like:
>> try
>> {
>> transformClass(felix, hooks, wovenClassListeners,
>> name, bytes);
>> }
>> catch (Exception e)
>> {
>> wci.setState(WovenClass.TRANSFORMING_FAILED);
>> callWovenClassListeners(felix, wovenClassListeners,
>> wci);
>> }
>> /* Do locking code - omitted for breivity */
>> try {
>> clazz = defineClass(felix, wovenClassListeners, wci,
>> name,
>> clazz, bytes, content, pkgName, lock);
>> } catch (ClassFormatError e) {
>> if(wci != null){
>> wci.setState(WovenClass.DEFINE_FAILED);
>> callWovenClassListeners(felix,
>> wovenClassListeners, wci);
>> }
>> }
>>
>> Are there other arrangements that make more sense for this class or reasons
>> just to keep it all in the same method? I'm a big fan of smaller methods
>> where possible since it helps with readability and testing. I might be
>> interesting to run the code through a static analysis tool such as SonarQube
>> (I know some other projects build this into their Jenkins processes) for no
>> other than to identify some spots that might benefit from some refactoring.
>> Also I realized that I added a logger to the constructor of the
>> BundleClassLoader only to realize that a logger was being exposed through
>> the BundleWiringImpl class that's passed in the constructor. Any thoughts
>> on exposing the logger through a constructor instead? Looking forward to
>> input on this.
>>
>> - Bob
Re: Refactoring possibilities in the BundleWiringImpl for Weaving
Posted by David Bosschaert <da...@gmail.com>.
Hi Bob,
I'm personally a big fan of smaller methods too. As you say, it makes
it easier to write unit tests for them and also makes it easier to
understand them.
It might be good to write a unit test for the big method as a start
and make sure that it's still passing with the refactored methods.
Wrt to the logger. Generally the central logger as defined in the
Felix.java class is used throughout other classes in the framework. A
common pattern is to pass the logger via the constructor so that
sounds good to me. I have seen cases where the Felix object was
available and Felix.getLogger() was used instead. Personally I think
passing it explicitly is a little bit nicer, although it does have the
cost of being stored in a field, so that's something to keep in mind.
Cheers,
David
On 20 November 2014 17:39, Bob Paulin <bo...@bobpaulin.com> wrote:
> All,
>
> Wanted to get some thoughts on some things in the BundleWiringImpl. There
> seems to be some very large methods in this class which makes it very
> difficult to write tests. I wanted to get some opinions on breaking some of
> the methods up to improve readability and testability. The findClass method
> within the BundleClassLoader is a good example of a large method that could
> benefit from this. I'd like to break down the method to better fit the
> specification around Weaving so that there are specific methods around
> Transform and Define phases of the weaving.
>
> So for example it could be something like:
> try
> {
> transformClass(felix, hooks, wovenClassListeners,
> name, bytes);
> }
> catch (Exception e)
> {
> wci.setState(WovenClass.TRANSFORMING_FAILED);
> callWovenClassListeners(felix, wovenClassListeners,
> wci);
> }
> /* Do locking code - omitted for breivity */
> try {
> clazz = defineClass(felix, wovenClassListeners, wci,
> name,
> clazz, bytes, content, pkgName, lock);
> } catch (ClassFormatError e) {
> if(wci != null){
> wci.setState(WovenClass.DEFINE_FAILED);
> callWovenClassListeners(felix,
> wovenClassListeners, wci);
> }
> }
>
> Are there other arrangements that make more sense for this class or reasons
> just to keep it all in the same method? I'm a big fan of smaller methods
> where possible since it helps with readability and testing. I might be
> interesting to run the code through a static analysis tool such as SonarQube
> (I know some other projects build this into their Jenkins processes) for no
> other than to identify some spots that might benefit from some refactoring.
> Also I realized that I added a logger to the constructor of the
> BundleClassLoader only to realize that a logger was being exposed through
> the BundleWiringImpl class that's passed in the constructor. Any thoughts
> on exposing the logger through a constructor instead? Looking forward to
> input on this.
>
> - Bob