Skip to content
This repository has been archived by the owner on Jan 14, 2018. It is now read-only.

Add all parameters to SpiceService.createRequestProcessor() #245

Closed
lsuski opened this issue Jan 10, 2014 · 8 comments
Closed

Add all parameters to SpiceService.createRequestProcessor() #245

lsuski opened this issue Jan 10, 2014 · 8 comments
Assignees
Milestone

Comments

@lsuski
Copy link

lsuski commented Jan 10, 2014

Hi,
could you add all parameters required in RequestProcessor constructor to SpiceService.createRequestProcessor() signature or make SelfStopperRequestProcessorListener class protected or provide some getters. It's hard to create own implementation of RequestProcessor using same parameters objects as default. Specially when it comes to RequestProcessorListener implementation.

I think that it could be a good idea to make whole project more "injectable" (if it is posiible) to let people provide own implementation in many cases, and not create issue here, and provide as well default factories to create default class configuration. For example it's not possible to pass custom RequestRunner to RequestProcessor although RequestRunner has no reference to RequestProcessor - we pass all arguments to RequestProcessor required for RequestRunner when we could just pass RequestRunner (as some interface rather than concrete class) to RequestProcessor constructor and then follow Inversion of Control pattern.

Thanks.

@ghost ghost assigned stephanenicolas Jan 14, 2014
@stephanenicolas
Copy link
Owner

Hi @lsuski ,

I am gonna work on that issue right after lunch. If you have any detail on the issue, I would appreciate them. To sum up, now I see :

  • add all parameters required in RequestProcessor constructor to SpiceService.createRequestProcessor()
  • make SelfStopperRequestProcessorListener class protected
  • investigate relation between RequestProcessor & RequestRunner

@stephanenicolas
Copy link
Owner

Hi @lsuski ,

you are right, those dependencies were a real mess ! Thx for pointing this out.

Would you mind to have a look at current master branch and detail if you want me to add/change anything. I am now really in favor of defining interfaces for all the core sub-components, I just feel like this would turn into a "bad smell" for the clarity of RS. But I can change my mind if you convince me.

@lsuski
Copy link
Author

lsuski commented Jan 14, 2014

Hi,
I,ve sent a pull request with my proposition of refactor but a little bit late :) . Sorry for code formatting, It's quite different in my IDE settings and I forgot about this.

@stephanenicolas
Copy link
Owner

BTW, this must be one of the last issue for the 1.4.11 release.

@stephanenicolas
Copy link
Owner

Let me change some stuff after having a look at PR #248 and push again. Then tell me if it's fine like this.

@lsuski
Copy link
Author

lsuski commented Jan 14, 2014

I am a purist when it comes to programming and I prefer in my work interface based architecture but agree that it introduces some difficulties in architecture understanding and debbuging (but provides flexibility) so it can be a problem with RS. It is used by many users so I wouldn't urge to change RS int this way.
Thanks a lot for interest.

@stephanenicolas
Copy link
Owner

Ok, is master looking better for your project ? @lsuski

@lsuski
Copy link
Author

lsuski commented Jan 14, 2014

Yes, it's ok thanks a lot.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants