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

Refactoring of JSONCreate and XMLCreate to extract common logic #586

Closed
wants to merge 2 commits into from
Closed

Refactoring of JSONCreate and XMLCreate to extract common logic #586

wants to merge 2 commits into from

Conversation

redwanulsourav
Copy link
Contributor

@redwanulsourav redwanulsourav commented Mar 10, 2023

For #523
Key changes:
- Created CreateHandler.kt which extracts common logic between XMLCreate() and JSONCreate()
- Currently common logic handled: Check if database already exists and if not then create it
- Updated SirixVerticle.kt to use CreateHandler instead of XMLCreate and JSONCreate
- Created XMLCreate2.kt JSONCreate2.kt that has mostly code from XMLCreate.kt and JSONCreate.kt so that it is usable from CreateHandler.kt

TODO:
- Extract more common logic between XMLCreate2 and JSONCreate2
- Handle multipart/form-data with XMLCreate2 and JSONCreate2
- Handle and test resource creation using CreateHandler
- Eventually remove XMLCreate and JSONCreate

@JohannesLichtenberger
Copy link
Member

I know I did something like that with few other handlers, but I think it would be worth considering if instead we could either encapsulate the common logic in abstract classes.

@JohannesLichtenberger
Copy link
Member

Thanks for working on this @redwanulsourav 👍

@redwanulsourav
Copy link
Contributor Author

I know I did something like that with few other handlers, but I think it would be worth considering if instead we could either encapsulate the common logic in abstract classes.

I agree, that would make it more modular. I was following the existing codebase and I thought that was how it was supposed to be done. I can implement this with abstracted classes and inheritance as well.

How it would be if I made CreateHandler a super abstract class, from which XMLCreate and JSONCreate would inherit? Later, all the common logic between handlers (CreateHandler, DeleteHandler) can be pushed upwords by creating an abstract class from which all the Handlers (CreateHandler, DeleteHandler) would inherit. If the Handlers don't have a lot in common, the super class can be turned into an interface as well. Does this plan sound ok?

@JohannesLichtenberger
Copy link
Member

Yes, sounds like a good plan (though I think maybe it's useful to have different abstract classes from which the XML and JSON implementations inherit for the CRUD stuff). XmlCreateHandler extends AbstractCreateHandler, JsonCreateHandler extends AbstractCreateHandler... and analogous the other types. I think it's better to stop at this point instead of to refactor to one big abstract class, but we'll see :-)

@redwanulsourav
Copy link
Contributor Author

I will do that and create a new PR. For now I am closing current PR. Thanks for the instructions.

@JohannesLichtenberger
Copy link
Member

Thanks for your work and interest in the project!

@redwanulsourav redwanulsourav deleted the rest-api-refactor branch March 11, 2023 07:55
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

Successfully merging this pull request may close these issues.

2 participants