Hi Justin,
Appreciate the efforts you making in getting things done. As a person who likes
feedback myself I have put together a few comments, please accept these as
feedback rather than criticism, I certainly don't want you to think your
efforts are not appreciated.
Unfortunately I have only had time to look at one function in one class,
hth
dan
PostcodeValidator.validatePostcode(...)
--
In general, seems to be a few literal strings which could be converted to
constants, e.g.:
private static const FORMAT_CHAR_NUMBER:String = "N";
private static const FORMAT_CHAR_LETTER:String = "A";
private static const FORMAT_CHAR_COUNTRY_CODE:String = "C";
public static const: ERROR_CODE_WRONG_FORMAT:String = "wrongFormat";
etc
Line 107:
107 var noformats:int;
> Ambiguous variable name; does it mean 'number of formats' or 'no formats', if
> number of formats, suggest rename to 'numberOfFormats'
---------------
Line 129:
129 if (postCode) {
130 length = postCode.length;
131 }
> Why make this check here, from what I can see, the value of postCode does not
> change within the loop, so why not make the check and assignment prior to the
> loop and avoid multiple checks ?
> I note also, that this condition would never occur when called by
> doValidation(), but accept it could happen if called by something else.
Line 122:
for (var f:int =
> this seems a strange variable name; I would suggest either sticking to i, j,
> etc or if using a more meaningful name like 'formatIndex' or something like
> that.
Line 138:
138 // ignore character past end of format string
139 if (i >= formatLength) {
140 break;
141 }
> Could this condition not be determined before getting to the start of the
> loop (inner loop) and then the i < length be a value dependent on i >=
> formatLength ; I don't see any code which changes the formatLength once
> inside the loop ? I see you are doing some kind of check anyway down on line
> 169, maybe this could be brought higher and used to determine the length
> variable in the loop ? Also if you are into the "clean code" approach by Bob
> Martin, then this comment is probably an indication that you need to
> refractor to more meaningful code, e.g.
if(reachedEndOfFormatString(position, formatLength))
{
break;
}
or if that feels like overkill:
if( characterPosition >= formatLength)
{
...
Line 143:
DECIMAL_DIGITS.indexOf(char) == -1
Suggest you could create functions which do checks like this and give them a
more meaningful name, this way, if you change the implementation you only have
to change it in one place, for example put DECIMAL_DIGITS.indexOf(char) == -1
inside of a function called noDecimalDigitsIn, then your code reads more
meaningful to those scanning through:
if( noDecimalDigitsIn(char) && noRomanDigitsInChar(char) &&
noSpacersInChar(char)
{
etc
private function noDecimalDigitsInChar(char:String):Boolean
{
return DECIMAL_DIGITS.indexOf(char) == -1;
}
Line 158:
countryDigit
> It took me a while to work out what you are doing here and then I realised (I
> think) you are counting the amount of times a country digit is found ? Maybe
> call the variable 'amountOfCountryDigitsFound' or 'countryDigitsLength' ?
Line 133 to 167 (inner loop)
for (var i:int = 0; i < length; i++)
Maybe all the code inside this loop could be extracted to a function, that way,
when I look at the validatePostcode function, I can see the big picture in less
lines and then if I care about what is going on inside the 'validating
characters' loop, I can go look in that function.
Line 192 to 218 could be put all of this into a function named something like
processResults(..) ? That way, when reading validatePostcode function we can
ignore that code unless/until we care about it ?
combining the above two suggestions I can then glance at the validatingPostcode
function and see quite quickly that it:
for each format
validates each character in the postcode
holds the error
converts the errors to results
Line 198 to 216
There seems to be the opportunity to reduce the code here, as its seems to be 3
very similar blocks of code, could you extract this somewhere else and pass in
the differences ? For example:
public static const: ERROR_CODE_INVALID_CHAR:String = "invalidChar";
public static const: ERROR_CODE_WRONG_LENGTH:String = "wrongLength";
public static const: ERROR_CODE_WRONG_FORMAT:String = "wrongFormat";
----
var error:Object = getPreferredError();
addValidationErrorToResults(error.invalidChar, ERROR_CODE_INVALID_CHAR,
validator.invalidCharError);
addValidationErrorToResults(error.wrongLength, ERROR_CODE_WRONG_LENGTH,
validator.wrongLengthError);
addValidationErrorToResults(error.invalidFormat, ERROR_CODE_WRONG_FORMAT,
validator.wrongFormatError);
----
private function getPreferredError():Object
{
errors.sortOn(ERROR_COUNT_FIELD, Array.NUMERIC);
return errors[0];
}
private function addValidationErrorToResults(shouldProcessField:Boolean,
errorCode:String, errorMessage:String):ValidationResult
{
results.push(new ValidationError(true, basefield, errorCode, errorMessage));
}
--
note: not that important, but also noticed a very minor inconsistency in the
naming convention of the error type vs the errorCode and errorMessage:
.invalidFormat, "wrongFormat", .wrongFormatError
wrongLength and invalidChar use wrong and invalid for all variables, whilst
format uses invalid for one and wrong for other two
________________________________
From: Justin Mclean <jus...@classsoftware.com>
To: flex-dev@incubator.apache.org
Sent: Thursday, 8 March 2012, 3:07
Subject: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Hi,
This PostCodeValidator and PostCodeFormatter components are are at stage where
they could be considered for inclusion into the SDK.
The components, tests and example application can be found here:
http://svn.apache.org/viewvc/incubator/flex/whiteboard/jmclean/validators/src/
There's a few minor issues to sort out such as what name space should they live
under and probably some minor code layout issues. (And I would certainly like
at least one person to do a solid review of the code.)
They are well unit tested see PostCodeValidatorTests and PostCodeFormatterTests
for tests.
There is a example application be viewed online here:
http://cdn.classsoftware.com/apacheflex/postcodes/PostCodeValidationExample.html
I'd welcome any feedback people have.
Thanks,
Justin
