incubator-flex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Harfleet <>
Subject Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Date Fri, 09 Mar 2012 16:13:53 GMT
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,





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";


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))

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)

private function noDecimalDigitsInChar(char:String):Boolean
    return DECIMAL_DIGITS.indexOf(char) == -1;

Line 158:


> 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,
    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 <>
Sent: Thursday, 8 March 2012, 3:07
Subject: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK

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:

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:

I'd welcome any feedback people have.

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message