Re: Checkstyle RedundantThrowsCheck
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 signature.asc Description: OpenPGP digital signature
Re: Checkstyle RedundantThrowsCheck
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
Re: Checkstyle RedundantThrowsCheck
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=820554view=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
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 signature.asc Description: OpenPGP digital signature
Re: Checkstyle RedundantThrowsCheck
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
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
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the License); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an AS IS BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/* $Id */
package org.apache.fop.fonts.opentype.common;
import java.io.UnsupportedEncodingException;
/**
* Array of four uint8s (length = 32 bits) used to identify a script, language system, feature, or
* baseline.
* p/
* Tags are the names given to tables in the OpenType font file. All tag names consist of four
* characters. Names with less than four letters are allowed if followed by the necessary trailing
* spaces. All tag names defined within a font (e.g., table names, feature tags, language tags) must
* be built from printing characters represented by ASCII values 32-126.
*/
public final class Tag {
public static final Tag TTCF = Tag.valueOf(ttcf);
public static final Tag OTTO = Tag.valueOf(OTTO);
public static final Tag TRUE = Tag.valueOf(true);
public static final Tag TYP1 = Tag.valueOf(typ1);
private static final int MIN_BYTE_VALUE = 0x20;
private static final int MAX_BYTE_VALUE = 0x7E;
private final int value;
public static Tag valueOf(String str) {
int length = str.length();
if (length 4) {
throw new IllegalArgumentException(str.length() 4; was: + length);
}
try {
return new Tag(str.getBytes(ISO-8859-1));
} catch (UnsupportedEncodingException e) {
//TODO: not the best solution
throw new InternalError(e.getMessage());
}
}
Tag(byte[] bytes) {
if (bytes.length 4) {
throw new IllegalArgumentException(bytes.length 4; was: + bytes.length);
}
int value = 0;
for (int i = 0; i 4; i++) {
checkByte(bytes[i], i);
value += (bytes[i] ((3 - i) * 8));
}
this.value = value;
}
public boolean isAllUpperCase() {
String tagStr = toString();
for (int i = 0; i tagStr.length(); i++) {
char ch = tagStr.charAt(i);
if (!Character.isUpperCase(ch) !Character.isDigit(ch)
Re: Checkstyle RedundantThrowsCheck
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
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 signature.asc Description: This is a digitally signed message part
Re: Checkstyle RedundantThrowsCheck
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 signature.asc Description: This is a digitally signed message part
Re: Checkstyle RedundantThrowsCheck
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 alexanderk...@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: alexanderk...@gmx.net web:www.alexanderkiel.net
Re: Checkstyle RedundantThrowsCheck
Vincent, 2009/9/29 Vincent Hennebert vhenneb...@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
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
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
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 alexanderk...@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: alexanderk...@gmx.net web: www.alexanderkiel.net
Re: Checkstyle RedundantThrowsCheck
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: alexanderk...@gmx.net web:www.alexanderkiel.net Jeremias Maerki signature.asc Description: This is a digitally signed message part
Re: Checkstyle RedundantThrowsCheck
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: alexanderk...@gmx.net web:www.alexanderkiel.net Jeremias Maerki Jeremias Maerki
Re: Checkstyle RedundantThrowsCheck
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
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: alexanderk...@gmx.net web:www.alexanderkiel.net Jeremias Maerki
