incubator-flex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Mclean <jus...@classsoftware.com>
Subject Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Date Sat, 10 Mar 2012 00:03:21 GMT
Hi,

> you already got a lot of feedback from dan, and I agree with him but i have additional
input (please treat as constructive critic):
All feedback is constructive as far as I'm concerned.

> *) The file headers say "Copyright Adobe Systems ... "
Couldn't find an instance of this can you point out which file?

> *) Casting Booleans to Numbers ... I am not sure if I like that.
If you are referring to:
Number(invalidFormat) + Number(invalidChar) + Number(wrongLength) * 1.5

I don't see it as an issue as the alternative would involve 3 if statements and an extra variable.

> *) I you have code without braces if() else I am not sure if that is okay in the current
code convention.
I can't find any instances of that either. I used the "local" bracket style with the exception
of single line if statements which was discussed (briefly) on the list the other week.

if ()
  doSomething();
else
  doSomethingElse();

while compact without braces can lead to errors when modified at a later date.

> *) You mix tabs and spaces! (at many occasions) afaik its 4 spaces indent
The code hasn't been formatted space/tab wise yet and was based on an existing file. Noted
and will be cleaned up/converted down the track.

> *) You mix "brace in new line" (left) with "brace in current line" (right), afaik its
in the current line.
Again I can't find an instance of this in the formatter or validator classes. I find the 
brace on current line more compact and have used that style in the unit tests.

> *) You have string checks like "if(value)" this also checks on "" but that is actually
a valid "format", right?
I had considered "" as a invalid format as it's not particularly interesting. I'll double
check the unit tests around this.

> *) if() { return ... } return; is less good readable compared to if() { return } else
{return} again: Not sure what the style guide says.
The SDK use both styles.

> *) You only need one "Errorset" in the at PostCodeValidator.as - 193 . Why not just store
the error set with the fewest count
It may be nice to know all errors for formats at a later date. We're only talking about very
small arrays here.

> *) PostCodeValidator.as - 564: Why don't you use var but cast to a String again?
Nice catch will check why that was the case. May be from existing mx validator code.

> *) I have seen Japanese people writing their postal code with the unicode character prefix:
〒 That is not an accepted Format
From what I've read the ITU only accepts A-Z and 0-9, but I certainly have no issue in expanding
the character range.

> *) Japanese do have wide format numbers that they also like to use in postal adresses:
0123456789
I'll also look into this.

> *) extraValidation is not sufficiently documented: No mention that the input can be null,
no mention that the output has to be
> one of the error types that it can deal with. I would expect following signature: extraValidation(postCode:String,
baseField:String):Vector.<ValidationResult>;
Will add more documentation. The existing mx class don't use arrays not vectors so changing
it to use vectors could cause confusion in people using multiple validator classes.

> Architecturally:
> 
> *) The static function validatePostCode uses many "properties" of the validator. I would
change the method signature to
The signature is in the same format as the other mx validate classes and is familiar to people
who use them.  I agree your signature is better but it's probably not the time to be changing
them all.

> *) I would put the static functions in their own function file to reduce dependency hell.
Again this is not the style of the currently validiators And I don't want to go down the path
of rewriting all of them just yet:-)

> *) suggestFormat Should be separated from the Validator as it is not "necessary" to use
the Validator (dependencies, dependencies)
I think the connivence of having a single class is better. But happy to be convinced otherwise.
Can you give a code sample?

	<fx:Declarations>
		<validators:PostCodeValidator id="pcv" formats="{postCodeFormats}" source="{postcode}"
property="text" />
	</fx:Declarations>

	postCodeFormats = pcv.suggestFormat(country.selectedItem.locale);


> *) The approach to "wrong-ness" seems quite arbitrary and a ineffective: If a format
has a difference in length of 5 and another format has a difference in length of 200 then
the difference of 5 might be more appropriate.
Sure but in both case they would return exactly the same error, while one may be a better
fit both at the end of the day are not the correct length and that's what reported in the
UI. Again this is along the same lines as other validator classes.

> *) Having no format equals: Never an error? Shouldn't one format be required?
A good change I'll add that.

> *) Why do is it restricted to FP 10.2? Does it not work in former versions? Why do you
use Array instead of Vector in the validator? (FP10.2 supported Vector)
Fp 10.2 is the earliest version the current 4.6 SDK will compile with (and that's only after
I changed the build scripts)

> *) @private
> public function set formats
> 
> What is this setter good for?
For setting multiple formats: eg
validator.formats = ["AN NAA", "ANN NAA", "AAN NAA", "ANA NAA", "AANN NAA", "AANA NAA"];

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