Skip to content
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

Naming of methods and constructors #585

Closed
HosseinYousefi opened this issue Aug 14, 2023 · 4 comments · Fixed by dart-archive/jnigen#362
Closed

Naming of methods and constructors #585

HosseinYousefi opened this issue Aug 14, 2023 · 4 comments · Fixed by dart-archive/jnigen#362

Comments

@HosseinYousefi
Copy link
Member

Java classes can have multiple (unnamed) constructors. Currently we name the other constructors as ctor1, ctor2, ... . I propose changing the names to new1, new2, ... .

Rationale: Currently if we have multiple Java methods with the same name like foo, we name them as foo, foo1, foo2, ... meaning we could write:

final foo = clazz.foo;
final foo1 = clazz.foo1;
// ...

Similarly we could

final constructor = clazz.new;
// but currently
final constructor1 = clazz.ctor1;
// after this change
final constructor2 = clazz.new2; // similar to clazz.new

@dcharkes @mahesh-hegde @lrhn Any better idea for naming multiple constructors / methods instead of the added number? Do you like ctor or new better?

@lrhn
Copy link
Member

lrhn commented Aug 14, 2023

If you have foo1, foo2, etc. for methods, then new1, new2, ... seems reasonable for constructors.

The next question is which constructor gets to be just Foo.new. If the Java class has a zero-argument constructor, it's the obvious choice, but if it doesn't, should there be no Foo.new constructor in the Dart API? (Because users might start assuming that you can alway do new Foo(), if it's almost always possible.)

An alternative could be going by number of arguments, and have .new1 for a one-argument constructor, new2 for a two-argument constructor, and then go to new1a, new1b if there are multiple ones. For a few, fairly distinct constructors, that might be useful. If there are many constructors with the same number of arguments, it's just more verbose than new1, new2, etc.
And it's different from what you do for methods, so if you do something else, you should probably do it everywhere.

(Would you worry about sorting in APIs if you get above new10? Would a letter be better, or at least postpone the problem to when you get above new2z?)

A completely different approach could be to try to derive a name from the parameter type, like Foo.fromInt, Foo.fromIntxInt. That may get very, very verbose for long parameter lists, at which point Foo.new17q starts looking reasonable. Again, only makes sense if you try the same for methods.

I definitely don't like ctor. Dart style guide says to avoid abbreviations. I also think constructor is too damn long.

The new1 is both close to something meaningful, and weird enough that it's clearly not trying to be a Dart name.
I'd go with that.

@HosseinYousefi
Copy link
Member Author

I definitely don't like ctor. Dart style guide says to avoid abbreviations. I also think constructor is too damn long.

Agreed.

An alternative could be going by number of arguments, and have .new1 ...

That would create a lot of unnecessary leading digits for constructors and methods that otherwise don't even have a same-named counterpart.

A completely different approach could be to try to derive a name from the parameter type, like Foo.fromInt, Foo.fromIntxInt. That may get very, very verbose for long parameter lists.

Yes, especially since Java is not known for short parameter type names either: Foo.fromAbstractBarBuilderFactory!

@HosseinYousefi
Copy link
Member Author

One general problem with numbers is that they can move around when for example the new version of the library comes with added methods. And users must update their code to reflect this change. We even had this issue before: #696

One way of fixing this is to use some hashed version of argument names at the end of the method name. This is 1. ugly and 2. Requires ALL methods to end with this hash.

@dcharkes
Copy link
Collaborator

In FFIgen we've also gone with the numbering, so far we've not had too many issues with renumbering there. Though it might be that it's more common in Java libs?

A completely different approach could be to try to derive a name from the parameter type, like Foo.fromInt, Foo.fromIntxInt. That may get very, very verbose for long parameter lists, at which point Foo.new17q starts looking reasonable. Again, only makes sense if you try the same for methods.

This is what I use in the generated tests for FFI as well. But it's ridiculously long method (and in that case class) names. Those long names really only work in that situation because all the call-sites are generated as well.

Not sure if hashes are better or worse. 🙊 But at least both approaches are stable.

Maybe some kind of compromise is the name-postfix / hash from the moment we see two method names. In that case there's only ever 1 migration for users, instead of multiple as with renumbering.

A completely different approach would be to read in the yaml file that we generate, and use that to keep the renumberings from jumping around. (However, that makes JNIgen unstable if it is run on the same API in a clean project later...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants