On 07/09/2014 05:29 AM, Wang Weijun wrote:
On Jul 9, 2014, at 3:21, Sean Mullan <sean.mul...@oracle.com> wrote:
- several methods are now defined to throw IllegalStateExc, but you
(perhaps accidentally) removed the code that does that (ex:
getKeyType).
That's because KeyImpl.getKeyType() already throws it. I mentioned it
in that last paragraph of my previous mail. If you think it makes the
code more difficult to read, I'll add them back.
No, you don't have to add them back, but if you could add a comment to
each method indicating that the check for IllegalStateExc is in the
KeyImpl method, it would help.
* KerberosTicket
352-3: put braces around the if (destroyed) statement; same comment
applies in rest of code.
There are quite some single line block without braces in this file
(see init()). My habit was if I don't touch the part I will not
change it. Some people believe whenever a file is touched it's better
to update all appearances of such codes. If you think this is good
I'll update all of them.
Missing braces can more easily lead to inadvertant security bugs.
Because the destroyed block is security related, I think you should at
least fix those, but if you want to fix all of them to be consistent,
that is fine too.
* KerberosCredMessage
- is there a reason why you didn't override hashCode/equals?
I don't think it's likely for someone to put several
KerberosCredMessage into a set (which is where I think
hashCode/equals is useful). Maybe it's a good habit to always verride
hashCode/equals for these "data" classes?
If you think there is a chance it could be useful, I think it is better
to do it now than later. In particular, it should be very easy to
implement, so I would probably do it.
- I think it would actually be better to name this class
EncryptionKey since that is what it is and that way we would be
able to reuse this type in the future if necessary for other
structures or APIs that reference this type (EncryptionKey)
This makes sense. I'll withdraw CCC and resubmit it.
Great.
Thanks,
Sean
