Dan Diephouse wrote:
I just took a look at the refactoring. Overall I like it.
Good to hear.
Some comments:
Most of my comments have to do with the ext package. I like the
interfaces for the most part in the top level package. But I think to
some degree the separation which has occurred inside the ext/basic
packages is unecessary.
Agreed.
AbstractProvider already includes the concepts of CollectionAdapter &
WorkspaceManager. So why have so many derivative implementations? There
is AbstractProivder, AbstractWorkspaceManagerProvider, BasicProvider,
ExtendedProvider. They are all slight variations on the same thing and
should all be combined into one thing IMO. I was kind of on the fence
before, but after seeing the class proliferation, think its a bad idea
to have a provider implement CollectionAdapter & WorkspaceManager. It
creates an overly complex code base and will be confusing for users -
i.e. which Provider do I use? Lets make the typical answer to that
question just one answer unless they have to override some advanced
functionality. Then the process of writing a server becomes conceptually
simpler:
- Write a CollectionAdapter
- If necessary write a workspace manager
- Glue it together with a ServiceContext and a provider
See how this eliminates a step and confusion from the process?
Along these lines:
- ExtendedWorkspaceManager -> WorkspaceManagerImpl, SimpleWorkspaceInfo
-> WorkspaceImpl (I personally like Impl better, but whatever)
- Collapse NamedCollectionAdapter into CollectionAdapter
- Delete AbstractWorkspaceManagerProvider,
AbstractNamedCollectionAdapter, redundant Basic* stuff, etc
And see how many classes can be removed? :-)
Again agreed. I would have combined much of this stuff already but I
wanted to keep the various approaches separate in this first revision
and then start iterating to simplify things. Mainly, I'm still not
convinced that there is a need for the BasicAdapter interface. I
understand what the feed-server stuff is doing but I think the
additional interfaces causes more difficulty than it is worth.
Other comments:
- Can we please move to TestNG or JUnit4? The test code to deal with
servers is fairly hackish and this would alleviate that.
- I find the code style a little unreadable. I'm hesitant to bring it up
though as its a neverending battle. If I had two wishes though they
would be that we don't break constrcutor/method lines unless necessary
and we move to 4 spaces :-)
I have more thoughts, but I need to hop on a plane. Nice work James and
looking forward to digging in more soon.
- Dan
--
Dan Diephouse
MuleSource
http://mulesource.com | http://netzooid.com/blog
