-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Code health] Rename on*
functions
#2439
Comments
I saw a related ToTT about naming recently, and I partly disagree with the idea that naming handler methods as such is necessarily a bad thing. Like setters and getters, they're such a common pattern that a consistent naming convention can add clarity. For additional clarity, those methods could call "action" methods that actually do things (eg |
It is a definite tradeoff. The
Contrarily, giving more semantic names doesn't convey the handler intention within the enclosing class, but it gives readers in the layout fragment way more context:
So I think it depends on what we think is more valuable. I'd also argue that the naming scheme overemphasizes a shared property that isn't important. Knowing that all the various action handlers we have are ui handlers doesn't really benefit us since it's rare that we'd ever modify all of these functions based on this shared characteristic, in spite of this, the naming scheme really puts this shared characteristic front and center. I think names with terms like button in them are also problematic because it means we'll need a rename if we ever happen to change the control component we bind the behavior to. Maybe we can get the best of both worlds by adopting a nice suffix instead of a prefix, and by dropping references to buttons etc. What about, e.g. As for the android terminology, I think that's necessary and makes sense given android's role as the platform. I think it makes less sense for application logic to mirror this approach. In fact, listeners are another example where using the Just my 2c, the tott is what inspired this. I'd be happy to drop it if we don't feel strongly, but I did touch the photo fragment recently and felt that it'd have been easier to reason through what was happening if the On.... had more meaningful names. Instead, there were a bunch of |
Ok, I'm convinced: As I read your response I realized I was thinking of event handlers as the primary means of documenting the result of a particular UI interaction. However, with both Compose and XML Data Bindings handlers are already described in a discoverable, non-ambiguous way. In that case adding another level of indirection is unnecessary. The one potential exception I can think of is when an event triggers multiple semi-related actions. In that case we have to option of putting all the effects in the method name ( |
We have some functions throughout the app that are called
on...Result...
on...Click...
and similar. These names are really opaque and do not convey anything about the actual behavior that's happening when the function is called. It'd be better to convey semantics in these names instead of intended/current call site use, e.g.onTakePhoto
-->startPhotoCapture
,onSignInButtonClick
-->attemptUserSignIn
etc.The text was updated successfully, but these errors were encountered: