You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Alexander Kiel <al...@gmx.net> on 2009/09/26 14:41:37 UTC
Checkstyle RedundantThrowsCheck
Hi,
why didn't our code style allow unchecked exceptions or subclasses of
thrown exceptions in Javadoc?
From checkstyle-5.0.xml:
<module name="RedundantThrowsCheck">
<property name="allowSubclasses" value="false"/>
<property name="allowUnchecked" value="false"/>
<property name="severity" value="warning"/>
</module>
From "J. Bloch: Effective Java, Second Edition" [1] page 252:
>Use the Javadoc @thows tag to document each unchecked exception
>that a method can throw, but do not use the throws keyword to
>include unchecked exceptions in the method declaration.
Every good code I know, documents unchecked exceptions. Take the Java
Collections API. Every possible ClassCastException or
NullPointerException is documented.
Another quote from J. Bloch:
>A well-documented list of unchecked exceptions that a method
>can throw effectively describes the preconditions for its
>successful execution. It is essential that each method's
>documentation describe its preconditions [...]
I think that everyone can agree with the statements J. Bloch made. So I
would strongly vote to allow documenting unchecked exceptions.
The second point is not allowing subclasses of exceptions in Javadoc. I
don't use this very often, but I have just one example in my mind where
this makes sense. If you have a look into
java.io.DataInputStream#readByte(), there are both IOException and
EOFException documented. EOFException is a subclass of IOException. As
you know a normal InputStream.read() returns -1 at EOF but readByte()
doesn't. So it's worth documenting that readByte() is throwing a
EOFException instead.
So I would also vote allowing subclasses.
Best Regards
Alex
[1]: <http://www.amazon.com/dp/0321356683/>
--
e-mail: alexanderkiel@gmx.net
web: www.alexanderkiel.net
Re: Checkstyle RedundantThrowsCheck
Posted by Alexander Kiel <al...@gmx.net>.
Hi Vincent,
> Speaking of that, there’s a rule that I would suggest to disable: the
> HiddenFieldCheck. I don’t really see its benefit. It forces to find
> somewhat artificial names for variables, where the field name is exactly
> what I want. Sometimes a method doesn’t have a name following the
> setField pattern, yet still acts as a setter for Field. This rule would
> make sense if we were using a Hungarian-like notation for variables
> (mMember, pParam, etc.), but that’s not the case in FOP.
>
> WDYT?
Yes I would vote for it. In modern IDE's one sees clearly the difference
between an instance field and a local variable. This is also the reason
why this Hungarian-like scope notation is largely gone in Java.
Best Regards
Alex
Re: Checkstyle RedundantThrowsCheck
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
I also find the HiddenField check annoying. So I removed the two rules
that were talked about and removed the Checkstyle 3.5 configuration. I
haven't deleted the v4 configuration due to Vincent's comment (I've also
not upgraded, yet), so I let it stay for the moment. We can always
remove it later.
http://svn.apache.org/viewvc?rev=820554&view=rev
On 29.09.2009 19:20:57 Vincent Hennebert wrote:
> Hi Max,
>
> Max Berger wrote:
> > Vincent,
> >
> >
> > 2009/9/29 Vincent Hennebert:
> >> I started to write my own checkstyle configuration from scratch some
> >> time ago, enabling everything that looked important to me. But I’d like
> >> to test it a bit more before submitting it.
> >
> > Same here. See the checkstyle file for JEuclid as an example.
> >
> > http://jeuclid.hg.sourceforge.net/hgweb/jeuclid/jeuclid/file/tip/support/build-tools/src/main/resources/jeuclid/checkstyle.xml
> >
> >> Speaking of that, there’s a rule that I would suggest to disable: the
> >> HiddenFieldCheck. I don’t really see its benefit. It forces to find
> >> somewhat artificial names for variables, where the field name is exactly
> >> what I want. Sometimes a method doesn’t have a name following the
> >> setField pattern, yet still acts as a setter for Field. This rule would
> >> make sense if we were using a Hungarian-like notation for variables
> >> (mMember, pParam, etc.), but that’s not the case in FOP.
> >> WDYT?
> >
> > I like the rule, BUT I am ok with an exception for setters and
> > constructors (this is IMO a new option in checkstyle 5):
> > http://checkstyle.sourceforge.net/config_coding.html#HiddenField
>
> (Actually this option is available in checkstyle 4.)
>
> But what is the benefit of that rule? I find it annoying, so unless I am
> convinced of its usefulness I’d rather disable it.
>
>
> Vincent
Jeremias Maerki
Re: Checkstyle RedundantThrowsCheck
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Max,
Max Berger wrote:
> Vincent,
>
>
> 2009/9/29 Vincent Hennebert:
>> I started to write my own checkstyle configuration from scratch some
>> time ago, enabling everything that looked important to me. But I’d like
>> to test it a bit more before submitting it.
>
> Same here. See the checkstyle file for JEuclid as an example.
>
> http://jeuclid.hg.sourceforge.net/hgweb/jeuclid/jeuclid/file/tip/support/build-tools/src/main/resources/jeuclid/checkstyle.xml
>
>> Speaking of that, there’s a rule that I would suggest to disable: the
>> HiddenFieldCheck. I don’t really see its benefit. It forces to find
>> somewhat artificial names for variables, where the field name is exactly
>> what I want. Sometimes a method doesn’t have a name following the
>> setField pattern, yet still acts as a setter for Field. This rule would
>> make sense if we were using a Hungarian-like notation for variables
>> (mMember, pParam, etc.), but that’s not the case in FOP.
>> WDYT?
>
> I like the rule, BUT I am ok with an exception for setters and
> constructors (this is IMO a new option in checkstyle 5):
> http://checkstyle.sourceforge.net/config_coding.html#HiddenField
(Actually this option is available in checkstyle 4.)
But what is the benefit of that rule? I find it annoying, so unless I am
convinced of its usefulness I’d rather disable it.
Vincent
Re: Checkstyle RedundantThrowsCheck
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Max, I don't think that Checkstyle is the right tool to avoid the
self-assignment problem. Tools like FindBugs are much better in that
regard since they actually check the semantics of the assignment rather
than just complaining that a field and a parameter are named the same.
On 01.10.2009 10:47:31 Max Berger wrote:
> Hi *,
>
> this rule is usefull in the case where you use common names for
> attributes (such as x, or y), and accidentially "overwrite" them as
> parameters. This again comes back to the point of readability.
>
> The same variable name should ALWAYS refer to the same variable / value.
>
> For setters and constructors this makes sense -> after all, in each of
> these you have a simple assignment, and both variables will carry the
> same value.
>
> But in most other methods, the parameter you pass is NOT assigned to the
> internal variable, so they actually refer to a different thing, and
> thats where the confusion starts.
>
> I know modern IDEs can show you which variable you actually refer to,
> but this usually requires selecting the variable or hovering over it,
> which you will not do if you are just reading the code trying to
> understand it.
>
> However, since we cannot agree to keep the rule, I'll have to be content
> with removing it (which is already done).
>
> Max
>
> Alexander Kiel schrieb:
> > Hi Max,
> >
> >>> Speaking of that, there’s a rule that I would suggest to disable: the
> >>> HiddenFieldCheck. I don’t really see its benefit. It forces to find
> >>> somewhat artificial names for variables, where the field name is exactly
> >>> what I want. Sometimes a method doesn’t have a name following the
> >>> setField pattern, yet still acts as a setter for Field. This rule would
> >>> make sense if we were using a Hungarian-like notation for variables
> >>> (mMember, pParam, etc.), but that’s not the case in FOP.
> >>> WDYT?
> >> I like the rule, BUT I am ok with an exception for setters and
> >> constructors (this is IMO a new option in checkstyle 5):
> >> http://checkstyle.sourceforge.net/config_coding.html#HiddenField
> >
> > The exclusion of constructors an setters is important. Otherwise we
> > would be forced to use some Hungarian-like scope notation.
> >
> > But why do you think, that this rule is useful at all?
> >
> > Best Regards
> > Alex
>
>
> --
> http://max.berger.name/
> OpenPGP ID: C93C5700 Fpr: AB6638CE472A499B3959 ADA2F989A2E5C93C5700
>
Jeremias Maerki
Re: Checkstyle RedundantThrowsCheck
Posted by Alexander Kiel <al...@gmx.net>.
Hi Max,
> DISCLAIMER: These comments are to be seen as purely "academic", and may
> be complete overkill. For practical purposes, your code is just fine.
No, its ok, I like code reviews.
> - value is a very generic name, and could be reconsidered. What does the
> "value" actually specify? Looking at the detail, it is the int
> representation of the tag in little-endian. So I'd propose
> intRepresentation instead.
You are right, value is a bit to generic. The representation is actually
big-endian, the first byte of the array is the highest byte. So I should
really put this information in a comment. I'm even not sure why I've
chosen such a compact and difficult to understand representation. A
String would be better.
> - in your constructor, you use value to build up the intRepresentation.
> In this case, I'd use something like "intValue"
Here I would say that repeating the type in the variable name is not
needed. So the question would be why repeating "int" in
"intRepresentation"? Than one could say that the field should be really
named "compactBigEndianIntegerRepresentation". But than this whole
concept of a compact big-endian integer representation of a String with
length of four and reduced ASCII charset should be really go into its
own class.
> - you have a static method valueOf(String) and a constructor (byte[]).
> Why two different ways of initializing the class?
The valueOf(String) is the only public constructor. Its used all over
the font subsystem to create tags if needed. The package private
constructor is only used in the OpenTypeDataInputImpl. So here I have
the reading code of the data representation in the OpenType file (byte
array of length four) at the wrong place. It really needs to be moved
into the OpenTypeDataInputImpl.
> - The constructor should be made private. If you really need to access
> the (byte[]) from within the package, you may provide a static public
> method for access.
Yes this byte[] constructor is a bit odd.
> - This class could be optimized using the flyweight pattern (e.g.
> caching the created objects)
Yes you are right, it could. But I really like to have ConcurrentHashMap
for such a task. So maybe I should wait until we switch to Java 1.5 or
can you recommend the ConcurrenthashMap from the backports JAR?
> - equals would be more readable if you rename tag to otherTag, and use
> this.value == otherTag.value
Yes, please blame Intellij Idea for that.
> - checkByte also uses "value". In this case, you mean "byteValue" or
> "charValue".
You are right!
> - why go with "toChars" creating an array and then using it?
> StringBuilder may be the easier solution.
>
> - in the "alluppercase" and "alllowercase" methods: You may consider
> using Character.isLetter rather than explicitly checking for space and
> numbers. Some characters, such as @ (although probably not used) would
> otherwise create bugs.
The problem is, that a digit is also considered as lowercase. In fact I
realized that this method should be named "containsNoUpperCaseLetters".
I also changed the implementation to:
if (Character.isLetter(ch) && Character.isUpperCase(ch)) {
return false;
}
> > Another example is the method getEntriesInOffsetOrder() in the attached
> > file OffsetTable.java. It is just a getter of entries but it is named
> > different.
>
> getEntriesInOffsetOrder returns a sorted version of the entries. So why
> not call the variable sortedEntries?
Because it is not sorted before calling Collections.sort(). If you read
List sortedEntries = getEntries();
you would expect, that getEntries() will return alright sorted entries.
The problem is, that Collections.sort() uses the "output parameter"
anti-pattern.
> Other notes:
> - getEntries does not return the entries attribute. This means you are
> confusing internal and external representation. getEntrieValues() could
> be a better name.
No entries is really a simple collection of enties (look at the
constructor). The Map is really a mapping from tag to entry. So I should
name it tagToEntryMap. Ahh and than the problem with the hidden variable
is also solved. Good point :-)
> - since the entries are re-ordered anyways when adding to the map, why
> not use a SortedMap (e.g. TreeMap instead)? Then one "getEntries" method
> would suffice.
Uhh. You spottet a bug. I need a LinkedHashMap here. Thanks! I really
like to have the entries in original order and in offset order.
> - you have some default visibility methods and classes, would should be
> reconsidered.
What is wrong with package local visibility? I find it very useful. In
fact, I think, its the most useful visibility right after private.
> > I think this rule ist mostly helpful in order to think about variable
> > names. But I also think that here are a few cases where violating this
> > rule makes sense. So maybe the rule ist just not smart enough to detect
> > the remaining special cases.
>
> If you are really sure you can always temporary disable CHECKSTLYE with
>
> // CHECKSTYLE:OFF
> violating code
> // CHECKSTYLE:ON
I that allowed? :-)
> > Thats the same as with the "Javadoc on public things rule". If there
> > isn't anything to say about a public thing which will say more than the
> > name itself, than I prefer no comment at all. But how should Checkstyle
> > detect such cases?
>
> In most cases there is more information to be transported. For example
> the "getEntriesInOffsetOrder" method. This may be clear to you because
> you have written the code, but I first had to think for a while before I
> realized what the "offsetOrder" is. It was easier for me since I saw the
> source code. But would I use your class, it would not get it immediately.
Yes this class is just not finished yet. The offset order needs
explanation.
I thank you for your code review. At the end I have now a much cleaner
Tag class. I attached all relevant files to this mail.
Best Regards
Alex
Re: Checkstyle RedundantThrowsCheck
Posted by Max Berger <ma...@berger.name>.
Hi Alex,
DISCLAIMER: These comments are to be seen as purely "academic", and may
be complete overkill. For practical purposes, your code is just fine.
Alexander Kiel schrieb:
> In my attachment Tag.java, you can see a variable named "value" in the
> constuctor and as field. According the rule, the variable in the
> constructor hides the field. But its really the same thing. I even
> assign it in the last line of the constuctor.
Options to make this code more readable:
- value is a very generic name, and could be reconsidered. What does the
"value" actually specify? Looking at the detail, it is the int
representation of the tag in little-endian. So I'd propose
intRepresentation instead.
- in your constructor, you use value to build up the intRepresentation.
In this case, I'd use something like "intValue"
- you have a static method valueOf(String) and a constructor (byte[]).
Why two different ways of initializing the class?
- The constructor should be made private. If you really need to access
the (byte[]) from within the package, you may provide a static public
method for access.
- This class could be optimized using the flyweight pattern (e.g.
caching the created objects)
- equals would be more readable if you rename tag to otherTag, and use
this.value == otherTag.value
- checkByte also uses "value". In this case, you mean "byteValue" or
"charValue".
- why go with "toChars" creating an array and then using it?
StringBuilder may be the easier solution.
- in the "alluppercase" and "alllowercase" methods: You may consider
using Character.isLetter rather than explicitly checking for space and
numbers. Some characters, such as @ (although probably not used) would
otherwise create bugs.
> Another example is the method getEntriesInOffsetOrder() in the attached
> file OffsetTable.java. It is just a getter of entries but it is named
> different.
getEntriesInOffsetOrder returns a sorted version of the entries. So why
not call the variable sortedEntries?
Other notes:
- getEntries does not return the entries attribute. This means you are
confusing internal and external representation. getEntrieValues() could
be a better name.
- since the entries are re-ordered anyways when adding to the map, why
not use a SortedMap (e.g. TreeMap instead)? Then one "getEntries" method
would suffice.
- you have some default visibility methods and classes, would should be
reconsidered.
>> But in most other methods, the parameter you pass is NOT assigned to the
>> internal variable, so they actually refer to a different thing, and
>> thats where the confusion starts.
> You are absolutely right. In most cases the variable really refers to a
> different thing. The above two examples are the only two cases where I
> violated the HiddenFieldRule in 155 new classes.
Thats good to know :)
> I think this rule ist mostly helpful in order to think about variable
> names. But I also think that here are a few cases where violating this
> rule makes sense. So maybe the rule ist just not smart enough to detect
> the remaining special cases.
If you are really sure you can always temporary disable CHECKSTLYE with
// CHECKSTYLE:OFF
violating code
// CHECKSTYLE:ON
[don't know if we have that in our checkstyle rules or not, I usually
have it there].
Of course, in this case it would be nice to get a notice why checkstyle
was disabled:
// CHECKSTYLE:OFF
// this.value being build-up
int value;
// CHECKSTYLE:ON
...
> Thats the same as with the "Javadoc on public things rule". If there
> isn't anything to say about a public thing which will say more than the
> name itself, than I prefer no comment at all. But how should Checkstyle
> detect such cases?
In most cases there is more information to be transported. For example
the "getEntriesInOffsetOrder" method. This may be clear to you because
you have written the code, but I first had to think for a while before I
realized what the "offsetOrder" is. It was easier for me since I saw the
source code. But would I use your class, it would not get it immediately.
> There is a @SuppressWarnings annotation. I don't know if Checkstyle uses
> it. So maybe if we switch to Java 1.5, we could use it. But even than
> this annotation is a lot of clutter. It's a pity that computers can't
> think.
see above
> Best Regards
> Alex
Max
--
http://max.berger.name/
OpenPGP ID: C93C5700 Fpr: AB6638CE472A499B3959 ADA2F989A2E5C93C5700
Re: Checkstyle RedundantThrowsCheck
Posted by Alexander Kiel <al...@gmx.net>.
Hi Max,
> The same variable name should ALWAYS refer to the same variable / value.
I think, I can second this.
> For setters and constructors this makes sense -> after all, in each of
> these you have a simple assignment, and both variables will carry the
> same value.
In my attachment Tag.java, you can see a variable named "value" in the
constuctor and as field. According the rule, the variable in the
constructor hides the field. But its really the same thing. I even
assign it in the last line of the constuctor.
Another example is the method getEntriesInOffsetOrder() in the attached
file OffsetTable.java. It is just a getter of entries but it is named
different.
> But in most other methods, the parameter you pass is NOT assigned to the
> internal variable, so they actually refer to a different thing, and
> thats where the confusion starts.
You are absolutely right. In most cases the variable really refers to a
different thing. The above two examples are the only two cases where I
violated the HiddenFieldRule in 155 new classes.
> I know modern IDEs can show you which variable you actually refer to,
> but this usually requires selecting the variable or hovering over it,
> which you will not do if you are just reading the code trying to
> understand it.
Not in Intellij Idea. There fields are bold and dark magenta and all
other variables are just normal and black.
> However, since we cannot agree to keep the rule, I'll have to be content
> with removing it (which is already done).
I think this rule ist mostly helpful in order to think about variable
names. But I also think that here are a few cases where violating this
rule makes sense. So maybe the rule ist just not smart enough to detect
the remaining special cases.
Thats the same as with the "Javadoc on public things rule". If there
isn't anything to say about a public thing which will say more than the
name itself, than I prefer no comment at all. But how should Checkstyle
detect such cases?
There is a @SuppressWarnings annotation. I don't know if Checkstyle uses
it. So maybe if we switch to Java 1.5, we could use it. But even than
this annotation is a lot of clutter. It's a pity that computers can't
think.
Best Regards
Alex
Re: Checkstyle RedundantThrowsCheck
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Max,
Thanks for your explanation.
Max Berger wrote:
> Hi *,
>
> this rule is usefull in the case where you use common names for
> attributes (such as x, or y), and accidentially "overwrite" them as
> parameters. This again comes back to the point of readability.
>
> The same variable name should ALWAYS refer to the same variable / value.
>
> For setters and constructors this makes sense -> after all, in each of
> these you have a simple assignment, and both variables will carry the
> same value.
>
> But in most other methods, the parameter you pass is NOT assigned to the
> internal variable, so they actually refer to a different thing, and
> thats where the confusion starts.
You definitely have a point here. But I’ve found that in a majority of
cases, warnings raised by this Checkstyle rule are “false positive”
i.e., correspond to setters that don’t match the simple setField pattern
(Alexander’s examples are good ones). Insofar as this rule creates more
noise than useful warnings, I’d remove it.
> I know modern IDEs can show you which variable you actually refer to,
> but this usually requires selecting the variable or hovering over it,
> which you will not do if you are just reading the code trying to
> understand it.
>
> However, since we cannot agree to keep the rule, I'll have to be content
> with removing it (which is already done).
Yeah, at least waiting for Max’ explanation before applying the majority
rule would have been good.
<snip/>
Vincent
Re: Checkstyle RedundantThrowsCheck
Posted by Max Berger <ma...@berger.name>.
Hi *,
this rule is usefull in the case where you use common names for
attributes (such as x, or y), and accidentially "overwrite" them as
parameters. This again comes back to the point of readability.
The same variable name should ALWAYS refer to the same variable / value.
For setters and constructors this makes sense -> after all, in each of
these you have a simple assignment, and both variables will carry the
same value.
But in most other methods, the parameter you pass is NOT assigned to the
internal variable, so they actually refer to a different thing, and
thats where the confusion starts.
I know modern IDEs can show you which variable you actually refer to,
but this usually requires selecting the variable or hovering over it,
which you will not do if you are just reading the code trying to
understand it.
However, since we cannot agree to keep the rule, I'll have to be content
with removing it (which is already done).
Max
Alexander Kiel schrieb:
> Hi Max,
>
>>> Speaking of that, there’s a rule that I would suggest to disable: the
>>> HiddenFieldCheck. I don’t really see its benefit. It forces to find
>>> somewhat artificial names for variables, where the field name is exactly
>>> what I want. Sometimes a method doesn’t have a name following the
>>> setField pattern, yet still acts as a setter for Field. This rule would
>>> make sense if we were using a Hungarian-like notation for variables
>>> (mMember, pParam, etc.), but that’s not the case in FOP.
>>> WDYT?
>> I like the rule, BUT I am ok with an exception for setters and
>> constructors (this is IMO a new option in checkstyle 5):
>> http://checkstyle.sourceforge.net/config_coding.html#HiddenField
>
> The exclusion of constructors an setters is important. Otherwise we
> would be forced to use some Hungarian-like scope notation.
>
> But why do you think, that this rule is useful at all?
>
> Best Regards
> Alex
--
http://max.berger.name/
OpenPGP ID: C93C5700 Fpr: AB6638CE472A499B3959 ADA2F989A2E5C93C5700
Re: Checkstyle RedundantThrowsCheck
Posted by Alexander Kiel <al...@gmx.net>.
Hi Max,
> > Speaking of that, there’s a rule that I would suggest to disable: the
> > HiddenFieldCheck. I don’t really see its benefit. It forces to find
> > somewhat artificial names for variables, where the field name is exactly
> > what I want. Sometimes a method doesn’t have a name following the
> > setField pattern, yet still acts as a setter for Field. This rule would
> > make sense if we were using a Hungarian-like notation for variables
> > (mMember, pParam, etc.), but that’s not the case in FOP.
> > WDYT?
>
> I like the rule, BUT I am ok with an exception for setters and
> constructors (this is IMO a new option in checkstyle 5):
> http://checkstyle.sourceforge.net/config_coding.html#HiddenField
The exclusion of constructors an setters is important. Otherwise we
would be forced to use some Hungarian-like scope notation.
But why do you think, that this rule is useful at all?
Best Regards
Alex
Re: Checkstyle RedundantThrowsCheck
Posted by Max Berger <ma...@berger.name>.
Vincent,
2009/9/29 Vincent Hennebert <vh...@gmail.com>:
> I started to write my own checkstyle configuration from scratch some
> time ago, enabling everything that looked important to me. But I’d like
> to test it a bit more before submitting it.
Same here. See the checkstyle file for JEuclid as an example.
http://jeuclid.hg.sourceforge.net/hgweb/jeuclid/jeuclid/file/tip/support/build-tools/src/main/resources/jeuclid/checkstyle.xml
> Speaking of that, there’s a rule that I would suggest to disable: the
> HiddenFieldCheck. I don’t really see its benefit. It forces to find
> somewhat artificial names for variables, where the field name is exactly
> what I want. Sometimes a method doesn’t have a name following the
> setField pattern, yet still acts as a setter for Field. This rule would
> make sense if we were using a Hungarian-like notation for variables
> (mMember, pParam, etc.), but that’s not the case in FOP.
> WDYT?
I like the rule, BUT I am ok with an exception for setters and
constructors (this is IMO a new option in checkstyle 5):
http://checkstyle.sourceforge.net/config_coding.html#HiddenField
Max
Re: Checkstyle RedundantThrowsCheck
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi Max,
Max Berger wrote:
> Alex,
>
> The checkstyle checks are historically grown, and are therefore
> incomplete. I personally would turn on much more checks for certain
> style issues I like. IMO every option set helps deciding a certain
> factor. So more the more checks the better :)
If you think that the current checkstyle could be improved, then by all
means, do suggest changes.
I started to write my own checkstyle configuration from scratch some
time ago, enabling everything that looked important to me. But I’d like
to test it a bit more before submitting it.
Speaking of that, there’s a rule that I would suggest to disable: the
HiddenFieldCheck. I don’t really see its benefit. It forces to find
somewhat artificial names for variables, where the field name is exactly
what I want. Sometimes a method doesn’t have a name following the
setField pattern, yet still acts as a setter for Field. This rule would
make sense if we were using a Hungarian-like notation for variables
(mMember, pParam, etc.), but that’s not the case in FOP.
WDYT?
> (in short: +1 to your changes).
>
> Right now we have 3 checkstyle files: 3.5, 4.0, and 5.0, which also
> means the checks would need to be added in all of them (if possible).
> Can we remove any of them? I'd volunteer to modify the ant buildfile
> to support 5.0.
>
> I'd also vote for dropping 3.5 support, and potentially dropping checkstyle 4.
+1. Let’s avoid redundancy. Checkstyle 5.0 still looks a bit on the
bleeding edge to me, but I’m happy to update my checkstyle plug-in
accordingly.
Vincent
> Max
>
>
>
> 2009/9/26 Alexander Kiel <al...@gmx.net>:
>> Hi,
>>
>> why didn't our code style allow unchecked exceptions or subclasses of
>> thrown exceptions in Javadoc?
>>
>> From checkstyle-5.0.xml:
>>
>> <module name="RedundantThrowsCheck">
>> <property name="allowSubclasses" value="false"/>
>> <property name="allowUnchecked" value="false"/>
>> <property name="severity" value="warning"/>
>> </module>
>>
>> From "J. Bloch: Effective Java, Second Edition" [1] page 252:
>>
>>> Use the Javadoc @thows tag to document each unchecked exception
>>> that a method can throw, but do not use the throws keyword to
>>> include unchecked exceptions in the method declaration.
>> Every good code I know, documents unchecked exceptions. Take the Java
>> Collections API. Every possible ClassCastException or
>> NullPointerException is documented.
>>
>> Another quote from J. Bloch:
>>
>>> A well-documented list of unchecked exceptions that a method
>>> can throw effectively describes the preconditions for its
>>> successful execution. It is essential that each method's
>>> documentation describe its preconditions [...]
>> I think that everyone can agree with the statements J. Bloch made. So I
>> would strongly vote to allow documenting unchecked exceptions.
>>
>>
>> The second point is not allowing subclasses of exceptions in Javadoc. I
>> don't use this very often, but I have just one example in my mind where
>> this makes sense. If you have a look into
>> java.io.DataInputStream#readByte(), there are both IOException and
>> EOFException documented. EOFException is a subclass of IOException. As
>> you know a normal InputStream.read() returns -1 at EOF but readByte()
>> doesn't. So it's worth documenting that readByte() is throwing a
>> EOFException instead.
>>
>> So I would also vote allowing subclasses.
>>
>>
>> Best Regards
>> Alex
>>
>> [1]: <http://www.amazon.com/dp/0321356683/>
>>
>> --
>> e-mail: alexanderkiel@gmx.net
>> web: www.alexanderkiel.net
Re: Checkstyle RedundantThrowsCheck
Posted by Max Berger <ma...@berger.name>.
Alex,
The checkstyle checks are historically grown, and are therefore
incomplete. I personally would turn on much more checks for certain
style issues I like. IMO every option set helps deciding a certain
factor. So more the more checks the better :)
(in short: +1 to your changes).
Right now we have 3 checkstyle files: 3.5, 4.0, and 5.0, which also
means the checks would need to be added in all of them (if possible).
Can we remove any of them? I'd volunteer to modify the ant buildfile
to support 5.0.
I'd also vote for dropping 3.5 support, and potentially dropping checkstyle 4.
Max
2009/9/26 Alexander Kiel <al...@gmx.net>:
> Hi,
>
> why didn't our code style allow unchecked exceptions or subclasses of
> thrown exceptions in Javadoc?
>
> From checkstyle-5.0.xml:
>
> <module name="RedundantThrowsCheck">
> <property name="allowSubclasses" value="false"/>
> <property name="allowUnchecked" value="false"/>
> <property name="severity" value="warning"/>
> </module>
>
> From "J. Bloch: Effective Java, Second Edition" [1] page 252:
>
>>Use the Javadoc @thows tag to document each unchecked exception
>>that a method can throw, but do not use the throws keyword to
>>include unchecked exceptions in the method declaration.
>
> Every good code I know, documents unchecked exceptions. Take the Java
> Collections API. Every possible ClassCastException or
> NullPointerException is documented.
>
> Another quote from J. Bloch:
>
>>A well-documented list of unchecked exceptions that a method
>>can throw effectively describes the preconditions for its
>>successful execution. It is essential that each method's
>>documentation describe its preconditions [...]
>
> I think that everyone can agree with the statements J. Bloch made. So I
> would strongly vote to allow documenting unchecked exceptions.
>
>
> The second point is not allowing subclasses of exceptions in Javadoc. I
> don't use this very often, but I have just one example in my mind where
> this makes sense. If you have a look into
> java.io.DataInputStream#readByte(), there are both IOException and
> EOFException documented. EOFException is a subclass of IOException. As
> you know a normal InputStream.read() returns -1 at EOF but readByte()
> doesn't. So it's worth documenting that readByte() is throwing a
> EOFException instead.
>
> So I would also vote allowing subclasses.
>
>
> Best Regards
> Alex
>
> [1]: <http://www.amazon.com/dp/0321356683/>
>
> --
> e-mail: alexanderkiel@gmx.net
> web: www.alexanderkiel.net
>
>
Re: Checkstyle RedundantThrowsCheck
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi,
Jeremias Maerki wrote:
> On 27.09.2009 13:27:35 Alexander Kiel wrote:
>> Hi Jeremias,
>>
>>> Makes sense. I stumbled over that myself from time to time but it didn't
>>> really bother me so much to take action.
>> Okay. Can you please modify the checkstyle XML files to reflect that?
>
> Sure, but only after a period of at least 72 hours to allow the other
> committers to raise an objection.
I can’t imagine anybody objecting such a well-argued change proposal...
Anyway... +1 from me.
<snip/>
Vincent
Re: Checkstyle RedundantThrowsCheck
Posted by Alexander Kiel <al...@gmx.net>.
Hi Jeremias,
>>> Makes sense. I stumbled over that myself from time to time but it didn't
>>> really bother me so much to take action.
>>>
>> Okay. Can you please modify the checkstyle XML files to reflect that?
>>
>
> Sure, but only after a period of at least 72 hours to allow the other
> committers to raise an objection.
>
Of course.
>> I'm a great fan of that checkstyle stuff. I didn't use it before, but I
>> find a common coding style important for such a big and shared project
>> like FOP.
>>
>> What's about severities? Did you commit code with checkstyle errors?
>>
>
> No, I always fix errors (mine or others'). Sometimes tab characters
> creep in, for example. The Checkstyle plug-in for Eclipse is really
> helpful in that department. If I didn't fix Checkstyle errors I might
> not notice any build failures prior to a commit.
>
I find it very comfortable to hopefully contribute something real useful
in such a great project. Please don't understand me in a wrong sense, as
I sometimes be a bit to harsh. Its only my nature that I like to be precise.
Best Regards
Alex
Re: Checkstyle RedundantThrowsCheck
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
On 27.09.2009 13:27:35 Alexander Kiel wrote:
> Hi Jeremias,
>
> > Makes sense. I stumbled over that myself from time to time but it didn't
> > really bother me so much to take action.
>
> Okay. Can you please modify the checkstyle XML files to reflect that?
Sure, but only after a period of at least 72 hours to allow the other
committers to raise an objection.
> I'm a great fan of that checkstyle stuff. I didn't use it before, but I
> find a common coding style important for such a big and shared project
> like FOP.
>
> What's about severities? Did you commit code with checkstyle errors?
No, I always fix errors (mine or others'). Sometimes tab characters
creep in, for example. The Checkstyle plug-in for Eclipse is really
helpful in that department. If I didn't fix Checkstyle errors I might
not notice any build failures prior to a commit.
> Best Regards
> Alex
>
> > On 26.09.2009 14:41:37 Alexander Kiel wrote:
> > > Hi,
> > >
> > > why didn't our code style allow unchecked exceptions or subclasses of
> > > thrown exceptions in Javadoc?
> > >
> > > From checkstyle-5.0.xml:
> > >
> > > <module name="RedundantThrowsCheck">
> > > <property name="allowSubclasses" value="false"/>
> > > <property name="allowUnchecked" value="false"/>
> > > <property name="severity" value="warning"/>
> > > </module>
> > >
> > > From "J. Bloch: Effective Java, Second Edition" [1] page 252:
> > >
> > > >Use the Javadoc @thows tag to document each unchecked exception
> > > >that a method can throw, but do not use the throws keyword to
> > > >include unchecked exceptions in the method declaration.
> > >
> > > Every good code I know, documents unchecked exceptions. Take the Java
> > > Collections API. Every possible ClassCastException or
> > > NullPointerException is documented.
> > >
> > > Another quote from J. Bloch:
> > >
> > > >A well-documented list of unchecked exceptions that a method
> > > >can throw effectively describes the preconditions for its
> > > >successful execution. It is essential that each method's
> > > >documentation describe its preconditions [...]
> > >
> > > I think that everyone can agree with the statements J. Bloch made. So I
> > > would strongly vote to allow documenting unchecked exceptions.
> > >
> > >
> > > The second point is not allowing subclasses of exceptions in Javadoc. I
> > > don't use this very often, but I have just one example in my mind where
> > > this makes sense. If you have a look into
> > > java.io.DataInputStream#readByte(), there are both IOException and
> > > EOFException documented. EOFException is a subclass of IOException. As
> > > you know a normal InputStream.read() returns -1 at EOF but readByte()
> > > doesn't. So it's worth documenting that readByte() is throwing a
> > > EOFException instead.
> > >
> > > So I would also vote allowing subclasses.
> > >
> > >
> > > Best Regards
> > > Alex
> > >
> > > [1]: <http://www.amazon.com/dp/0321356683/>
> > >
> > > --
> > > e-mail: alexanderkiel@gmx.net
> > > web: www.alexanderkiel.net
> > >
> >
> >
> >
> >
> > Jeremias Maerki
> >
> >
>
Jeremias Maerki
Re: Checkstyle RedundantThrowsCheck
Posted by Alexander Kiel <al...@gmx.net>.
Hi Jeremias,
> Makes sense. I stumbled over that myself from time to time but it didn't
> really bother me so much to take action.
Okay. Can you please modify the checkstyle XML files to reflect that?
I'm a great fan of that checkstyle stuff. I didn't use it before, but I
find a common coding style important for such a big and shared project
like FOP.
What's about severities? Did you commit code with checkstyle errors?
Best Regards
Alex
> On 26.09.2009 14:41:37 Alexander Kiel wrote:
> > Hi,
> >
> > why didn't our code style allow unchecked exceptions or subclasses of
> > thrown exceptions in Javadoc?
> >
> > From checkstyle-5.0.xml:
> >
> > <module name="RedundantThrowsCheck">
> > <property name="allowSubclasses" value="false"/>
> > <property name="allowUnchecked" value="false"/>
> > <property name="severity" value="warning"/>
> > </module>
> >
> > From "J. Bloch: Effective Java, Second Edition" [1] page 252:
> >
> > >Use the Javadoc @thows tag to document each unchecked exception
> > >that a method can throw, but do not use the throws keyword to
> > >include unchecked exceptions in the method declaration.
> >
> > Every good code I know, documents unchecked exceptions. Take the Java
> > Collections API. Every possible ClassCastException or
> > NullPointerException is documented.
> >
> > Another quote from J. Bloch:
> >
> > >A well-documented list of unchecked exceptions that a method
> > >can throw effectively describes the preconditions for its
> > >successful execution. It is essential that each method's
> > >documentation describe its preconditions [...]
> >
> > I think that everyone can agree with the statements J. Bloch made. So I
> > would strongly vote to allow documenting unchecked exceptions.
> >
> >
> > The second point is not allowing subclasses of exceptions in Javadoc. I
> > don't use this very often, but I have just one example in my mind where
> > this makes sense. If you have a look into
> > java.io.DataInputStream#readByte(), there are both IOException and
> > EOFException documented. EOFException is a subclass of IOException. As
> > you know a normal InputStream.read() returns -1 at EOF but readByte()
> > doesn't. So it's worth documenting that readByte() is throwing a
> > EOFException instead.
> >
> > So I would also vote allowing subclasses.
> >
> >
> > Best Regards
> > Alex
> >
> > [1]: <http://www.amazon.com/dp/0321356683/>
> >
> > --
> > e-mail: alexanderkiel@gmx.net
> > web: www.alexanderkiel.net
> >
>
>
>
>
> Jeremias Maerki
>
>
Re: Checkstyle RedundantThrowsCheck
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Makes sense. I stumbled over that myself from time to time but it didn't
really bother me so much to take action.
On 26.09.2009 14:41:37 Alexander Kiel wrote:
> Hi,
>
> why didn't our code style allow unchecked exceptions or subclasses of
> thrown exceptions in Javadoc?
>
> From checkstyle-5.0.xml:
>
> <module name="RedundantThrowsCheck">
> <property name="allowSubclasses" value="false"/>
> <property name="allowUnchecked" value="false"/>
> <property name="severity" value="warning"/>
> </module>
>
> From "J. Bloch: Effective Java, Second Edition" [1] page 252:
>
> >Use the Javadoc @thows tag to document each unchecked exception
> >that a method can throw, but do not use the throws keyword to
> >include unchecked exceptions in the method declaration.
>
> Every good code I know, documents unchecked exceptions. Take the Java
> Collections API. Every possible ClassCastException or
> NullPointerException is documented.
>
> Another quote from J. Bloch:
>
> >A well-documented list of unchecked exceptions that a method
> >can throw effectively describes the preconditions for its
> >successful execution. It is essential that each method's
> >documentation describe its preconditions [...]
>
> I think that everyone can agree with the statements J. Bloch made. So I
> would strongly vote to allow documenting unchecked exceptions.
>
>
> The second point is not allowing subclasses of exceptions in Javadoc. I
> don't use this very often, but I have just one example in my mind where
> this makes sense. If you have a look into
> java.io.DataInputStream#readByte(), there are both IOException and
> EOFException documented. EOFException is a subclass of IOException. As
> you know a normal InputStream.read() returns -1 at EOF but readByte()
> doesn't. So it's worth documenting that readByte() is throwing a
> EOFException instead.
>
> So I would also vote allowing subclasses.
>
>
> Best Regards
> Alex
>
> [1]: <http://www.amazon.com/dp/0321356683/>
>
> --
> e-mail: alexanderkiel@gmx.net
> web: www.alexanderkiel.net
>
Jeremias Maerki