Finding local variables only used once

Sometimes I find code where a local variable is declared and then only
used once. In some cases inlining the variable would result in complex
expressions and make the code more difficult to read, but there are also
cases where the variable used to be used in many places throughout the
code and where this is no longer the case, and where inlining the
variable would result in more readable code.

A case in point: Replacing a plain "for" construct with "foreach".

final List list = foo();
for (int i = 0; i < list.size(); i++) {
Object item = list.get(i);
...
}

--->

final List list = foo();
for (Object item : list) {
...
}

---> often more readable as follows:

for (Object item : foo()) {
...
}

Is there an inspection to catch such cases, or should I submit a feature
request?

4 comments

Funny you should mention that. I was in the process of debugging a "Redundant local variable" inspection. The trick with this one is going to be figuring out just which local variables should be deleted. The "only used once" criterion seems far too broad. There are plenty of cases where intermediate values used once greatly improve readability. Two cases I've implemented so far are local variables which simply copies of other local variables, and local variables which are immediately returned/thrown. I could probably add local variables which are immediately assigned from and then not used. Other than that, I'm not sure of a good heuristic.

--Dave Griffith

0

Dave Griffith wrote:

The trick with this one is going to be figuring out just which local
variables should be deleted. The "only used once" criterion seems
far too broad. There are plenty of cases where intermediate values
used once greatly improve readability.


Yes, I definitely agree there. I only considered the "only used once"
criterion because I couldn't come up with a better one that would still
match my use case without being too complex or specific. Going through
all local variable declarations that are only used once would take a lot
of time, but it would beat going through all local variable
declarations in the entire code base!

I could probably add local variables which are immediately assigned
from and then not used. Other than that, I'm not sure of a good
heuristic.


Possibly some kind of line length heuristic? Trigger the inspection
when inlining the variable would result in a line which is shorter or
only slightly longer than max(old declaration line length, old usage
line length)?

final List list = complexMethodCall(someParam, someOtherParam);
for (Object item : list) {}

for (Object item : complexMethodCall(someParam, someOtherParam)) {}

==> only slightly longer.

But maybe this heuristic is too rough. You'd really want to have some
other measure of complexity and readability than line length...

0

Perhaps running one of your complexity metrics on the inlined result could
give you a benchmark to decide whether to recommend inlining the variable
or not.

Tobin

Funny you should mention that. I was in the process of debugging a
"Redundant local variable" inspection. The trick with this one is
going to be figuring out just which local variables should be deleted.
The "only used once" criterion seems far too broad. There are plenty
of cases where intermediate values used once greatly improve
readability. Two cases I've implemented so far are local variables
which simply copies of other local variables, and local variables
which are immediately returned/thrown. I could probably add local
variables which are immediately assigned from and then not used.
Other than that, I'm not sure of a good heuristic.

--Dave Griffith



0

Getting closer, I think. The current "redundant local variable" implementation detects three issues:

1)The local variable is immediately returned or thrown.
2)The local variable is immediatly assigned to another variable, and then never used.
3)The local variable is assigned from another local variable or method parameter, and from that point on neither of them are altered during the scope of the variables existence.

These seem always to be safe. I'm thinking of adding another rule.

4)The variable is only used once, immediately after it is declared, and inlining it would not result in any of the following:
-a chained method call
-a nested method call
-an operator expression containing more than three terms.

Thoughts?

--Dave Griffith

0

Please sign in to leave a comment.