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
