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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
47f60e7
refactor: move Main to testclasses - second try
zielint0 Jul 12, 2018
bd7ef7a
Revert "refactor: move Main to testclasses - second try"
zielint0 Jul 12, 2018
41fd248
Merge branch 'master' of https://github.com/INRIA/spoon
zielint0 Jul 13, 2018
d7220ba
Merge branch 'master' of https://github.com/INRIA/spoon
zielint0 Jul 15, 2018
136e66b
refactor: fix code inspection: performance: manual array to collectio…
zielint0 Jul 15, 2018
ce9d9dc
refactor addAll()
zielint0 Jul 17, 2018
0e7f84e
refactor addAll()
zielint0 Jul 17, 2018
68911e0
Fix import
zielint0 Jul 17, 2018
936981a
Restore Collections.addAll()
zielint0 Jul 18, 2018
d8b4ea8
Fix imports
zielint0 Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/main/java/spoon/reflect/visitor/chain/CtQueryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,7 @@ public CtQueryImpl addInput(Object... input) {
this.inputs = new ArrayList<>();
}
if (input != null) {
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 •
edited
Loading

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 •
edited
Loading

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.

}
return this;
}
Expand Down
13 changes: 4 additions & 9 deletions src/main/java/spoon/support/util/RtHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand All @@ -57,9 +58,7 @@ public static Field[] getAllFields(Class<?> c) {

private static void addAllFields(Class<?> c, List<Field> fields) {
if (c != null && c != Object.class) {
for (Field f : c.getDeclaredFields()) {
fields.add(f);
}
Collections.addAll(fields, c.getDeclaredFields());
addAllFields(c.getSuperclass(), fields);
for (Class<?> iface : c.getInterfaces()) {
addAllFields(iface, fields);
Expand Down Expand Up @@ -89,9 +88,7 @@ public static Method[] getAllMethods(Class<?> c) {
getAllIMethods(c, methods);
} else {
while (c != null && c != Object.class) {
for (Method m : c.getDeclaredMethods()) {
methods.add(m);
}
Collections.addAll(methods, c.getDeclaredMethods());
// methods.addAll(Arrays.asList(c.getDeclaredMethods()));
c = c.getSuperclass();
}
Expand All @@ -101,9 +98,7 @@ public static Method[] getAllMethods(Class<?> c) {
}

private static void getAllIMethods(Class<?> c, List<Method> methods) {
for (Method m : c.getDeclaredMethods()) {
methods.add(m);
}
Collections.addAll(methods, c.getDeclaredMethods());
for (Class<?> i : c.getInterfaces()) {
getAllIMethods(i, methods);
}
Expand Down