As discovered in our last session - Opening a lot of connections breaks things.
The creation of the OkHttpClient is done in HackerNewsNetwork#getClient... ugg... A get.
Ruthless refactoring!
As I was saying; the creation is done in HackernewsNetwork#httpClient.
private OkHttpClient httpClient() {
return interceptorObj == null
? new OkHttpClient()
: new OkHttpClient().newBuilder().addInterceptor(interceptorObj).build();
}
Obviously just spinning up a bunch of them.
We need to keep a single client to limit the concurrency.
While I suspect there will be minimal issue if we ignored thread safety. It takes a decent number to spin up before the file count is too large, so we might get away with just checking and setting the value, like so
private OkHttpClient reusableHttpClient() {
if(reusedHttpClient == null) {
reusedHttpClient = httpClient();
}
return reusedHttpClient;
}
private OkHttpClient httpClient() {
return interceptorObj == null
? new OkHttpClient()
: new OkHttpClient().newBuilder().addInterceptor(interceptorObj).build();
}
We'll get some extra objects; but once it's set; it won't get re-built.
How much work is it to not have this edge case? One keyword. Java makes it easy to protect this behavior with the synchronized keyword.
With an easy implementation; I'd advocate using it. It protects us and costs ~nothing. (Ignoring app performance, which is also ~nothing in our context).
This puts our initial method to
private synchronized OkHttpClient reusableHttpClient() {
if(reusedHttpClient == null) {
reusedHttpClient = httpClient();
}
return reusedHttpClient;
}
I don't like the if. It's not a guard clause.
Ruthless Refactor!
private synchronized OkHttpClient reusableHttpClient() {
return reusedHttpClient == null ? reusedHttpClient = httpClient() : reusedHttpClient;
}
This... not QUITE an if. Yes; ternary. Kotlin would be a bit of a pain. Kotlin doesn't have ternary statements; the if is an expression, which allows it to be written out as an if... And back from that tangent.
The amount of boiler plate required to push that if down... not reasonable to do. It's all private.
Our final form looks like
private synchronized OkHttpClient reusableHttpClient() {
return reusedHttpClient == null
? reusedHttpClient = httpClient()
: reusedHttpClient;
}
private OkHttpClient httpClient() {
return interceptorObj == null
? new OkHttpClient()
: new OkHttpClient().newBuilder().addInterceptor(interceptorObj).build();
}
... And to my shame (and the whole 5 minutes doing this) I didn't run the tests... Let's run them.
They all pass. Now let's run the emulator!
... Let's try it w/o docker running and making android studio ANGRY at me.
Also gives me a chance to upgrade to 2.4. What? No update... what am I, on stable... oh... Yep. Sounds boring - TO CANARY!
...
The file structure is ... flat. Not bad; but they went all flat.
OK; back to... what was I doing - Not Docker; right.
The 2.4 Lint really steps up. I have 32 warnings now. Most due to some generics I'm using. I'm fine with that. I added some suppressions for places I'm not sure how it changes things.
Jobs now display and the app doesn't crash.
Parting Thoughts
I don't know the details of pulling a single OkHttpClient like I am. Until it shows a problem; I'm gonna run with it. :)
This is a bit of a quick fix post; but this is the type of thing we'd want to defer until it's shown an issue; and then fix. I don't know if holding the client will cause issues - and until I do know it; I'm doing nothing. Wait until the last responsible moment to do/implement.
Code's here