Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should Partner-related methods be in IPartnershipFactory? #9

Closed
jochenberger opened this issue Sep 9, 2015 · 6 comments
Closed

Should Partner-related methods be in IPartnershipFactory? #9

jochenberger opened this issue Sep 9, 2015 · 6 comments

Comments

@jochenberger
Copy link
Collaborator

This is rather a question than an actual issue.
I wonder if the Partner-related methods and classes (e.g. PartnerMap) should be moved to XMLPartnershipFactory. AFAICT, the whole "Partner" concept is currently local to the XML configuration, the rest of the code seems to work with complete Partnerships.
I'm asking because I have created a custom IPartnershipFactory that stores partnerships in a MongoDB collection (I could contribute the code if you're interested) and was confused that I had to implement all those methods but they were never called from anywhere.

@phax
Copy link
Owner

phax commented Sep 9, 2015

Hi Jochen!
I adopted the IPartnershipFactory and removed the getPartnerMap and getPartnershipMap methods as suggested - they are still available directly from within AbstractPartnershipFactory.
Additionally I changed the return type of addPartnership from void to EChange to handle existing partnerships more gracefully.

Concerning the Mongo Partnership: I'd be happy to add this. I would create a separate Maven module (like as2-partnership-mongodb) and add it to this workspace. Is there a standalone test available that works without having Mongo locally installed?

Thanks, Philip

@jochenberger
Copy link
Collaborator Author

Is there a standalone test available that works without having Mongo locally installed?

Not yet, but I wanted to write one anyway. I'm not too familiar with Maven and JUnit anymore, I usually do Gradle/Spock these days.

@jochenberger
Copy link
Collaborator Author

I adopted the IPartnershipFactory and removed the getPartnerMap and getPartnershipMap methods as suggested

Do you want to keep the other methods, like addPartner, removePartner and the others that don't have "ship" in their name? I think they are also unused.

@phax
Copy link
Owner

phax commented Sep 9, 2015

They do make sense in certain ways. I extracted a sub-interface IPartnershipFactoryWithPartners that should do the trick for you by sticking to the existing interface :)

@jochenberger
Copy link
Collaborator Author

Yes, it does the trick, thanks.
I must admit that I find it hard to understand the separation between Partner and Partnership. Why is Partnership a real class whereas Partner is only a Map? It seems to me as if Partners are helper objects that are only used to bootstrap the Partnerships from an XML configuration.
I think that an application using as2-lib should be supposed to care about either Partners or Partnerships. If it should care about Partners, then the creation of the actual Partnerships should be encapsulated somewhere within the lib.
I'm not sure if that makes any sense to you, if it doesn't, maybe that proves, that the current parter vs. partnership concept is a little hard to grasp. ;-)

@phax
Copy link
Owner

phax commented Sep 10, 2015

Well good point. I introduced a class Partner with a corresponding IPartner interface. Class Partner is basically a StringMap but with a different type around. This should make things more easily graspable.

But you are absolutely right that this construct is only used within XML. For the rest, the getSenderID and getReceiverID stuff is sufficient. Therefore I moved everything that is "xml partnership" related to a separate sub-package.

@phax phax closed this as completed Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants