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

refactor: fix code inspection manual array to collection copy #2224

Merged
merged 10 commits into from
Jul 18, 2018
Merged

refactor: fix code inspection manual array to collection copy #2224

merged 10 commits into from
Jul 18, 2018

Conversation

zielint0
Copy link
Contributor

@zielint0 zielint0 commented Jul 15, 2018

This is extracted from #2222

I am ready to merge.

for (Object in : input) {
this.inputs.add(in);
}
Collections.addAll(this.inputs, input);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using the static call instead of using this.inputs.addAll(input)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Motivation: use explicit and readable way to add all elements. The foreach is worse because developer must analyze it deeply and longer. And older loop (int i = 0; i < 10; i++) is the worst because it is slower to read and more error prone.

In other words: if it is possible to perform mass/one time action for all elements - do it. Avoid processing elements in loop one by one.

Hierarchy of processing elements (the lower, the more error prone and longer)
1 one call (addAll)
2 foreach (internal iteration)
3 for / while (external iteration)
4 manual processing

Copy link
Contributor Author

@zielint0 zielint0 Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://m.youtube.com/watch?v=15X0qFtBqiQ
Use abstraction, high level mechanisms. Let the compiler do the dirty job. Exploit modern Java constructs.

Copy link
Contributor Author

@zielint0 zielint0 Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think that static call shows what is added to what in a clearer way.
But of course both styles work :)
The main point is to use addAll(), not looping.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with using addAll instead of the loop: my question was only about why not using the invocation from the instance directly.
IMHO it's more readable than calling Collections.addAll with two arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to refactor from Collections.addAll() to this.input.addAll(input), but second variant cannot be compiled because of argument type which is: addInput(Object... input)

Collections.addAll() is:
addAll(Collection<? super T> c, T... elements)

this.input.addAll() is:
addAll(Collection<? extends E> c)

The exact mismatch in this.input.addAll(input) is:

addAll in List (java.util.Collection<?>) cannot be applied to (java.lang.Object[])

In other words: Object... input is an array, so it can be used only in Collection<? super T> c, T... elements)

So the only way in this context is static method Collections.addAll().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for this one then. But for the other ones you could have use the form instance.addAll() no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used explicit and simple way in all cases: x.addAll(Arrays.asList(y)) :-)
No need for Collections.addAll()
I am ready to merge.

@zielint0 zielint0 changed the title refactor: fix code inspection manual array to collection copy Wip: refactor: fix code inspection manual array to collection copy Jul 16, 2018
@zielint0 zielint0 changed the title Wip: refactor: fix code inspection manual array to collection copy refactor: fix code inspection manual array to collection copy Jul 16, 2018
@zielint0 zielint0 changed the title refactor: fix code inspection manual array to collection copy WIP refactor: fix code inspection manual array to collection copy Jul 17, 2018
@zielint0 zielint0 changed the title WIP refactor: fix code inspection manual array to collection copy refactor: fix code inspection manual array to collection copy Jul 17, 2018
@pvojtechovsky
Copy link
Collaborator

@surli, I prefer instance.addAll(Collection) too, but when the argument is not an Collection but an array, then we have two ways:
A) x.addAll(Arrays.asList(y))
B) Collections.addAll(x, y)

I vote for the B), which is nicer from my point of view in the case of working with array. WDYT? So the original implementation of Tomasz was better then the current one, which we convinced him (by mistake?) to implement. ;-)

@surli
Copy link
Collaborator

surli commented Jul 18, 2018

My mistake: I though that except the first change, the other ones were about adding list in list, not array in list: that was my original question. So in this case, yeah I agree its better using Collections.addAll sorry for the bothering @zielint0

@zielint0 zielint0 changed the title refactor: fix code inspection manual array to collection copy WIP refactor: fix code inspection manual array to collection copy Jul 18, 2018
@zielint0 zielint0 changed the title WIP refactor: fix code inspection manual array to collection copy refactor: fix code inspection manual array to collection copy Jul 18, 2018
@pvojtechovsky pvojtechovsky merged commit 2b23448 into INRIA:master Jul 18, 2018
@pvojtechovsky
Copy link
Collaborator

Thank you Tomasz

@zielint0
Copy link
Contributor Author

@pvojtechovsky

Thank you Tomasz

Děkuji za dobré slovo!

@monperrus monperrus mentioned this pull request Sep 20, 2018
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants