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

Update HOXT API to use the builder pattern #50

Closed
wants to merge 1 commit into from

Conversation

Tibo-lg
Copy link
Contributor

@Tibo-lg Tibo-lg commented Jun 28, 2015

I tried to implement the builder pattern for the HOXT API:

  • The AbstractHttpOverXmpp, HttpOverXmppReq and HttpOverXmppResp implement the builder pattern in the same style as ConnectionConfiguration and XMPPTCPConnectionConfiguration.
  • The providers had to be refactored to make use of the builders; I had to separate the parsing of headers and data.
  • I realized my previous patch did not solve the NullPointerException issue. I updated so that headers is given directly to optAppend but data does not extend Element so it needs an extra if.

@Flowdalic
Copy link
Member

Thanks for your contribution. I really appreciate your work and effort towards working together on a HOXT API. +1

I hadn't had a close look at the changes, But could you please

  • remove the merge commit (i.e. your work should be (usually) a single commit)
  • fix the build errors on java 8 (see travis)

if (parser.getName().equals(ELEMENT_DATA)) {
done = true;
if (eventType == XmlPullParser.START_TAG) {
if (parser.getName().equals(ELEMENT_TEXT)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an ideal use case for a string switch-case. See (most) other providers for examples.

@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Jun 29, 2015

remove the merge commit (i.e. your work should be (usually) a single commit)

I fixed that!

fix the build errors on java 8 (see travis)

Should also be fixed.

That's an ideal use case for a string switch-case. See (most) other providers for examples.

Agreed, I had lazily reused the existing code. It is updated now.

@Flowdalic
Copy link
Member

Cherry picked as 4d57848

I'd did some further cleaning of the HOXT code based on your commit.

Thank you !

@Flowdalic Flowdalic closed this Jul 1, 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
2 participants