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

- 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

>> 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

violating code

[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:
// this.value being build-up
int value;

> 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


OpenPGP ID: C93C5700 Fpr: AB6638CE472A499B3959 ADA2F989A2E5C93C5700

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to