Improve TwitchClientHelper class

[thomasvdvoort]

Description

The sleeping time should be 1 second, but it is 10 seconds.

twitch4j/twitch4j/src/main/java/com/github/twitch4j/TwitchClientHelper.java

Line 179 in 270e884

Thread.sleep(10000);

Solutions

Change 10000 to 1000

[PhilippHeuer]

Actually it was supposed to be 10 seconds to start off, i didn't update the comment. Do you think a 10 sec delay between polling the data is to long?

[thomasvdvoort]

Hey,

I've noticed for example StreamElements has a shorter delay than 10 seconds. I do think 1 second would be better, if it does not ruin major performance.
Is there any downside to shorten the time?

[potb]

I find that 1 second is fine for implementations that would need real-time updates.
Could be good if we had an option to choose the delay.

[PhilippHeuer]

It takes the api about 2-3 minutes until it reports the change because of caching on twitches side. Therefore it isn't really worth using up your api quota to request it every second. But its a good idea to make it configurable.

[boostchicken]

There should be a configurable splay on this, possibly using a random value or exponential back off. Most API implementations handle this out of the box. It should be configurable and enabled on all APIs.

[boostchicken]

Also, sleeping threads on large scale apps could be dangerous, might be a good idea to make the jump to a CompleteableFuture?

[boostchicken]

I went and reviewed this class, there are some serious style and thread safety issues here.....

• Using Arrays.asList as opposed to Collections.singletonList
• Auto-boxing issues, specifically around booleans
• Lack of Optional use to avoid NPEs
• Lack of exponential back-off/splay
• The caches are hardly thread safe and probably a good candidate to move to something like Caffeine.
• No eviction for cache / cleanup or specified parameters around it (also why Caffeine might be a good move). This will lead to massive run away memory utilization.
• Overall there is no configuration on how many threads can be created or ran, that should be configurable. If I only want 10 threads handling this I should be able to have Executor pool and a
• Lock to make sure I don't just overrun my server. The child API calls that this creates should also execute in the same Thread context, not in a new one
• The generic usage of streams here have other threading consequences and possible unintended behavior as relates to its parent object
• The usage of streams and traditional for loops depending on the event present a big style / predictability problem
• This is very much not OOP/Java coding style, we are checking specific cases by hand instead of delegating to a thread pool and/or Strategy pattern for an arbitrary number of enabled events. As more events are supported this code will become unreadable and as it stands, near untestable
• Does not take into account Twitch limits on listeners and connections from a single server / IP and leads the user into many failure states easily.
• The usage of Exception's printStackTrace function should be delegated to Slf4j for evaluation
• The usage of LocalDateTime in general is a bad idea, deal in UTC. Multiple implementations of this in different timezones will lead to different results.

This should be a candidate for a large scale refactor. Enabling this feature could lead to many unintended consequences on API limits and JVM issues on a large scale and should not be considered anything more than a "demo" of what is possible.

[PhilippHeuer]

@boostchicken thanks a lot for the review again, i'm making this a checklist to work on as all points are valid.