7:30 p.m.
On 08/16/2013 01:24 PM, Ade Lee wrote:
Please review.
Thanks,
Ade
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
Public method "toLowerCaseSubsystemType(String s)" seems to be not null
pointer safe in regards of its parameter "s".
Thanks,
Andrew
1:35 p.m.
On 8/16/2013 3:24 PM, Ade Lee wrote:
Please review.
Thanks,
Ade
The tests work, some comments:
1. There's a typo in UserMessages.properties:
CMS_SELFTESTS_TPS_VALIDITY_DESCRIPTION=... TKS is valid.
2. When throwing a new exception, it would be better to attach the
original exception so it can be traced back to the actual line
triggering the error. The SelfTest exception constructors don't seem to
take any exception parameter though, so this will require some changes
in the exception classes. This can be done separately later.
3. Just minor thing, but there seems to be a little bit too many code
blocks & indentations. For clarity, the main code execution shouldn't be
in the if-then-clause unless necessary. For example, instead of this:
if (something != null) {
do something
} else {
throw new Exception();
}
it can be simplified into this:
if (something == null) {
throw new Exception();
}
do something
So the if-then-clause is only used to divert from the main execution if
there's an error.
--
Endi S. Dewata
10:27 a.m.
On Mon, 2013-08-19 at 13:35 -0500, Endi Sukma Dewata wrote:
On 8/16/2013 3:24 PM, Ade Lee wrote:
> Please review.
> Thanks,
> Ade
The tests work, some comments:
1. There's a typo in UserMessages.properties:
CMS_SELFTESTS_TPS_VALIDITY_DESCRIPTION=... TKS is valid.
Fixed.
2. When throwing a new exception, it would be better to attach the
original exception so it can be traced back to the actual line
triggering the error. The SelfTest exception constructors don't seem to
take any exception parameter though, so this will require some changes
in the exception classes. This can be done separately later.
Yes, lets do this separately.
3. Just minor thing, but there seems to be a little bit too many code
blocks & indentations. For clarity, the main code execution shouldn't be
in the if-then-clause unless necessary. For example, instead of this:
if (something != null) {
do something
} else {
throw new Exception();
}
it can be simplified into this:
if (something == null) {
throw new Exception();
}
do something
So the if-then-clause is only used to divert from the main execution if
there's an error.
Agreed. I was taking the old self tests as a template and forgot to
change that bit.
Also added a check for null-ness in the function toLowerCaseSubsystem()
as found by Andrew.
Pushed to master.
3903
days inactive
3907
days old
3 comments
3 participants
participants (3)
-
Ade Lee -
Andrew Wnuk -
Endi Sukma Dewata