search

enpassio / Databinding

Various examples on databinding which can help not only understand how to get started with it but it also shows how this can be used in complex and real use cases
Apache License 2.0
26 stars 14 forks source link

databinding newsapi - java #32

Closed OyaCanli closed 5 years ago

OyaCanli commented 5 years ago

Basically, I moved the methods in NetworkUtils class into the asynctask in the repository and added a livedata to keep track of connection status. Since we already check connection in our makehttprequest method, I just set the value to livedata there inside the asynctask: https://github.com/OyaCanli/Databinding/blob/dev-databinding-newsapi/DatabindingWithNewsApi/app/src/main/java/com/enpassio/databindingwithnewsapi/repository/NewsRepository.java#L161 This way we avoid adding an extra http request with an extra asynctask. Although putting all those methods inside repository made the repository a bit crowded! Tell me what you think.

I have also added the same enum for facilitating uistate switches in java.

OyaCanli commented 5 years ago

@rajtheinnovator Thanks for reviewing. It is certainly not polished, I can still extract a part of code in utils. I just wanted to ask your idea about the solution: I mean solution for making a real internet connectivity check. And I see you accepted and merged both kotlin and java versions, but they are diferent for the moment. So if you tell me which one seems better I'll change the other accordingly. The difference is as follows: kotlin version check only for network availability. It may still fail in that rare case of "network available but not internet". I'm not sure if it will crash or simply return no result without showing no network error warning. I'm not sure how that extension function URL.readText() works under the hood. Java version checks for availability of internet connection and returns a warning according to the result of http request. But it is quite verbose.. Since we don't use a library and use raw http request..

rajtheinnovator commented 5 years ago

@OyaCanli I've looked into both of them and first thing, they're working fine. However, I think the Kotlin version is better, because we're not waiting to hear the response of network request to update user on the status. However, basically we're still interacting with Repository in some way but as the network part should be better interacting with user before making any interaction with repository classes so maybe we can move this part to ViewModel. What do you think? Should we be following this Google IO style here? https://github.com/google/iosched/blob/d8bc48b20c132b519879eda3a8e9af120519e159/mobile/src/main/java/com/google/samples/apps/iosched/ui/sessiondetail/SessionDetailViewModel.kt#L389

OyaCanli commented 5 years ago

The sample is interesting. I'll examine it a nit. Thanks for sharing!

rajtheinnovator commented 5 years ago

Sure thing, @OyaCanli . Let me know what you think of it and what strategy you plan ahead. I'd like to take a look into that

OyaCanli commented 5 years ago

@rajtheinnovator Ok. Let me first correct a point: In java version, it doesn't (necessarily) wait for response of request for the result of connection status. It first checks network availability and if there is no network, already shows error message. But if there is network, it goes on with http request and according to the response code, if there is no internet, it shows no internet warning. So there is a double-check. I thought of this solution, because while I search for pinging internet for real internet availability, many of the examples were making a http request to google, which requires a background thread of course.. So I thought may be instead of making two http requests in two different asynctasks we can observe the error case in our existing http request.

The Google sample is interesting but frankly it's overwhelming! I guess I need to study Uncle Bob's Clean Architecture first to understand this. In the part you showed they simply check network availability like we had done before. But as far as I understand they are also keeping track of other network errors. I don't fully understand those UseCases for now. For our sample, moving the network check to viewmodel is no problem. But let us first clarify: do we want a real internet connection check (even if it may more or less complicate our code)? or do we settle for simple network availability check?

rajtheinnovator commented 5 years ago

Looks like I overlooked that part, @OyaCanli . I got it now, so then I'd say, the Java version looks more complete. Regarding the background thread, we can use some executors there and that should help.

Yeah, that Google sample is awesome and needs some understanding of clean architecture but I think that too is not the best possible code, simply because there is no best code. It's relative and varies according to our need. For sure we should however look into it as it has a more in depth example of production code.

For this sample, I think we can skip the pinging part/real internet check. Let's do it in a real complete sample which is production ready.

OyaCanli commented 5 years ago

Ok, good. Then it's not gonna be that difficult. Then kotlin version is mostly fine. I'll just move the network check logic to the viewmodel instead of fragment. (That was what you proposed, right?) @rajtheinnovator Once kotlin version is done, I'll also change java version accordingly.

rajtheinnovator commented 5 years ago

Yeah, that's what I meant @OyaCanli :+1: And let me know if you find something interesting out of that Google IO sample, I'd like to learn more things :smile: