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: ０１２３４５６７８９
*) 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.
