Mailing-List: contact flex-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: flex-dev@incubator.apache.org
Received-SPF: pass (nike.apache.org: local policy)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
  s=s1024; d=yahoo.com;
  h=X-YMail-OSG:Received:X-Mailer:References:Message-ID:Date:From:Reply-To:Subject:To:Cc:In-Reply-To:MIME-Version:Content-Type;
  b=17z3yh2kHC3HvCg8zNjVqQ4xSeVwIsfO/w/w6Q4Bc9xDnkrSDhKhsCdqV3uMsT4+9+q0vbIeg4hwmIosJTr/zouO94O3c/f8s1hf0DXrgZXwZIjo2ZBmIiKyoXvaRKMjSnbcI1cuKT+NeV1SQ3z1mNgnpcukyFCupZkYc7M5l1k=;
References: <503131DD-2947-4AF1-9815-DD4CFF6B028A@classsoftware.com>
Message-ID: <1331309633.52878.YahooMailNeo@web110414.mail.gq1.yahoo.com>
Date: Fri, 9 Mar 2012 08:13:53 -0800 (PST)
From: Daniel Harfleet <dharfleet@yahoo.com>
Reply-To: Daniel Harfleet <dharfleet@yahoo.com>
Subject: Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
To: "justin@classsoftware.com" <justin@classsoftware.com>
Cc: "flex-dev@incubator.apache.org" <flex-dev@incubator.apache.org>
In-Reply-To: <503131DD-2947-4AF1-9815-DD4CFF6B028A@classsoftware.com>
MIME-Version: 1.0
Content-Type: multipart/alternative;
 boundary="-789465230-2067891103-1331309633=:52878"

---789465230-2067891103-1331309633=:52878
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable

Hi Justin,=0A=0AAppreciate the efforts you making in getting things done. A=
s a person who likes feedback myself I have put together a few comments, pl=
ease accept these as feedback rather than criticism, I certainly don't want=
 you to think your efforts are not appreciated.=0A=0AUnfortunately I have o=
nly had time to look at one function in one class,=0A=0Ahth=0A=0Adan=0A=0A=
=0A=0A=0APostcodeValidator.validatePostcode(...)=0A=0A--=0A=0A=0A=0A=0AIn g=
eneral, seems to be a few literal strings which could be converted to const=
ants, e.g.:=0A=0Aprivate static const FORMAT_CHAR_NUMBER:String =3D "N";=0A=
private static const FORMAT_CHAR_LETTER:String =3D "A";=0Aprivate static co=
nst FORMAT_CHAR_COUNTRY_CODE:String =3D "C";=0A=0Apublic static const: ERRO=
R_CODE_WRONG_FORMAT:String =3D "wrongFormat";=0A=0Aetc=0A=0A=0ALine 107:=0A=
=0A107 var noformats:int;=0A=0A> Ambiguous variable name; does it mean 'num=
ber of formats' or 'no formats', if number of formats, suggest rename to 'n=
umberOfFormats'=0A=0A---------------=0A=0ALine 129:=0A=0A129=A0=A0=A0 if (p=
ostCode) {=0A130 =A0=A0=A0 length =3D postCode.length;=0A131 =A0=A0=A0 }=0A=
=0A=0A> Why make this check here, from what I can see, the value of postCod=
e does not change within the loop, so why not make the check and assignment=
 prior to the loop and avoid multiple checks ?=0A=0A> I note also, that thi=
s condition would never occur when called by doValidation(), but accept it =
could happen if called by something else.=0A=0A=0A=0A=0ALine 122:=0A=0Afor =
(var f:int =3D =0A=0A> this seems a strange variable name; I would suggest =
either sticking to i, j, etc or if using a more meaningful name like 'forma=
tIndex' or something like that.=0A=0A=0ALine 138:=0A=0A=0A138 =A0=A0=A0 // =
ignore character past end of format string=0A139 =A0=A0=A0 if (i >=3D forma=
tLength) {=0A140 =A0=A0=A0 break;=0A141 =A0=A0=A0 }=0A=0A=0A> Could this co=
ndition not be determined before getting to the start of the loop (inner lo=
op) and then the i < length be a value dependent on i >=3D 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 c=
ould be brought higher and used to determine the length variable in the loo=
p ? 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 meani=
ngful code, e.g.=0A=0Aif(reachedEndOfFormatString(position, formatLength))=
=0A{=0A=A0break;=0A}=0A=0Aor if that feels like overkill:=0A=0Aif( characte=
rPosition >=3D formatLength)=0A{=0A=A0=A0=A0 ...=0A=0A=0ALine 143:=0A=0ADEC=
IMAL_DIGITS.indexOf(char) =3D=3D -1=0A=0ASuggest 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, f=
or example put DECIMAL_DIGITS.indexOf(char) =3D=3D -1 inside of a function =
called noDecimalDigitsIn, then your code reads more meaningful to those sca=
nning through:=0A=0Aif( noDecimalDigitsIn(char) && noRomanDigitsInChar(char=
) && noSpacersInChar(char)=0A{=0A=A0etc=0A=0Aprivate function noDecimalDigi=
tsInChar(char:String):Boolean=0A{=0A=A0=A0=A0 return DECIMAL_DIGITS.indexOf=
(char) =3D=3D -1;=0A}=0A=0A=0ALine 158:=0A=0AcountryDigit=0A=0A> 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'=A0 or 'countryDigitsLength' ?=0A=
=0A=0A=0ALine 133 to 167=A0 (inner loop)=0A=0Afor (var i:int =3D 0; i < len=
gth; i++)=0A=0A=0AMaybe 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.=
=0A=0A=0A=0ALine 192 to 218=A0 could be put all of this into a function nam=
ed something like processResults(..) ? That way, when reading validatePostc=
ode function we can ignore that code unless/until we care about it ?=0A=0A=
=0Acombining the above two suggestions I can then glance at the validatingP=
ostcode function and see quite quickly that it:=0A=0Afor each format=0A=A0=
=A0=A0 validates each character in the postcode=0A=A0=A0=A0 holds the error=
=0Aconverts the errors to results=0A=0A=0A=0ALine 198 to 216=A0 =0A=0A=0ATh=
ere 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 p=
ass in the differences ? For example:=0A=0Apublic static const: ERROR_CODE_=
INVALID_CHAR:String =3D "invalidChar";=0Apublic static const: ERROR_CODE_WR=
ONG_LENGTH:String =3D "wrongLength";=0Apublic static const: ERROR_CODE_WRON=
G_FORMAT:String =3D "wrongFormat";=0A=0A=0A----=0A=0A=0Avar error:Object =
=3D getPreferredError();=0A=0A=0AaddValidationErrorToResults(error.invalidC=
har, ERROR_CODE_INVALID_CHAR, validator.invalidCharError);=0AaddValidationE=
rrorToResults(error.wrongLength, ERROR_CODE_WRONG_LENGTH, validator.wrongLe=
ngthError);=0AaddValidationErrorToResults(error.invalidFormat, ERROR_CODE_W=
RONG_FORMAT, validator.wrongFormatError);=0A=0A----=0A=0A=0A=0Aprivate func=
tion getPreferredError():Object=0A{=0A=A0=A0=A0 errors.sortOn(ERROR_COUNT_F=
IELD, Array.NUMERIC);=0A=A0=A0=A0 return errors[0];=0A}=0A=0A=0Aprivate fun=
ction addValidationErrorToResults(shouldProcessField:Boolean, errorCode:Str=
ing, errorMessage:String):ValidationResult=0A{=0A=A0=A0=A0 results.push(new=
 ValidationError(true, basefield, errorCode, errorMessage));=0A}=0A=0A=0A=
=0A--=0Anote: not that important, but also noticed a very minor inconsisten=
cy in the naming convention of the error type vs the errorCode and errorMes=
sage:=0A=0A.invalidFormat, "wrongFormat", .wrongFormatError=0A=0AwrongLengt=
h and invalidChar use wrong and invalid for all variables, whilst format us=
es invalid for one and wrong for other two=0A=0A=0A=0A=0A=0A=0A____________=
____________________=0A From: Justin Mclean <justin@classsoftware.com>=0ATo=
: flex-dev@incubator.apache.org =0ASent: Thursday, 8 March 2012, 3:07=0ASub=
ject: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK=0A =
=0AHi,=0A=0AThis PostCodeValidator and PostCodeFormatter components are are=
 at stage where they could be considered for inclusion into the SDK. =0A=0A=
The components, tests and example application can be found here:=0Ahttp://s=
vn.apache.org/viewvc/incubator/flex/whiteboard/jmclean/validators/src/=0A=
=0AThere's a few minor issues to sort out such as what name space should th=
ey live under and probably some minor code layout issues. (And I would cert=
ainly like at least one person to do a solid review of the code.)=0A=0AThey=
 are well unit tested see PostCodeValidatorTests and PostCodeFormatterTests=
 for tests.=0A=0AThere is a example application be viewed online here:=0Aht=
tp://cdn.classsoftware.com/apacheflex/postcodes/PostCodeValidationExample.h=
tml=0A=0AI'd welcome any feedback people have.=0A=0AThanks,=0AJustin
---789465230-2067891103-1331309633=:52878--