incubator-flex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Heidegger ...@leichtgewicht.at>
Subject Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Date Fri, 09 Mar 2012 19:30:12 GMT
Hello Justin,

you already got a lot of feedback from dan, and I agree with him but i 
have additional input (please treat as constructive critic):

Generally:

*) The file headers say "Copyright Adobe Systems ... "
*) Casting Booleans to Numbers ... I am not sure if I like that.
*) I you have code without braces if() else I am not sure if that is 
okay in the current code conventionn.
*) You mix tabs and spaces! (at many occasions) afaik its 4 spaces indent
*) You mix "brace in new line" (left) with "brace in current line" 
(right), afaik its in the current line.
*) You have string checks like "if(value)" this also checks on "" but 
that is actually a valid "format", right?
*) if() { return ... } return; is less good readable compared to if() { 
return } else {return} again: Not sure what the style guide says.

Specifically
*) You only need one "Errorset" in the at PostCodeValidator.as - 193 . 
Why not just store the error set with the fewest count
var wrongNess: Number = invalidFormat + invalidChar + (wrongLength*1.5);
if( wrongNess > 0 && error && error.wrongNess > wrongNess) {
error = {
wrongNess: wrongNess
};
}
*) PostCodeValidator.as - 564: Why don't you use var but cast to a 
String again?
*) I have seen Japanese people writing their postal code with the 
unicode character prefix: 〒 That is not an accepted Format
*) Japanese do have wide format numbers that they also like to use in 
postal adresses: 0123456789
*) 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>;

Architecturally:

*) The static function validatePostCode uses many "properties" of the 
validator. I would change the method signature to
function validatePostCode(postCode:String, baseField:String, 
formats:Array=null, countryCode:String=null, 
extraValidation:Function=null):Array

This way it doesn't have a dependency to the "dataholder" Validator. 
That might actually make more sense if this function were in its own 
function file.
Consequently you wouldn't have a dependency to PostCodeValidator in 
PostCodeFormatter
*) I would put the static functions in their own function file to reduce 
dependency hell.
*) suggestFormat Should be separated from the Validator as it is not 
"necessary" to use the Validator (dependencies, dependencies)
*) 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.
*) Having no format equals: Never an error? Shouldn't one format be 
required?

And some questions:

*) 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)
*) @private
public function set formats

What is this setter good for?

yours
Martin.

Mime
View raw message