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

Rest api refactor #590

Closed
wants to merge 2 commits into from
Closed

Rest api refactor #590

wants to merge 2 commits into from

Conversation

redwanulsourav
Copy link
Contributor

@redwanulsourav redwanulsourav commented Mar 11, 2023

Refactored the code for database and resource creation. #523

@redwanulsourav
Copy link
Contributor Author

redwanulsourav commented Mar 11, 2023

@JohannesLichtenberger I kept the handle() functions in XMLCreate2 and JSONCreate2 commented out, because initially that's what I wrote, but later realized if I put the common handle() code in CreateHandler.handle() then I would have less code duplication. Similar stuff can be done for some other functions as well I believe.

Also I think for Creation, I couldn't find many common stuff among XML and JSON that can be refactored into abstract class. Before proceeding further, I wanted to check if this was ok with you. Would it be okay to put some common logic in CreateHandler class? In another design, CreateHandler can be minimal if its handle() function's body simply called the handle() function in XMLCreate2 and JSONCreate2. I wasn't sure about which one was better design, so I kept it like this for now.

@JohannesLichtenberger
Copy link
Member

@redwanulsourav I would delete CreateHandler and just call XMLCreate2 / JSONCreate2 similar to how it's in the master branch (with the exception, that these classes extend the abstract class, which handles common stuff). Furthermore, I'd introduce an interface for the methods, then the common stuff in the abstract classes and specific stuff in XMLCreate2 and JSONCreate2 (obviously your changes have to overwrite the existing classes once we're ready to process with merging your PR) and the two new classes have to be deleted afterwards.

I'd proceed like this as well for the other classes (interface + abstract class + implementations).

@redwanulsourav
Copy link
Contributor Author

redwanulsourav commented Mar 11, 2023

@JohannesLichtenberger I think I understood what you have mentioned. I just wanted to confirm it again. Would it be like this:AbstractCreateHandler implements HandlerInterface and AbstractDeleteHandler implements HandlerInterface. From there, XMLCreate extends AbstractCreateHandler and JSONCreate extends AbstractCreateHandler. And, from there in SirixVerticle when calling, I would call XMLCreate.handle() or XMLDelete.handle() and so on.

@JohannesLichtenberger
Copy link
Member

Yes :-)

@JohannesLichtenberger
Copy link
Member

I think it's probably the cleanest solution, but of course I don't want to dictate this :-) I hope it makes sense, so I'm of course up for discussions if you think another approach would be better.

@redwanulsourav
Copy link
Contributor Author

I think (interface + abstract + implementations) approach will be a clean one. It is pretty close to what I have implemented so far. I am closing this PR and will create a new PR soon. Thanks for the clear instructions.

@redwanulsourav redwanulsourav deleted the rest-api-refactor branch March 14, 2023 23:34
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