search

ustwo / android-boilerplate

Android Boilerplate à la ustwo
https://ustwo.com/
Apache License 2.0
42 stars 6 forks source link

`on` prefixes #74

Open Migge opened 7 years ago

Migge commented 7 years ago

I see a lot of on prefixes which implies that they are callbacks, when they are not really callbacks but instructions. Best example I could find was

BasePresenter.onViewAttached(T view)

This is not really a callback, it's in fact a setter. It's an instruction to the Presenter to attach the view. A traditional attachView() name seems more appropriate.

Of course not a big deal, and mainly philosophical :-) Thoughts? I find it confusing. There are other similar, but more borderline, examples such as onInject() and onInitialize() on the BaseFragment, which also feel more like instructions rather than callbacks.

An opposite example is onViewWillShow(). That is an actual callback, so there the naming is fine.

mshipton commented 7 years ago

onX in the BasePresenter represent the boilerplate's lifecycle and follow naming conventions of other lifecycle events in Android like View.onAttachedToWindow or Fragment.onActivityCreated.

onViewAttached is essentially a setter, but one that only occurs at certain times (and with rules that bound when/how often it can be called to preserve correct lifecycle).

Migge commented 7 years ago

Regarding the onX in BasePresenter, yeah, I suppose it's fine with the "representing the lifecycle" argument, even though I'm not 100% convinced because of the very examples you had. onAttachedToWindow() is a callback, called when the View is attached to window. onActivityCreated() is a callback, called when the Activity is created. Hence the on prefix, that is what that prefix is used for, normally.

That's why I especially don't like onViewAttached(). That name implies that it's a callback that's called when the View is attached, which it is not. For sure, there is some bias on my side in that attachView() has been a much more common name in my experience in MVP implementations, but even in the general case I think the on prefix is misleading.

At the same time, admittedly there are other cases in the Android framework, like onCreateView(), which for sure is asking the implementer to create the View and return it, and it's not bugging me. So when I think more about it now, it could just be the wording used, the past tense in onViewAttached(), that is bugging me. The past tense implies that it's already attached and letting the client know about it. So perhaps onViewAttach() would be more consistent and a compromise.

But that's enough from my side, at least that were my 2 cents :-) You can close this issue if you do not intend to change anything.

/Migge

mshipton commented 7 years ago

Good point about the tense. So onViewAttach / onViewDetach?