InspectionGadgets RFEs

Hi,

Maybe it is already there and I cannot find it.

I would like to be able to find:
1) redundant final parameters and
2) parameters in constructors and setters, which are not of the same
name as the field, which they set.

Thanks in advance.

Tom

23 comments

For 1), I'm not sure what you mean. On a parameter, "final" always has semantics, namely that the parameter can't be assigned to. Is the idea to find and remove any "final" marks on parameters that aren't required for compilation (because, for example, the parameter is used in an anonymous inner class)? If so, it's doable, and I'll put it on the list. Hell, even if you meant something else, I like this one, so it's going on the list.

2) is easy enough for setters. The constructor arg matching issue might make that more difficult. For full generality, I'd have to do it modulo field/parameter prefixes/suffixes. In any case, it doesn't currently exist. I'll add it to the list.

--Dave "IDEA + Inspection Gadgets: Software for picky bastards" Griffith

0

Dave have you got an inspection for when the return value of a method is
not used? I'm sure there was one there but I've had a good look through
and I can't find it... my eyes hurt now though, which is nice ;)

Cheers,
N.

Dave Griffith wrote:

For 1), I'm not sure what you mean. On a parameter, "final" always has semantics, namely that the parameter can't be assigned to. Is the idea to find and remove any "final" marks on parameters that aren't required for compilation (because, for example, the parameter is used in an anonymous inner class)? If so, it's doable, and I'll put it on the list. Hell, even if you meant something else, I like this one, so it's going on the list.

2) is easy enough for setters. The constructor arg matching issue might make that more difficult. For full generality, I'd have to do it modulo field/parameter prefixes/suffixes. In any case, it doesn't currently exist. I'll add it to the list.

--Dave "IDEA + Inspection Gadgets: Software for picky bastards" Griffith

0

Doesn't it exist from IDEA directly?

Tom

0

For 1), I'm not sure what you mean. On a parameter, "final" always has semantics, namely that the parameter can't be assigned to. Is the idea to find and remove any "final" marks on parameters that aren't required for compilation (because, for example, the parameter is used in an anonymous inner class)? If so, it's doable, and I'll put it on the list. Hell, even if you meant something else, I like this one, so it's going on the list.


BTW, I know, that some coding styles require to make the parameter final
to avoid modifying the parameter's value, but I prefer shorter parameter
lists (and still avoid to modify the value).

Tom

0

On 2004/02/11 13:22, Thomas Singer (MoTJ) wrote:

>> For 1), I'm not sure what you mean. On a parameter, "final" always
>> has semantics, namely that the parameter can't be assigned to. Is
>> the idea to find and remove any "final" marks on parameters that
>> aren't required for compilation (because, for example, the
>> parameter is used in an anonymous inner class)? If so, it's
>> doable, and I'll put it on the list. Hell, even if you meant
>> something else, I like this one, so it's going on the list.


BTW, I know, that some coding styles require to make the parameter
final to avoid modifying the parameter's value, but I prefer shorter
parameter lists (and still avoid to modify the value).


I would like an inspection that warns about modifying a parameter value
inside a method. Then there wouldn't be the need to mark parameters as
final.

Bas

0


InspectionGadgets already has such an inspection: "Assignment to method parameter" under "Probable bugs".

0

This is a built-in IDEA inspection, although I'm not sure which one. Note that it's not a local inspection, and thus only shows up in the batch inspection tool, not in the on-line errors.

0

Ah, so it should still be in the list along with yours then? How comes
it's not a local inspection? I can't see why it would need to know
anything outside of the scope of the calling method...

N.

Dave Griffith wrote:

This is a built-in IDEA inspection, although I'm not sure which one. Note that it's not a local inspection, and thus only shows up in the batch inspection tool, not in the on-line errors.

0


Got it. That's my preference as well. Since I've already got a "Assignment to method parameter" inspection, removing extraneous "final" modifiers on parameters fits my style beautifully. If I get a seat on the train this morning, I might have such an inspection before I get to work.

--Dave Griffith

0

If I've understood correctly, what you're talking about is an inspection
which finds unused variable assignments (that just happen, in this case, to
be from the return value of a function), or maybe one that finds instances
of calls to functions which ignore the returned value. What Dave is talking
about (I think) is an inspection which checks whether the value that a
function returns is ever used - i.e. whether it could be made into a void
function or not.

Hope that helps,
Vil.

Nathan Brown wrote:

Ah, so it should still be in the list along with yours then? How comes
it's not a local inspection? I can't see why it would need to know
anything outside of the scope of the calling method...

N.

Dave Griffith wrote:

>> This is a built-in IDEA inspection, although I'm not sure which one.
>> Note that it's not a local inspection, and thus only shows up in the
>> batch inspection tool, not in the on-line errors.
>>
>>

--
Vilya Harvey
vilya.harvey@digitalsteps.com / digital steps /
(W) +44 (0)1483 469 480
(M) +44 (0)7816 678 457 http://www.digitalsteps.com/

0

On 2004/02/11 14:13, Dave Griffith wrote:

InspectionGadgets already has such an inspection: "Assignment to
method parameter" under "Probable bugs".


Ah thanks! I didn't notice it (and it's actually under "Potentially
confusing code constructs" for anybody who's looking...). It's really
hard to find anything there. Are there any SCR's for improvements to the
Errors dialog I can vote for?

Bas

0


Ah, I see. Got it. Yes, I was talking about finding whether a method's return value is ever used, not whether it's discarded in a particular case. For the second, no such inspection currently exists, as there would be way to many false positives for it to be useful. The standard libraries are chock full of return values that rarely get used. InspectionGadgets does have an inspection for the particularly bug prone and dangerous case of InputStream.read() having it's return value thrown away. Any other similar cases you know of I would love to hear about.

--Dave Griffith

0

Dave,

What prompted the original question was was pseudo-mutator methods for
immutables e.g. String.concat(); sometimes you get bugs where people
think if they call these methods, the object will be modified, but as
it's immutable it won't. The case I was dealing with was a call to a
homegrown method prependArray() which returns a new array. In both
these cases the return value not being used is a probable bug.

Trouble is that it's impossible to give a general case for this, as
theres no way of automatically knowing which methods make no sense
without the return value being used. The only thing I can think of
would be being able to specify the methods that this should apply to
e.g. via regexp, and have a pre-initialised config that automatically
included the commonly used pseudo-mutator methods from the JDK. You
could then add items for your own pseudo-mutator methods to the config -
these might want to be project specific.

Sound sensible for a new feature?

N.

Dave Griffith wrote:

Ah, I see. Got it. Yes, I was talking about finding whether a method's return value is ever used, not whether it's discarded in a particular case. For the second, no such inspection currently exists, as there would be way to many false positives for it to be useful. The standard libraries are chock full of return values that rarely get used. InspectionGadgets does have an inspection for the particularly bug prone and dangerous case of InputStream.read() having it's return value thrown away. Any other similar cases you know of I would love to hear about.

--Dave Griffith

0


Now that seems useful. More config panel coding than I'm up for right now, but useful. I just want to avoid the case where every call to Map.put() gets flagged, simply because it returns a value that no one cares about (Pop Quiz: what does it return?)

Your request is now on the list.

0

Yeah I completely agree. Cheers.

N.

ps. It's the original object that was mapped to that key. I didn't
peek, honest!

Dave Griffith wrote:

Now that seems useful. More config panel coding than I'm up for right now, but useful. I just want to avoid the case where every call to Map.put() gets flagged, simply because it returns a value that no one cares about (Pop Quiz: what does it return?)

Your request is now on the list.

0

If it's on the list, then can I ask you make sure BigDecimal is on the
list for the pre-initialised config - I've used BigDecimal.negate() the
wrong way one too many times ...

Robbie

0

Dave, it strikes me that maybe you could possibly do this in stages -
initially have an internal config set up for all the useful JDK ones,
then when you feel up to the task you can expose the config via ui to be
(ab)used by the rest of us. Of course, doing the initial config will be
a big chunk of work in itself... but maybe even that could be done
incrementally as suggestions come in.

Thanks for adding it to your list!

N.

Robert Gibson wrote:

If it's on the list, then can I ask you make sure BigDecimal is on the
list for the pre-initialised config - I've used BigDecimal.negate() the
wrong way one too many times ...

Robbie

0


I've got a counter-proposal. The business logic for this is pretty trivial. Indeed, it's likely to just be an abstraction of the already existing "InputStream.read() result ignored" inspection. If it didn't take the config panel, I'd just do it, and it would probably only take an hour. I've also got a couple of other inspections (one very powerful) that also need a "list of regexes" config panel. Alas, my UI design and Swing skills are not really up to the task. If some kind soul would send me code for an appropriate JPanel, I would finish off this inspection and the others, and thank the provider profusely in the release notes and wiki.

I'm also for submitting input on listing of method values that shouldn't be ignored. Entries should be either easy to forget, or dangerous if forgotten (or ideally both). I'll start

InputStream.read()
String.substring()
String.replace()
String.replaceAll()
String.replaceFirst()
String.intern()
String.toLowerCase()
String.toUpperCase()
String.replaceFirst()
Color.darker()
Color.lighter()
Any constructor(?)

0

I could probably do this panel for you, I'd just need to know how you'd
want to interface with it... would you just want to pass the panel an
array of strings (e.g. on construction) and be able to query an array
back out again? Plus just simple validation of the regexes as they are
being added/changed?

N.

Dave Griffith wrote:

I've got a counter-proposal. The business logic for this is pretty trivial. Indeed, it's likely to just be an abstraction of the already existing "InputStream.read() result ignored" inspection. If it didn't take the config panel, I'd just do it, and it would probably only take an hour. I've also got a couple of other inspections (one very powerful) that also need a "list of regexes" config panel. Alas, my UI design and Swing skills are not really up to the task. If some kind soul would send me code for an appropriate JPanel, I would finish off this inspection and the others, and thank the provider profusely in the release notes and wiki.

I'm also for submitting input on listing of method values that shouldn't be ignored. Entries should be either easy to forget, or dangerous if forgotten (or ideally both). I'll start

InputStream.read()
String.substring()
String.replace()
String.replaceAll()
String.replaceFirst()
String.intern()
String.toLowerCase()
String.toUpperCase()
String.replaceFirst()
Color.darker()
Color.lighter()
Any constructor(?)

0

Roughly in order of severity:
1/ Methods that don't quite do what they sound like they should and have
tripped me up (or nearly) at least once
BigDecimal.movePointLeft(int)
BigDecimal.movePointRight(int)
BigDecimal.negate()
BigDecimal.setScale(int)
BigInteger.clearBit(int)
BigInteger.flipBit(int)
BigInteger.negate()
BigInteger.setBit(n)
BigInteger.shiftLeft(int)
BigInteger.shiftRight(int)

2/ Methods that maybe sound like they should modify?
BigDecimal.abs()
BigDecimal.add(BigDecimal)
BigDecimal.divide(BigDecimal,int)
BigDecimal.divide(BigDecimal,int,int)
BigDecimal.multiply(BigDecimal)
BigDecimal.subtract(BigDecimal)
BigInteger.abs()
BigInteger.add(BigInteger)
BigInteger.and(BigInteger)
BigInteger.andNot(BigInteger)
BigInteger.divide(BigInteger)
BigInteger.divideAndRemainder(BigInteger)
BigInteger.mod(BigInteger)
BigInteger.modInverse(BigInteger)
BigInteger.multiply(BigInteger)
BigInteger.not()
BigInteger.or(BigInteger)
BigInteger.pow(int)
BigInteger.remainder(BigInteger)
BigInteger.subtract(BigInteger)
BigInteger.xor(BigInteger)

3/ While I'm at it ...
Number.byteValue()
Number.doubleValue()
Number.floatValue()
Number.intValue()
Number.longValue()
Number.shortValue()

4/ Any compareTo, equals or toString method?

Actually now that I've actually typed the blasted things out I think the
only ones I would really use wrongly and not notice are the first set -
anything else is usually pretty clear from the semantics of the code.
You be the judge, it'll break my heart to delete all that typing!

Robbie

0


All the panel code will be living inside an inspection class, which will have the list of regexes as a public field. The panel code should manipulate that field directly. It has to be this way for the built-in inspection configuration management code to work. Simple validation on add/change seems fine.

--Dave Griffith

0

OK, well if you can forward me the relevant class(es) I'll see what I
can do. You mentioned that you'd want to reuse the same class with
another inspection so unless you're going to copy'n'paste there's going
to need to be some decoupling somewhere along the line either via an
interface on your side or by events generated by my ui. I'm sure it
will all become clear... :)

N.

Dave Griffith wrote:

All the panel code will be living inside an inspection class, which will have the list of regexes as a public field. The panel code should manipulate that field directly. It has to be this way for the built-in inspection configuration management code to work. Simple validation on add/change seems fine.

--Dave Griffith

0

Please sign in to leave a comment.