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.