Code is pretty highly-coupling, convoluted in the responsibilities

[sleepylee]

• There are multiple things that lead to this comment, but here are a few of them:
  • ViewExt is holding a lot of things outside of its scope, it is responsible for holding the observers and defining the conditional behaviors.
  • BaseViewModel is holding the networking error handler from what they can suppose to be handled by another handler from a different layer.
  • I'm not pretty sure why would we place getInitialLoadRequest or getItemsPerRequest at the Application level, what could happen later when we extend the application and having more such configuration fields? Also the getApiEndpoint, it's normal that an application might need to be pointed to another API endpoint during the development period, with this current implementation it doesn't seem to be ready for that.

[hoang06kx1]

• ViewExt or ViewExtension is intended to be used as a plain utility place to contain all extension functions for the "View" in general: Activity, Fragment, CustomView...
• This app's feature is all about rendering API responses and my chosen architecture is heavily based on ViewModel. It came to me quite naturally to put all the network-related handler into a base ViewModel. Would you please suggest which layer I should put the network error handler?
• My bad decision. But it came from a hidden problem can not be solved in a short time: the Koin Dependency Injection did not work as expected when I set up the test dependencies. So I can not change those two constants value when creating the test. I don't want to abuse Reflection. The same reason why you see the getApEndpoint in Application class: I must use the technique here to swap Application class when running test. TestApplication has the API endpoint point to the MockWebServer endpoint and SurveyAppliation point to the real API endpoint.
  I did not have enough time to switch back to Dagger2. But my workaround solution seems to shoot into my foot this time.

[sleepylee]

hi @hoang06kx1 ,

• Putting more logic to the ViewExt it means harder to cover it with testing imo...or easily people might forget it because it doesn't appear in the main targeting test class (say, for e.g, the viewModel.loading part from your code that's been used in the ViewExt).

• Since you're using RxJava adapter for Retrofit as well, I'd recommend you to take a look at lift() operator, and create an ErrorHandlerOperator that can be used at your Repository layer.

  • The idea is to guarantee that all the network requests are being made from the Repository layer, will be covered once they are back with either a Positive Response or a Throwable.
  • At the center of your Operator, you can get intermediate callbacks like onNext(), onSubscribe()... to define what should be passed next to the Observer.
  • Once you meet an error, you can parse it (with a typical separated ErrorParser) and pass it to the observer's onError to handle it. And consider a different/customized type of Exception to wrap it as well?
  • If it is a positive response, just pass the body's response to the onNext?
  • There might be more different ways to handle it for sure, but just an idea, as we can always share the responsibility somewhere else, so how about to where the idea starts? 😄 Repository is where we're managing the network calls and returning the response to ViewModel right?

• We all love the innovation, thrive for using the latest technology to make our life easier and making our product nicer, we understand you well, Hoang. But sometimes there is a drawback once the solution is not well tested, validated and pushing to Production could cause huge consequential damage to the whole team's productivity. We hope you got this lesson well.

[hoang06kx1]

Yah as you said, I got my lesson.
It's interesting to know your point of view, I believe I gain a better taste of code organization now.
Thanks for your warming and positive words, too. 😄