Skip to content

Validation of IBAN (Internation Bank Account Number) #630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Validation of IBAN (Internation Bank Account Number) #630

wants to merge 2 commits into from

Conversation

digitalica
Copy link
Contributor

Please have a look at this. Although it works for me, there are a few things to think about:

(second try for pull request, previous one was mixed up with PR #628, so this time created new local branch, based before the other changes)

  • IBAN will be introduced all over EU (and more) over the next years, maybe it should be in the main validate.js, and not in additional methods? At least it is a reason to think it over carefully: naming, location, maybe split per country.
  • the code is quite long, maybe it can be shorter / more efficient? Especially the way the regexp for the country specific check can be build in other ways, or even be defined directly for every country (longer, but more clear: now a 'definition' string is parsed to build the regexp)
  • There is a country-specific format check done now, just before the 'real' checksum check. This helps to block invalid IBANs (correct checksum, but incorrect country specific format) but will also block valid IBANS if a new country starts to use IBAN or if a country specific format will change in the future (I doubt the latter will ever happen, the first will though). So I think we should eigther provide 2 checks (iban and ibanwithoutcountrycheck) OR allow unknown countries, and just for those not check the format....
  • At least in theory the check could be more specific, as within the 'local' account number there can be another checksum / limitations that are not checked now. I think this is not really needed though, as the checksum in the IBAN itself is quite strong and countries may stop using their local restrictions anyway to just rely in IBAN in the future.

@digitalica digitalica mentioned this pull request Feb 6, 2013
Closed
@digitalica
Copy link
Contributor Author

thinking about it some more I think the country-specific checks should be moved to a separate check (ibancountryspecific) or removed completely. This will drastically reduce the number of false negatives in the future, at the cost of some false positives. This seems a good tradeoff to me, and is in line with the other validations (email, url) that also allow some invalid strings

@nschonni
Copy link
Collaborator

nschonni commented Feb 8, 2013

What about something like this instead of the case

var ibancountries = {
  AL: "8n16c",
  AD: "8n12c",
...
}
var bbanformat = ibancountries[countrycode];
if (typeof bbanformat === 'undefined') {
    console.log('not found ' + countrycode + ', ' + bbanformat);
} else {
    console.log('found ' + bbanformat);
}

@digitalica digitalica mentioned this pull request Feb 12, 2013
Closed
@digitalica
Copy link
Contributor Author

Just cleaned up the code, based on thinking it over and the suggestion by nschonni.

It is much easier to read and understand now, and will have no false negatives when new countries start supporting IBAN in the future (that will happen I suppose)

This is all I can think of for now.

@jzaefferer
Copy link
Collaborator

Thanks a lot! Just landed this as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@digitalica @nschonni @jzaefferer