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.