I'll take a look later tonight or early in the morning and let you know
my thoughts. No need to rush to write something up for me as the code is
probably OK for now.
- Dan
James M Snell wrote:
Ok, many changes. Later this evening (after the kids are in bed) I
will write up a note summarizing the changes and will take a first
stab at a brief server implementation guide based on the revised code.
Please take a look and let me know your thoughts.
- James
Dan Diephouse wrote:
I just took a look at the refactoring. Overall I like it.
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.
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? :-)
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
--
Dan Diephouse
MuleSource
http://mulesource.com | http://netzooid.com/blog
