IG: check for TestCase too
[idea/community.git] / plugins / InspectionGadgets / Refactoring
1 Just going through to see what's new/changed, and a couple small
2 thoughts to do with as you will.  I know this is a lot, but I hope it's
3 useful feedback, whatever you do with it.
4
5 >(*) Abstraction->Raw types
6 >Shouldn't this be disabled unless under 1.5 (like @Deprecated), or at
7 >least have that as an option?  Probably a general thing about generics
8 >-- some folks used the 1.4 version but others did not.  I don't know
9 >how to do it, but maybe a checkbox to disable/enable all generic checks
10 >under 1.4?
11
12 This is one that actually only turns itself on under 1.5 more or less automatically,
13 since you'll only see this if you are coding against classes that have type arguments.
14 If you're using the 1.5 libraries, you should probably be using them with parameters.
15
16
17 >(*) Class Struct->Static inheritence
18 >This is only avoidable easily with 1.5 imports of fields.  Maybe this
19 >also should have a "enable only if 1.5".
20
21 Strongly disagree.  Static inheritance can easily be avoided even with earlier JVMs,
22 simply by qualifying all the references. Indeed, there's now a quickfix that does
23 just that.  This is a best practice, as static inheritance unnecessarily pollutes
24 generated Javadoc, as well as being very iffy from an OO theory standpoint.
25
26 >(*) Cloning->doesn't throw CNSE
27 >I don't know who should ever turn this on.  If you are making a class
28 >cloneable, you often will not throw CNSE.
29 You want clone() to throw CNSE so that subclasses which don't wish to support cloning
30 can be written.  Not a major use, but somewhat important for those writing libraries,
31 and often forgotten.
32
33 (*) General
34 >Just a question: How do you decide whether a check is "General" or
35 >"Code Style"?  Just wondering.
36
37 All of the "General" and "Local Analysis" inspections are built-in by JetBrains.
38 I've asked them about making more descriptive names, but haven't got a response.
39
40 >Maybe I should be clear -- I want to use one setting for errors and
41 >switch between projects in 1.5 and 1.4.  So I don't want to play around
42 >with these settings.  For example, I want to use annotations properly
43 >in 1.5 code and not see them at all in code that I want to keep 1.4
44 >compatible.
45
46 I have two separate but largely similar profiles for just that purpose.
47
48 >Should be able to say "Same as <drop down list of other conventions>".
49
50 Sadly, the current API prohibits that.  I'll open a ticket.
51
52 >(*) Performance issues
53 >I just want to mark some disquiet here.
54
55 I generally agree, but will note that there is one compelling counterargument.
56 J2ME environments count cycles and class file bytes like a supermodel counts
57 calories, and for good solid engineering and economic reasons.  They generally
58 lack JITs and state-of-the-art GC.  I'm adding some (frankly odd) J2ME specific
59 inspections in the next release, and will probably move some of the "Performance"
60 issues to the new "J2ME" category.  Certainly
61 "Field repeatedly accessed in method" and "Anonymous inner class may be made
62 named static inner class" will be moved to J2ME, as you
63 are quite right that stuff like that simply doesn't matter to any rational
64 J2SE or J2EE application.  "Inner class may be static" will probably be kept
65 where it is, although that's up in the air.
66
67 >(*) Performance issues -> StringBuffer field
68 >This is not a performance issue, is it?
69 I'm probably also splitting out a "Memory management" category.  StringBuffer fields
70 are only a problem to the extent that they can grow larger than you expect,
71 and are thus good places to search for memory leaks.
72
73 >(*) Performance issues -> multiply/div by power of 2
74 >This is flat wrong.
75 Yup, this one will either be killed or moved to the "J2ME" category.
76
77 >(*) Performance issues -> static collection
78 >Should have an option to ignore WeakHashMap or possibly some list of
79 >types (as in "prohibited exception" checks) that are weak collection
80 >types.
81 Ooh, good call.
82
83 >(*) Portability issues -> Hardcoded file sep
84 >Maybe an exception for URL classes?  Or strings that look like URLS?
85 >In them, the slash dir is pre-defined.  (This is the reason I leave
86 >this off -- it flags too many URL contents.)
87
88 It should be filtering this already, the logic for guessing URL's is quite involved.
89 'll look into it.
90
91 >(*) Confusing Code -> COnfusing 'else'
92 >The description could use a simple example to show what you mean.
93 Yup.
94
95 >(*) Probable bugs -> compareto vs. compareTo
96 >This is a special case of a general problem -- a subclass that provides
97 >a method whose name differs only in case from that of an inherited
98 >method.
99
100 Good ideas.
101
102 >(*) Probably bugs -> floating point equality
103 >Have an option to ignore checks for 0.0 and -0.0?  And the constants in
104 >Float and Double?  It's reasonable to say "if (f ==
105 >Double.POSITIVE_INFINITY)", and 0.0 is a very well-defined value.
106
107 Good idea.
108
109 >A new check for comparsion to Double.NaN?  That's always false:
110 >Double.NaN != Double.NaN.
111
112 May already be implemented as a "constant condition" by IDEA.  If not, it's
113 certainly a worthwhile candidate.
114
115 >Compressible: the non-constant string exceptions.
116
117 These were literally just added this weekend, and I'm going to let people play
118 with them a bit before seeing how to modify them.  Compression and a general list
119 solution are definitely within possible future scope.
120
121 >(*) Security -> serializable
122 >When would I have a serializable class that wasn't deserializable?  Is
123 >this really two checks?  (If so, maybe they're compressible?)
124
125 Say you subclass a Serializable library class, and want it to include secure
126 information (or generally just want to make your project hyper-secure).  What
127 you need to do then is explicitly make readObject() and writeObject() throw exceptions,
128 to keep the bad guys from either writing your objects to a file or loading garbage
129 objects into your application somehow.  Yes, these are probably compressible.
130
131 (*) Serialization -> Instance var ...
132 Two things: (1) "Instance var" is a "field".  Other places you use
133 "field".  Probably ought to be consistent overall.  (I used -- nay,
134 insisted upon -- "field" in my book because it's the thing people
135 regularly say.); (2) You don't say whether you take
136
137 >readObject and writeObject mismatched check?  Same for
138 >readResolve/writeReplace?
139
140 Good ones.
141
142 >Why is there a separate check for being without serialVersionUID for
143 >class and non-static inner class?
144
145 Because the second is much more dangerous, as many more changes can cause the
146 default serialVersionUID to break.
147
148 >Check for readobject vs. readObject (as in compareto vs. compareTo)?
149 >And for the other methods, or any other signature mismatch (a
150 >readObject(Foo) method)?
151
152 Reasonable.
153
154 >(*) Verbose -> Redundant no-arg
155 >This is a problem -- if you don't declare the no-arg ctor, then you
156 >can't javadoc it, and that's bad.  At the least it should, by default,
157 >not complain about such ctors that have javadoc.  Removing them is not
158 >harmless.
159 Good eye. I'll add a "ignore redundant no-arg constructors if javadoced" flag.
160
161 >(*) Verbose -> Redundant type cast
162 >Isn't this already somewhere about "Overly strong type casts"?
163 >Wouldn't that catch all the same things?
164
165 "Redundant" is JetBrains.  "Overly strong" is one of mine.  If one were designing
166 from a blank sheet of paper, you would merge these
167
168 >(*) Verbose -> Unn 'continue'
169 >As a way of showing that a loop is *meant* to be empty, a common style
170 >is this:
171 >       while (...)
172 >               continue;
173 >In this case I would want that warning suppressed.  Probably need an
174 >option.
175
176 I've never seen such a style, but am not doubting you.  Perhaps an option.
177
178 >(*) Verbose -> Unn boxing
179 >Is there any reason someone would want to set this a different way than
180 >they set Unn unboxing?  At least these seem compressible.
181
182 Reasonable.