On 9/1/2011 12:31 PM, Weijun Wang wrote:
>
> On 08/31/2011 09:17 PM, Xuelei Fan wrote:
>> I understand the code, fine to me. But the loadKeyStore() method looks
>> really ugly and lazy. :-)
>
> It's ugly, but not very lazy.
>
Lazy code is not a bad coding style. ;-)
> Anyway, I'm going to putback this version since you already said fine.
>
Please go ahead. It's fine to me. I just want to talk about some of my
concerns.
Thanks,
Xuelei
>>
>> Just for your reference in the inline comments.
>>
> ...
>
>>>
>>> Well, it looks more correct, but is complicated in 2 senses:
>>>
>>> 1. ~/.keystore and user-specified -keystore are not treated the same.
>>> You can ignore ~/.keystore, but if a user-specified -keystore does not
>>> exist, it's an error.
>>>
>> We can handle the logic simply at the following blok, right?
>> 1562 if (!nullStream&& keyStoreName == null) {
>> 1563 keyStoreName = System.getProperty("user.home") + File.separator
>> 1564 + ".keystore";
>> + // check file existence, ignore it if non-exist
>> 1565 }
>>
>>> 2. signing and verification have different behaviors on exception
>>> handling. See above.
>>>
>> We don't need to make more significant update in other blocks because of
>> we have ignore the non-exist keystore, right? If no required private
>> key, the following steps will throw the expected exception.
>
> If you ignore non-existing ~/.keystore, there will be no exception, and
> store will be either null or uninitialized. Then when it's used, NPE or
> KeyStoreException will be thrown and the user is confused. So you still
> need to check it earlier.
>
> Thanks
> Max
>
>>
>> Or is there any other thing that I missed?
>>
>> Thanks,
>> Xuelei
