Android-cleanarchitecture: Unsubscribe in onDestroy()

[shchurov]

I think the code hides a potential issue that might appear with a minor change to the requirements.
It calls Butterknife.unbind() in onDestroyView(). Also it calls view methods from Subscriber and unsubscribes in onDestroy(). If one of Subscriber's callbacks is called between onDestroy() and onDestroyView() we will get a NPE.
I think it's not possible to reproduce the issue in this particular sample. Howerver, in a more complicated application that is using fragments' backstack it might be a problem.

[Trikke]

you are welcome to push a PR for these matters.

[shchurov]

@Trikke I wanted to know first if the author had any reasons to implement it like this

[android10]

I think the code hides a potential issue that might appear with a minor change to the requirements.

My requirements were those so I didn't want to overthink here. Also there is no NPE.

I think it's not possible to reproduce the issue in this particular sample. Howerver, in a more complicated application that is using fragments' backstack it might be a problem.

If we cannot reproduce the problem, it does not make sense to fix something that does not exist right? If you can give an example, maybe we can work it out. Also as @Trikke suggested, any PR is very welcome.

[shchurov]

@android10, I believe that it's a complicated issue that must be discussed before any PR.
People use this repository as a base for their own projects and it would be good to keep it as general as possible.

The easiest way to have NPE that comes to my mind is to use fragments-backstack (sometimes you just have to use it):

1. Open fragment
2. Subscribe to an observable that is performing a web-request
3. Open another fragment and put the current to the backstack.
4. Now the system can call onDestroyView() in the first fragment. If so, Butterknife.unbind() is called inside it and all views' references are null now.
5. The web-request is completed and onNext() is called. It wants to update the views but they are null. NPE is thrown.

I believe there're other situations when onDestroyView() is called not followed by onDestroy(), e.g. detaching the fragment manually. So in general you can't rely on the fact that onDestroyView() and onDestroy() are always called together.

If this architecutre is designed to be used only with "one activity - one fragment" approach, then it would be great to write it somewhere.

[luizfp]

@shchurov I agree!

[android10]

@shchurov I understand, that is why PRs are welcome. This example was not meant to cover everyone's needs and approaches, it was mainly for learning purpose and discussions. For instance if you consider there is a path for improvement, I encourage you to move forward and send a PR so we can review it. :smiley:

[Trikke]

i'll just make a small comment here, unrelated to this possible PR. But it's something i've seen come up a lot on this issue tracker.

I'd like to point out that you shouldn't base your own projects entirely on this repository. By which i mean that this repository should be seen as an example from @android10 on how to implement the _Clean Architecture_ principles into your own work. I think it's wrong to go about copy-pasting this repository, remove some of @android10's app logic that you don't need and move on with your own code from there.
Of course this example repository won't cover everyone's use cases and every edge case out there. And i doubt the creator has the time to factor in ever little detail. It is just an example to showcase the architecture, not some holy codebase that screams "this is how it is done and stick to it".

Your own implementation and codebase could (and probably should) end up to be a whole lot different depending on your own scope. But the overarching principles should be the same. If you create your own project based on these, i should be able to understand it immediatly. But it could incorporate entirely different things and take care of edge cases that matter to you.

edit : it seems @android10 was 10 minutes quicker to answer with basically the same idea as my larger rant :smile: