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