You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flex.apache.org by Justin Mclean <ju...@classsoftware.com> on 2012/03/08 04:07:24 UTC
[Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Hi,
This PostCodeValidator and PostCodeFormatter components are are at stage where they could be considered for inclusion into the SDK.
The components, tests and example application can be found here:
http://svn.apache.org/viewvc/incubator/flex/whiteboard/jmclean/validators/src/
There's a few minor issues to sort out such as what name space should they live under and probably some minor code layout issues. (And I would certainly like at least one person to do a solid review of the code.)
They are well unit tested see PostCodeValidatorTests and PostCodeFormatterTests for tests.
There is a example application be viewed online here:
http://cdn.classsoftware.com/apacheflex/postcodes/PostCodeValidationExample.html
I'd welcome any feedback people have.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
>>> *) 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?
>
> I attached three examples how it think it could work else. All have less dependencies and better performance.
I like what you did with the POST_CODE_FORMAT. I had to look twice to see what you had done with the ='s and why at first I thought it was missing values and ;'s. The code is not is a style (using public cost objects as maps) that's used elsewhere in the SDK and certainly not any of the other validators. So I guess the question is do we want to break with tradition with a plan to change the other validators at a later stage? Might need to get a few other peoples input on this.
BTW Performance wise it may actually be worse (I'm not measured) as I don't believe the POST_CODE_FORMAT assignment code gets JITed (but not 100% sure). I don't think the few ms either way here are an issue in this case so that probably no need to have a discussion about that.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> What is the 1.5? We should try and not use any magic numbers/strings.
A few lines above is the following comment.
// We want invalid char and invalid format errors to show in preference
// so give wrong length errors a higher value
Re magic number string I agree in general but in this case my reasoning at the time was not to as:
1. It totally internal to that one function
2. It's only used in one place and I can't see the potential for it to be used elsewhere
3. It actually value is meaningless (just has to >1 and <2)
It is possible that this would be a tiny bit clearer.
Number(invalidFormat) + Number(invalidChar) + Number(incorrectFormat) + Number(wrongLength) * IMPORTANT_ERROR
If you look at the other validators they have hard coded strings and values all over the place. This code is most likely of a higher standard that what already exists in the Flex SDK both as originally submitted and with all the changes based on feedback.
If it really offends anyone that much please submit a patch :-) And don't forget to run the unit tests!
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
While I'm still not convinced that wide character support needs to be built into the PostCodeValidator class I think the simplest way is to do so is use the flash.globilization.Collator class and the equals method to compare character by character in all operations with ignoreCharacterWidth set to false.
If I code it up would anyone be willing to test it/write unit test for it?
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
Well that was actually quite easy - changes checked in. Just guess it goes to show that coding can be quicker than discussion :-) I'd still appreciated someone to supply alphanumeric unit tests (the wide character numeric tests pass fine) and to compile and test the sample application with full width alpha numeric postcodes (UK or Canadian for example).
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> As far as I know Firefox has never been webkit based. It's based on
> Mozilla's Gecko engine.
Yep I got that wrong (it's late here)- sorry. So you know what browsers it is supported in?
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
Just a not that the generated ASdoc files have been updated in both locations.
And if anyone want help generating ASDocs just ask. I'd forgotten how fiddly they can be.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
I've placed the generated ASdoc from the formatter and validator classes in the whiteboard but it not very readable there.
So here are some alternative links:
http://cdn.classsoftware.com/apacheflex/postcodes/asdocs/mx/formatters/PostCodeFormatter.html
http://cdn.classsoftware.com/apacheflex/postcodes/asdocs/mx/validators/PostCodeValidator.html
This has been generated (in isolation of the full SDK) so most of the external links etc are broken.
There a little work to do formatting wise (and describe the constants) but feedback is welcome.
There are most likely a few spelling errors and like and I'd appreciate someone who's better at technical writing than me have a look at it.
May be good to compare it with the existing ZipCode documentation (althoughthis includes a few extra link and examples and the like).
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/mx/formatters/ZipCodeFormatter.html
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/mx/validators/ZipCodeValidator.html
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Omar Gonzalez <om...@gmail.com>.
> Both HTML and flash applications? As I said it works in Safari for me
which is WebKit based like Firefox.
As far as I know Firefox has never been webkit based. It's based on
Mozilla's Gecko engine.
-omar
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> the IME has not much to do with that.
Validators are intended to be use with UI components usually text input field which have IME support. The documentation makes it fairly clear (to me) that the responsibility should be in with the text field (via IME) not in the validator.
> User is always the highest authority and can always change the IME conversionMode in a System toolbar!
You can set the the compatibility mode via ActionScript. Have a look at "Setting the IME conversion mode" and "Disabling the IME for certain text fields". It even mentions numeric only input fields ie postcodes.
I had a quick try of this and it setting the IME did work in Safari in OSX but I don't know how it works on other OS or browsers. I assume it's widely supported or that would be mentioned in the documentation. (I may of missed it?)
> Beyond that this recommendation system does not work in Chrome or Firefox, just Internet Explorer supports it, Opera completely left me standing in the rain....
Both HTML and flash applications? As I said it works in Safari for me which is WebKit based like Firefox. Any links that show this to be the case for Flesh/Flex?
> Well anyways: IME is to help make the users life easier but he can input both full and half width characters at any time! Which means it is related to the Validator imho.
Yep but you can disable/change mode and even convert character use using IME support in Flash.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the
SDK
Posted by Martin Heidegger <mh...@leichtgewicht.at>.
On 11/03/2012 11:46, Justin Mclean wrote:
> Hi,
>
> Just looked into the wide character issue and it seem Flex does have support for this via IME (Input Method Editor). These some info here:
> http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/system/IME.html
> http://livedocs.adobe.com/flex/3/html/help.html?content=18_Client_System_Environment_6.html
>
> This works on the TextInput not on the Validator, so Validator class support it is probably not the way forward. At this point I think it may be best to remove wide number support from the class and I'll see if I can get an wide number/character example working via IME.
>
> Thanks,
> Justin
Hello Justin,
the IME has not much to do with that. The IME class lets programmers
tell the browser that this particular field is supposed to get a
particular input but the User is always the highest authority and can
always change the IME conversionMode in a System toolbar! Beyond that
this recommendation system does not work in Chrome or Firefox, just
Internet Explorer supports it, Opera completely left me standing in the
rain.... Well anyways: IME is to help make the users life easier but he
can input both full and half width characters at any time! Which means
it is related to the Validator imho.
yours
Martin.
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
Just looked into the wide character issue and it seem Flex does have support for this via IME (Input Method Editor). These some info here:
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/system/IME.html
http://livedocs.adobe.com/flex/3/html/help.html?content=18_Client_System_Environment_6.html
This works on the TextInput not on the Validator, so Validator class support it is probably not the way forward. At this point I think it may be best to remove wide number support from the class and I'll see if I can get an wide number/character example working via IME.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Omar Gonzalez <s9...@apache.org>.
On Sat, Mar 10, 2012 at 2:21 PM, Justin Mclean <ju...@classsoftware.com>wrote:
> Hi,
>
> > Well the conversion game from boolean to number is best *read* like this:
> So on one hand with have 4 if statements (an extra error condition has
> been added) and a extra variable vs a single line of code. IMO the single
> line of code is just as readable even if it does uses casts.
>
> Number(invalidFormat) + Number(invalidChar) + Number(incorrectFormat) +
> Number(wrongLength) * 1.5
>
>
What is the 1.5? We should try and not use any magic numbers/strings. What
does the 1.5 mean? Why isn't it 1.1, or 2.5? I think that should be a
constant that describes its purpose. Not trying to be nitpicky, but if
someone picks up this code later they're not going to know why you chose
1.5 and if there is a reason for that specific number.
--
Omar Gonzalez
s9tpepper@apache.org
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> Well the conversion game from boolean to number is best *read* like this:
So on one hand with have 4 if statements (an extra error condition has been added) and a extra variable vs a single line of code. IMO the single line of code is just as readable even if it does uses casts.
Number(invalidFormat) + Number(invalidChar) + Number(incorrectFormat) + Number(wrongLength) * 1.5
> // I wouldn't use "count" as it doesn't mean anything
Take your point with "count" and I've renamed it.
> Actually double width characters can be typed in accidentally easily.
Currently the only case where this might occur would be for validation of a from by a Japanese user for a locale with alpha numeric postcodes (eg Canadian postcodes). I guess that while unlikely is possible. May require changes to the Validator base class I'll look into it.
> However: This "validator" usually sits in front of a server-side validation - right? Some server-side validation accepts full width, some doesn't. It should optional.
Validatiors don't do any data conversion that just say if it is valid or not. I'll give some though on how to have wide characters optional.
> How about using the character "Ｎ" instead of "N" to check for double width numbers
The combination of formats you would have to supply would be too large. eg "NNNN" would expand in 13 odd combination of N and wide N's (assuming mixed normal and wide format characters are valid).
Currently the existing mx validators do not support wide characters so anything to support that would be a bonus.
> Typo on my part: I will *note* a refactoring of the current signatures :)
They currently work and until we have tests and see what coverage they give I'm very reluctant to make any changes to them. While we as programmers may care about how the code is written (and for many good reasons) the users of the SDK just want to it work and for it not contain any surprises.
> I have never had that problem without the "@private" statement but I admit that I started from a late version of flex to use asdoc.
It's how the methods are marked up in the existing classes so I don't see a need to change it. I'll see if I can get ASdocs to generation from these new classes cleanly.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the
SDK
Posted by Martin Heidegger <mh...@leichtgewicht.at>.
On 10/03/2012 12:22, Justin Mclean wrote:
>
>> Couldn't you just use var invalidFormat: Number = 0 or 1 ?
> I could but then I would be casting a Number to boolean on the other fields.
Well the conversion game from boolean to number is best *read* like this:
var wrongNess: Number = 0.0; // I wouldn't use "count" as it
doesn't mean anything, wrongFactor might be something...
if( invalidFormat ) {
wrongNess = 1.0;
}
if( invalidChar ) {
wrongNess += 1.0
}
if( wrongLength ) {
wrongNess += 1.5:
}
>> Personally I see bad sides in the one with the missing else ... however: again: What did the Flex Gui
> The SDK code is inconsistent in this case and uses both forms.
Then we should write a consistent shape on a wiki page. (see other
mailinglist entry about code convention in wiki)
>
>> I made the experience that programming for possibilities is a very bad idea. I am not necessarily talking about the performance (which it also affects) but
>> also the readability.
> I think the readability is fine and it has been improved in the latest checked in version.
I am not of the believe that putting code in function increases
readability. Stepping through with the debugger However: private static
functions are the least evil.
That being said: programming for possibilities is usually a really bad
idea. I think it can be implemented again when its time has come. By
then the structure of the code may already
be entirely different.
> Personally it more how people want to use it, standards are only as
> good as how much they are used eg country code should in some cases be
> used in postcodes but is generally ignored. I've added support and
> test cases for it.
Its not about a "standard" its about a definition: What does the
documentation of the tool say? Does it say: "It can verify any code" "It
can verify some code" "It can very special code" "It verifes ITU
standardized", .... etc. I wasn't sure about this, that's all.
> May need to support that down the track if any country postcodes
> contain alpha characters and use wide characters. Currently I'm not
> aware of any.
Actually double width characters can be typed in accidentally easily. If
you hit a wrong key on the keyboard then you suddenly type in
full-width. I think most japanese people are used
to check for single width but it depends on the font and the eye-sight
to spot the differences. The real problem is that that they do use them
in office documents (where they usually write addresses -> copy paste).
However: This "validator" usually sits in front of a server-side
validation - right? Some server-side validation accepts full width, some
doesn't. It should optional. How about using
the character "Ｎ" instead of "N" to check for double width numbers
(only!) and "Ａ" instead of "A" to check for double-width characters.
All this stuff sounds already a lot like reg-exp.
>> I do not see a need for this to be consistent, does it? Anyways: I will not a refactoring of this signature for Flex X somewhere.
> I think it does need to be consistent with the current validators. Forms will often use a variety of validators and often place them into a single array so the entire form can be validated in one go. Having them all work and look the same makes them easier to use.
Typo on my part: I will *note* a refactoring of the current signatures :)
>> Then my question would be: Why is it private?
> It's not private it's public the "@private" comment is for the documentation so it handles setters and getters correctly.
I have never had that problem without the "@private" statement but I
admit that I started from a late version of flex to use asdoc.
yours
Martin.
<http://opensource.adobe.com/wiki/display/flexsdk/Coding+Conventions>
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> Couldn't you just use var invalidFormat: Number = 0 or 1 ?
I could but then I would be casting a Number to boolean on the other fields.
>>> *) You mix "brace in new line" (left) with "brace in current line" (right), afaik its in the current line.
> PostCodeValidator - 499 & 513 just as one example
Think I've fixed them all. When some work is done of the Flex formatter I'll run the code through that.
> Personally I see bad sides in the one with the missing else ... however: again: What did the Flex Gui
The SDK code is inconsistent in this case and uses both forms.
> I made the experience that programming for possibilities is a very bad idea. I am not necessarily talking about the performance (which it also affects) but
> also the readability.
I think the readability is fine and it has been improved in the latest checked in version.
> The question is how this util is defined
Personally it more how people want to use it, standards are only as good as how much they are used eg country code should in some cases be used in postcodes but is generally ignored. I've added support and test cases for it.
> This is a big problem for all number validation in japan. Oh: They also have double width A-Z ＡＢＣ．．．ＸＹＺ: try to build a search engine around that!
May need to support that down the track if any country postcodes contain alpha characters and use wide characters. Currently I'm not aware of any.
> I see thanks! And thinking about it further: shouldn't the "result" have a error-weight? To be compared with other errors?
It could be. By default only the first error is shown to the user when validating forms and the like. Also none of the other validators do that.
> I do not see a need for this to be consistent, does it? Anyways: I will not a refactoring of this signature for Flex X somewhere.
I think it does need to be consistent with the current validators. Forms will often use a variety of validators and often place them into a single array so the entire form can be validated in one go. Having them all work and look the same makes them easier to use.
> I attached three examples how it think it could work else. All have less dependencies and better performance.
Better performance is not really an issue here - how fast do you need to validate a post code as a use types?
> Then my question would be: Why is it private?
It's not private it's public the "@private" comment is for the documentation so it handles setters and getters correctly.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
HI,
>> 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)
> Just because it compiles to this version doesn't mean it will work from this version on?! it is compilable with earlier SDK's as well ?!
To be honest I have no idea and there's not really an easy way of finding out without running against every version. From looking though the SDK the convention does seen to be just put in the current FP version number not the earliest possible.
Allthese are good questions as other submissions will also have to deal with the same issues.
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the
SDK
Posted by Martin Heidegger <mh...@leichtgewicht.at>.
On 10/03/2012 09:03, Justin Mclean wrote:
> Couldn't find an instance of this can you point out which file?
Sorry, my fault, was browsing old version.
> I don't see it as an issue as the alternative would involve 3 if
> statements and an extra variable.
Couldn't you just use var invalidFormat: Number = 0 or 1 ?
> 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.
PostCodeValidator - Line 560
>
>> *) 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.
I think there is a definite standard on that in the code guidelines of
flex. however:
PostCodeValidator - 499 & 513 just as one example
>> *) 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.
It would be good to add that case to the documentation.
>
>> *) 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.
Personally I see bad sides in the one with the missing else ... however:
again: What did the Flex Gui
> It may be nice to know all errors for formats at a later date. We're
> only talking about very small arrays here.
I made the experience that programming for possibilities is a very bad
idea. I am not necessarily talking about the performance (which it also
affects) but
also the readability.
>> *) 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.
The question is how this util is defined (documentation) if it validates
to the ITU standard it should mention that.
But if its about usability: Japanese people certain
>
>> *) Japanese do have wide format numbers that they also like to use in postal adresses: ０１２３４５６７８９
> I'll also look into this.
This is a big problem for all number validation in japan. Oh: They also
have double width A-Z ＡＢＣ．．．ＸＹＺ: try to build a search engine
around that!
>> *) 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.
I see thanks! And thinking about it further: shouldn't the "result" have
a error-weight? To be compared with other errors?
> 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 do not see a need for this to be consistent, does it? Anyways: I will
not a refactoring of this signature for Flex X somewhere.
>> *) 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:-)
Same here: not sure this it is necessary to continue that trend :-)
>> *) 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?
I attached three examples how it think it could work else. All have less
dependencies and better performance.
> 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)
Just because it compiles to this version doesn't mean it will work from
this version on?! it is compilable with earlier SDK's as well ?!
>> *) @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"];
Then my question would be: Why is it private?
yours
Martin.
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
>> *) I have seen Japanese people writing their postal code with the unicode character prefix: 〒 That is not an accepted Format
The simplest way of doing this is to treat it as a country code. I've modified this to the code to accept this. From what I could find it usually has a space after it right?
>> *) Japanese do have wide format numbers that they also like to use in postal adresses: ０１２３４５６７８９
Checked in. I've just treated these as ordinary digits so they can be use in other postcodes as well.
>> *) Having no format equals: Never an error? Shouldn't one format be required?
I've also added this and checked for invalid formats.
Thanks again for the feedback.
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
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
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the
SDK
Posted by Carol Frampton <cf...@adobe.com>.
Ok, just looked it up. I was wrong.
If the various branches of an if/else statement involve single statements,
don't make them into blocks.
But if any branch has multiple statements, make all of them into blocks.
ref:
http://opensource.adobe.com/wiki/display/flexsdk/Coding+Conventions#CodingC
onventions-ifstatements
(doesn't seem to be finding if statement section so search for "If the
various branches of an if/else")
Carol
On 3/12/12 5 :13PM, "Justin Mclean" <ju...@classsoftware.com> wrote:
>HI,
>
>> One more clarification. If there is an else it should then be
>>
>> if (expresion)
>> {
>> statement;
>> }
>> else
>> {
>> statement;
>> }
>
>Even for single line statements?
>
>I've seen quite a lot of missing braces in if/elses when there only one
>line. I've also seen braces for single line if statements in the
>existing code base.
>
>Picking a file I've look at recently (Validator.as) it contains code like
>this.
> if (!suppressEvents)
> {
> dispatchEvent(resultEvent);
> }
>
> if (obj == null)
> {
> //return true;
> }
>
> if (_trigger)
> return _trigger;
>
> else if (_source)
> return _source as IEventDispatcher;
>
>
>I particularly like the second example :-) I guess it possible that the
>braces were added after the return true was commented out so not to have
>the condition effect the line below. The blank line in the 3rd example
>certainly doesn't help comprehension of the code.
>
>Justin
>
>
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
HI,
> One more clarification. If there is an else it should then be
>
> if (expresion)
> {
> statement;
> }
> else
> {
> statement;
> }
Even for single line statements?
I've seen quite a lot of missing braces in if/elses when there only one line. I've also seen braces for single line if statements in the existing code base.
Picking a file I've look at recently (Validator.as) it contains code like this.
if (!suppressEvents)
{
dispatchEvent(resultEvent);
}
if (obj == null)
{
//return true;
}
if (_trigger)
return _trigger;
else if (_source)
return _source as IEventDispatcher;
I particularly like the second example :-) I guess it possible that the braces were added after the return true was commented out so not to have the condition effect the line below. The blank line in the 3rd example certainly doesn't help comprehension of the code.
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the
SDK
Posted by Carol Frampton <cf...@adobe.com>.
On 3/9/12 5 :18PM, "Carol Frampton" <cf...@adobe.com> wrote:
>I didn't look at the code but I can tell you what the current style guides
>says.
>
>An if statement should look like
>
> if (expression)
> statement;
One more clarification. If there is an else it should then be
if (expresion)
{
statement;
}
else
{
statement;
}
>
>or if multiline
>
> if (expression)
> {
> statement1;
> statement2;
> }
>
>and there should be a space after the "if".
>
>This is not to open a debate on style - please don't. I am just reporting
>what is currently documented.
>
>Carol
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the
SDK
Posted by Carol Frampton <cf...@adobe.com>.
I didn't look at the code but I can tell you what the current style guides
says.
An if statement should look like
if (expression)
statement;
or if multiline
if (expression)
{
statement1;
statement2;
}
and there should be a space after the "if".
This is not to open a debate on style - please don't. I am just reporting
what is currently documented.
Carol
On 3/9/12 2 :30PM, "Martin Heidegger" <mh...@leichtgewicht.at> wrote:
>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.
>
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the
SDK
Posted by Martin Heidegger <mh...@leichtgewicht.at>.
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.
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
Thanks for the feedback.
I've made most of the suggested changes and will check in the code shortly. A lot of your suggestions also apply to the existing mx validators.
> public static const: ERROR_CODE_WRONG_FORMAT:String = "wrongFormat";
Changed but you might want to note that the existing mx validators have these hard coded.
>> Could this condition not be determined before getting to the start of the loop (inner loop)
It was needed to be in the loop so that any formatting errors can be caught and displayed.
> DECIMAL_DIGITS.indexOf(char) == -1
Again existing mx validators are written in this style but I like the suggestion as it makes for more readable code.
It would probably make sense to move these methods up to the base class at some point.
> combining the above two suggestions I can then glance at the validatingPostcode function and see quite quickly that it:
>
> for each format
> validates each character in the postcode
> holds the error
> converts the errors to results
I've made it close to that.
> There 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 pass in the differences ?
Perhaps but I left it as it is. IMO changing actually adds more code and doesn't really make it any easier to understand.
> private function getPreferredError():Object
> {
> errors.sortOn(ERROR_COUNT_FIELD, Array.NUMERIC);
> return errors[0];
> }
Again IMO replacing a one line of code with a function call (even a descriptive one) doesn't make the code any more or less understandable.
> note: not that important, but also noticed a very minor inconsistency in the naming convention of the error type vs the errorCode and errorMessage.
Again just based on what existing mx validators had. :-)
Thanks,
Justin
Re: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Posted by Daniel Harfleet <dh...@yahoo.com>.
Hi Justin,
Appreciate the efforts you making in getting things done. As a person who likes feedback myself I have put together a few comments, please accept these as feedback rather than criticism, I certainly don't want you to think your efforts are not appreciated.
Unfortunately I have only had time to look at one function in one class,
hth
dan
PostcodeValidator.validatePostcode(...)
--
In general, seems to be a few literal strings which could be converted to constants, e.g.:
private static const FORMAT_CHAR_NUMBER:String = "N";
private static const FORMAT_CHAR_LETTER:String = "A";
private static const FORMAT_CHAR_COUNTRY_CODE:String = "C";
public static const: ERROR_CODE_WRONG_FORMAT:String = "wrongFormat";
etc
Line 107:
107 var noformats:int;
> Ambiguous variable name; does it mean 'number of formats' or 'no formats', if number of formats, suggest rename to 'numberOfFormats'
---------------
Line 129:
129 if (postCode) {
130 length = postCode.length;
131 }
> Why make this check here, from what I can see, the value of postCode does not change within the loop, so why not make the check and assignment prior to the loop and avoid multiple checks ?
> I note also, that this condition would never occur when called by doValidation(), but accept it could happen if called by something else.
Line 122:
for (var f:int =
> this seems a strange variable name; I would suggest either sticking to i, j, etc or if using a more meaningful name like 'formatIndex' or something like that.
Line 138:
138 // ignore character past end of format string
139 if (i >= formatLength) {
140 break;
141 }
> Could this condition not be determined before getting to the start of the loop (inner loop) and then the i < length be a value dependent on i >= 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 could be brought higher and used to determine the length variable in the loop ? 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 meaningful code, e.g.
if(reachedEndOfFormatString(position, formatLength))
{
break;
}
or if that feels like overkill:
if( characterPosition >= formatLength)
{
...
Line 143:
DECIMAL_DIGITS.indexOf(char) == -1
Suggest 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, for example put DECIMAL_DIGITS.indexOf(char) == -1 inside of a function called noDecimalDigitsIn, then your code reads more meaningful to those scanning through:
if( noDecimalDigitsIn(char) && noRomanDigitsInChar(char) && noSpacersInChar(char)
{
etc
private function noDecimalDigitsInChar(char:String):Boolean
{
return DECIMAL_DIGITS.indexOf(char) == -1;
}
Line 158:
countryDigit
> 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' or 'countryDigitsLength' ?
Line 133 to 167 (inner loop)
for (var i:int = 0; i < length; i++)
Maybe 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.
Line 192 to 218 could be put all of this into a function named something like processResults(..) ? That way, when reading validatePostcode function we can ignore that code unless/until we care about it ?
combining the above two suggestions I can then glance at the validatingPostcode function and see quite quickly that it:
for each format
validates each character in the postcode
holds the error
converts the errors to results
Line 198 to 216
There 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 pass in the differences ? For example:
public static const: ERROR_CODE_INVALID_CHAR:String = "invalidChar";
public static const: ERROR_CODE_WRONG_LENGTH:String = "wrongLength";
public static const: ERROR_CODE_WRONG_FORMAT:String = "wrongFormat";
----
var error:Object = getPreferredError();
addValidationErrorToResults(error.invalidChar, ERROR_CODE_INVALID_CHAR, validator.invalidCharError);
addValidationErrorToResults(error.wrongLength, ERROR_CODE_WRONG_LENGTH, validator.wrongLengthError);
addValidationErrorToResults(error.invalidFormat, ERROR_CODE_WRONG_FORMAT, validator.wrongFormatError);
----
private function getPreferredError():Object
{
errors.sortOn(ERROR_COUNT_FIELD, Array.NUMERIC);
return errors[0];
}
private function addValidationErrorToResults(shouldProcessField:Boolean, errorCode:String, errorMessage:String):ValidationResult
{
results.push(new ValidationError(true, basefield, errorCode, errorMessage));
}
--
note: not that important, but also noticed a very minor inconsistency in the naming convention of the error type vs the errorCode and errorMessage:
.invalidFormat, "wrongFormat", .wrongFormatError
wrongLength and invalidChar use wrong and invalid for all variables, whilst format uses invalid for one and wrong for other two
________________________________
From: Justin Mclean <ju...@classsoftware.com>
To: flex-dev@incubator.apache.org
Sent: Thursday, 8 March 2012, 3:07
Subject: [Proposal] Add PostCodeValidator and PostCodeFormatter to the SDK
Hi,
This PostCodeValidator and PostCodeFormatter components are are at stage where they could be considered for inclusion into the SDK.
The components, tests and example application can be found here:
http://svn.apache.org/viewvc/incubator/flex/whiteboard/jmclean/validators/src/
There's a few minor issues to sort out such as what name space should they live under and probably some minor code layout issues. (And I would certainly like at least one person to do a solid review of the code.)
They are well unit tested see PostCodeValidatorTests and PostCodeFormatterTests for tests.
There is a example application be viewed online here:
http://cdn.classsoftware.com/apacheflex/postcodes/PostCodeValidationExample.html
I'd welcome any feedback people have.
Thanks,
Justin