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

Replace SimpleDateFormat with Joda Time to Improve Performance #31

Closed
damnhandy opened this issue Sep 20, 2015 · 12 comments
Closed

Replace SimpleDateFormat with Joda Time to Improve Performance #31

damnhandy opened this issue Sep 20, 2015 · 12 comments

Comments

@damnhandy
Copy link
Owner

@asaph recently added PR #30, which replaces SimpleDateFormat with JodaTime. I've been reluctant on adding new dependencies as this library is used in many different Java environments such as Android. I'm not sure how Android plays with JodaTime or how folks in the Android space feel about an additional dependency? Curious to hear from @drdamour and others working on Android and other non-Oracle JVMs.

@asaph
Copy link
Contributor

asaph commented Sep 21, 2015

@damnhandy Thanks for your consideration. Although most enterprise users will barely notice the addition of Joda Time (if they don't already have it in their project), you make a very good point about Android clients. I could submit another pull request that mitigates the SimpleDateFormat performance issue by using ThreadLocal without introducing a new dependency. Would you prefer to go that way instead?

@drdamour
Copy link

talked to some droid devs on our teams, they are already on Joda time. If you ever dropped java 6 or 7 support (8 forward) you'd want to revisit and remove Joda i think.

@asaph
Copy link
Contributor

asaph commented Sep 21, 2015

@drdamour Agreed. Specifically, Java 8 introduces java.time.format.DateTimeFormatter which mitigates the issues associated with SimpleDateFormat. But using it would require bumping source=1.6 and target=1.6 to source=1.8 and target=1.8, respectively.

@damnhandy
Copy link
Owner Author

Bumping the source isn't really an option either as that will definitely cause troubles for Android developers. Since it appears that Joda is not contentious, I'm going to accept the PR and close this issue.

@asaph
Copy link
Contributor

asaph commented Sep 22, 2015

@damnhandy Thank you for merging PR #30. I see that you added this enhancement to the 2.1 milestone. Do you have an estimated release date in mind for v2.1?

@damnhandy
Copy link
Owner Author

I'm going to see what I can do about issue #26 first. I have to jog my memory about what the hell I was thinking the VarExpanders. Realistically, we're looking at mid-October.

@four01
Copy link

four01 commented Dec 22, 2016

This has actually caused issues for us. We didn't upgrade to the this version because it introduced joda, but are forced to recently due to a recent bug fix.

We use threetenbp, so including joda as well isn't ideal. If this can be refactored to use a plugin system which can be registered at runtime, that would be ideal. For now we have been forced to fork your project to remove joda.

Thanks for a great library!

@drdamour
Copy link

when you say isn't ideal...do you just mean because you don't want another lib?

@four01
Copy link

four01 commented Dec 22, 2016

Sorry for being unclear.

We're using this library in our Android project where method counts matter. We're already using the excellent threetenbp library to handle dates. If we had the option we'd make URI Templates use threeten already because that's what we already have included. Including Joda instead adds too many extra methods.

Please look at http://www.methodscount.com/?lib=com.damnhandy%3Ahandy-uri-templates%3A2.1.6

Thanks.

@drdamour
Copy link

damn dalvik!

@four01
Copy link

four01 commented Dec 22, 2016

A simple plugin architecture could resolve this. Or an interface where we can implement our own approach.

In our app, we don't actually use dates as parameters at all. It seems like although converting dates to strings is a nice bonus feature it doesn't seem like it's a core feature of RFC 6570. Maybe this is something that needs to be exposed as user plugins and let the core library handle the basic/primitive cases.

@damnhandy
Copy link
Owner Author

Let's continue this discussion in issue #55 since this issue is closed.

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

4 participants