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

Keep arguments order as declared #25

Open
Yarikx opened this issue Jul 3, 2015 · 16 comments
Open

Keep arguments order as declared #25

Yarikx opened this issue Jul 3, 2015 · 16 comments

Comments

@Yarikx
Copy link

Yarikx commented Jul 3, 2015

Sometimes when there 2 or more required arguments, the order of generated builder constructor parameters differs from the order of it's declaration in fragment.
This is specially annoying when Fragment has 2 or more same type args (like String, String, String),
so you forced to build and generate Builder, just to identify arguments order.

@sockeqwe
Copy link
Owner

sockeqwe commented Jul 3, 2015

The parameters should always be in alphabetic order (field names), isn't it?

@Yarikx
Copy link
Author

Yarikx commented Jul 3, 2015

Hm.. You are right. At least now I know the rules how args are ordered)

But what is the point of forcing order to alphabetic?
As creator of API for creating fragments, I want to specify order, e.g. put important arguments in first place, like object id.

@sockeqwe
Copy link
Owner

sockeqwe commented Jul 3, 2015

In annotation processing there is no guarantee that you get the list of fields in the same order as you have declared them in your source file. As far as I know that is a compiler implementation detail. i.e. Eclipse JDT order them alphabetic. There are some workarounds and hacks to get the original declaration order like https://github.com/google/auto/blob/master/value/src/main/java/com/google/auto/value/processor/EclipseHack.java

By ordering them, I can guarantee that I always have a predictable result and to me the order of parameters is not that important as long as parameters are named correctly.

@Yarikx
Copy link
Author

Yarikx commented Jul 3, 2015

Interesting point, didn't know that.

Of course this can be fixed with explicit parameter like 'order' or 'index' that user can add to Arg annotations. What do you think?

@sockeqwe
Copy link
Owner

sockeqwe commented Jul 3, 2015

that would be a possible optional feature, but does order really matters that much? How often do you change or add arguments to a fragment? From my point of view there are not many people having trouble with the order.

@Yarikx
Copy link
Author

Yarikx commented Jul 3, 2015

Not so often.
I'm not saying this is big trouble, I would rather say that it would be nice feature to avoid confusion with multiple arguments (specially same type arguments).

If you think that such feature would be overengineering you can close this Issue.

@sockeqwe
Copy link
Owner

sockeqwe commented Jul 3, 2015

I will keep it open, if more people have interest in such a feature then I will consider to implement it.

@ersin-ertan
Copy link
Contributor

Since there is no guarantee of ordering by default, I would consider extra annotation params superfluous, and am ok with current alpha order.
However, I order my fields by priority, thus isLayoutTablet is a required arg and is near the top of my required field ordering, the required = false args are in the section below the required fields.

By having lots of child fragments inheriting args and providing their own required/not arguments, fragment construction becomes a mix and match when viewed from an initializing class. Working on a team/with consistent apis @Yarikx, each person working on a child fragment benefits in readability from preserving order based on initialization, allowing you to build on params for complex fragments.
Plus if you choose to refactor the name of the argument in the Fragment class, then all of your points of construction will require you to rearrange the arguments.

// ordered example: Frag06 extends Frag05 extends Frag04... 
new Frag06Builder(constructionId, isLayoutTablet, isBackStackable,
Frag04ReqArg, Frag05ReqArg, Frag06ReqArg).build();

// otherwise if using alpha ordering, inherited Frags mess the constructor cumulatively to:
new Frag06Builder(constructionId, Frag04ReqArg, Frag05ReqArg,
Frag06ReqArg, isBackStackable, isLayoutTablet).build();

@sockeqwe
Copy link
Owner

I understand your problem, but does your suggestion solves your problem?

new Frag06Builder(constructionId, Frag04ReqArg, Frag05ReqArg,
Frag06ReqArg, isBackStackable, isLayoutTablet).build();

From my point of view the problem is still the same: You have to set the required parameters according the alphabetical order and then the optionals in alphabetical order. You just have divided it into two pieces that you have to order (first the requires, then the optional arguments). Your example is misleading since you have used variable names that makes your sample look as a good solution.

Update: I had an error in reasoning, optional arguments are not part of the constructor

The problem with changing the order of arguments is that it will break backward compatibility. If you would use the newest version of FragmentArgs with changed argument order in an already existing app that have used a previous version of FragmentArgs you have to check your whole app for changes of arguments order. It's even harder to detect changes if your arguments are of the same type i.e. FragBuilder(String a, String b); to FragBuilder(String b, String a) since you don't get compiler errors.

Hence I would consider changing arguments order only for a very very good reason.
Keeping the order of declared fields may be one. If you (and your team mates) use oracle javac then we could assume that the ordering of args can be realized according the ordering as declared in source file (and i think open jdk works correctly as well). I guess that could be assumed (who is still programming in eclipse?), but what about inherited args. Should I add them add the end? in Front? Is the majority more happy with that solution?

Hence I still believe that an alphabetical order is the best way.

We could start to discuss about other solutions like adding a integer property to the annotation to manually influence the order like this: @Arg(order = 1) , @Arg(order = 2) and then FragmentArgs would first respect the order property and order the rest according alphabetical order. In that case it wouldn't break backward compatibility, but is that the best solution? (i.e. is that approach suitable for inherited arguments? What if two argument's have same order number? etc.) As already said, if we consider to change arguments ordering then only for the "best" (accepted by majority) solution.

However, from my point of view this problem is something specific to you and your company.

@ersin-ertan
Copy link
Contributor

I did not quite understand the first part of your post, apologies, my suggestion was the first ordered example, the second was an illustration of what could happen with each inherited iteration offering a mix of required variables that start with a, b, c...

Besides that, I agree with your point on backwards compat, and @ Arg(order) being a poor approach for inherited arguments for the same reason AnnotatedAdapter explains "...is that the view holders @arg(order = 0) constant integer value must be unique along the inheritance tree." which too me, is self defeating if you have to manage ordering manually(for FragmentArgs case).

I hope that clarified my perspective.

@Gautham
Copy link

Gautham commented Aug 31, 2015

Just an idea, wouldn't a Fluent Interface for non-optional arguments as well work here ? Ordering is no longer an issue although the code slightly increases. But I would prefer the extra code over the chore of reordering whenever I rename or change a parameter. Adding an extra check at runtime to make sure all the arguments are set would solve all the issues.

@sockeqwe
Copy link
Owner

@Gautham jep, I also thought about such a solution ... At least from my point of view a big issue is that you loose "compiletime" evaluation because you do the check if all required arguments are there at runtime. Imho this is even more problematic since you change the argument, i.e. in a base fragment class and all your app build but you have to verify that all fragment extending from that base fragment class still work at runtime.

@madki
Copy link

madki commented Nov 10, 2015

Actually I think it's possible to get the order in which the fields are declared. Instead of getting the elements annotated with @Arg directly, if we obtain the element annotated with @FragmentWithArgs and use getEnclosedElements() it returns the elements in the order defined in the Class.
That's what I understood from this documentation
The we can check if the element has @Arg annotation.

I suppose there might be some overhead using this approach if the file is very big and has many elements.

@sockeqwe
Copy link
Owner

That's true nowadays. FragmentArgs was build a few years ago when the majority of android development took place in Eclipse IDE. Eclipse used its own java compiler, where this wasn't true.

However, there are also some other things to consider like inheritance i.e. a FragmentA extends FragmentB and FragmentB has @Arg as well. How to order in that case? First FragmentA's arguments then FragmentB's ones ?

In my opinion alphabetic order is still the easiest and most predictable / understandable way

@madki
Copy link

madki commented Nov 10, 2015

Option can be provided for choosing the behaviour in @FragmentWithArgs I think. Or you can fix a default like super class fields always come first.

The only problem with alphabetical ordering is refactoring is an issue. Whenever the name changes so that the order changes the FragmentBuilder's arguments need to be changed. And at times this might not even throw an error when both interchange arguments are of the same type.

@sockeqwe
Copy link
Owner

Unfortunately that's true. But could also happen if you change the order of your fields during refactoring (but that seems to be more unlikely than renaming a field)

Configureable through annotation processing options (project wide) would be an option to specify how the arguments are sorted.

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

No branches or pull requests

5 participants