-
Notifications
You must be signed in to change notification settings - Fork 105
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.
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
Feat: Add bank validations #224
base: main
Are you sure you want to change the base?
Feat: Add bank validations #224
Conversation
Hello @mateusbw . We are very happy with your first contribution to our library! I gonna review your code as soon as I have time available. Thanks! |
} | ||
|
||
private getAccountVerificationDigit(accountNumber:string){ | ||
const digitSum = accountNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (9-index),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const digitSum = accountNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (9-index),0); | |
const digitSum = accountNumber.split("").reduce((acc, digit, index) => acc + parseInt(digit) * (9-index), 0); |
} | ||
|
||
private getAccountVerificationDigit(accountNumber:string){ | ||
const digitSum = accountNumber.split("").reverse().map(Number).reduce((acc,cur,index)=> acc + cur * ((index%7+2)),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const digitSum = accountNumber.split("").reverse().map(Number).reduce((acc,cur,index)=> acc + cur * ((index%7+2)),0); | |
const digitSum = accountNumber.split("").reverse().reduce((acc, digit, index)=> acc + parseInt(digit) * ((index%7+2)), 0); |
} | ||
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string, accountType: string){ | ||
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (8 - index),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (8 - index),0); | |
const agencySum = agencyNumber.split("").reduce((acc, digit, index)=> acc + parseInt(digit) * (8 - index), 0); |
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string, accountType: string){ | ||
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (8 - index),0); | ||
const accountTypeSum = accountType.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (4 - index),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const accountTypeSum = accountType.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (4 - index),0); | |
const accountTypeSum = accountType.split("").reduce((acc, digit, index)=> acc + parseInt(digit) * (4 - index),0); |
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string, accountType: string){ | ||
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (8 - index),0); | ||
const accountTypeSum = accountType.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (4 - index),0); | ||
const accountNumberSum = accountNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (9 - index),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const accountNumberSum = accountNumber.split("").map(Number).reduce((acc,cur,index)=> acc + cur * (9 - index),0); | |
const accountNumberSum = accountNumber.split("").reduce((acc, digit, index) => acc + parseInt(digit) * (9 - index), 0); |
} | ||
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string){ | ||
const sum = `${agencyNumber}${accountNumber}`.split("").map(Number).reduce((acc,cur,index) => acc + cur * (11-index),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const sum = `${agencyNumber}${accountNumber}`.split("").map(Number).reduce((acc,cur,index) => acc + cur * (11-index),0); | |
const sum = `${agencyNumber}${accountNumber}`.split("").reduce((acc, digit,index) => acc + parseInt(digit) * (11-index),0); |
} | ||
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string){ | ||
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index) => acc + cur * this.agencyMultiplicationWeights[index],0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index) => acc + cur * this.agencyMultiplicationWeights[index],0); | |
const agencySum = agencyNumber.split("").reduce((acc,digit,index) => acc + parseInt(digit) * this.agencyMultiplicationWeights[index],0); |
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string){ | ||
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index) => acc + cur * this.agencyMultiplicationWeights[index],0); | ||
const accountSum = accountNumber.split("").map(Number).reduce((acc,cur,index) => acc + cur * (index + 4),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const accountSum = accountNumber.split("").map(Number).reduce((acc,cur,index) => acc + cur * (index + 4),0); | |
const accountSum = accountNumber.split("").reduce((acc,digit,index) => acc + parseInt(digit) * (index + 4), 0); |
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur, index)=> { | ||
const digitMultiplication = cur * (isEven(index + 1)? 1 : 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur, index)=> { | |
const digitMultiplication = cur * (isEven(index + 1)? 1 : 2) | |
const agencySum = agencyNumber.split("").reduce((acc, digit, index)=> { | |
const digitMultiplication = parseInt(digit) * (isEven(index + 1)? 1 : 2) |
const accountNumberSum = accountNumber.split("").map(Number).reduce((acc,cur, index)=> { | ||
const digitMultiplication = cur * (isEven(index + 1)? 1 : 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const accountNumberSum = accountNumber.split("").map(Number).reduce((acc,cur, index)=> { | |
const digitMultiplication = cur * (isEven(index + 1)? 1 : 2) | |
const accountNumberSum = accountNumber.split("").reduce((acc, digit, index)=> { | |
const digitMultiplication = parseInt(digit) * (isEven(index + 1)? 1 : 2) |
return number | ||
.toString() | ||
.split('') | ||
.map(Number) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map(Number) |
.toString() | ||
.split('') | ||
.map(Number) | ||
.reduce((a, b) => a + b, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.reduce((a, b) => a + b, 0); | |
.reduce((a, b) => parseInt(a) + parseInt(b), 0); |
} | ||
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string){ | ||
const sum = `${agencyNumber}${accountNumber}`.split("").map(Number).reduce((acc,cur,index) => acc + cur * this.multiplicationWeights[index],0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const sum = `${agencyNumber}${accountNumber}`.split("").map(Number).reduce((acc,cur,index) => acc + cur * this.multiplicationWeights[index],0); | |
const sum = `${agencyNumber}${accountNumber}`.split("").reduce((acc,digit,index) => acc + parseInt(digit) * this.multiplicationWeights[index],0); |
} | ||
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string){ | ||
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index)=> acc + this.getLastDigit(cur * this.agencyMultypliers[index]),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index)=> acc + this.getLastDigit(cur * this.agencyMultypliers[index]),0); | |
const agencySum = agencyNumber.split("").reduce((acc,digit,index)=> acc + this.getLastDigit(parseInt(digit) * this.agencyMultypliers[index]),0); |
|
||
private getAccountVerificationDigit(accountNumber:string, agencyNumber:string){ | ||
const agencySum = agencyNumber.split("").map(Number).reduce((acc,cur,index)=> acc + this.getLastDigit(cur * this.agencyMultypliers[index]),0); | ||
const accountSum = accountNumber.split("").map(Number).reduce((acc,cur,index)=> acc + this.getLastDigit(cur * this.accountMultypliers[index]),0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const accountSum = accountNumber.split("").map(Number).reduce((acc,cur,index)=> acc + this.getLastDigit(cur * this.accountMultypliers[index]),0); | |
const accountSum = accountNumber.split("").reduce((acc,digit,index)=> acc + this.getLastDigit(parseInt(digit) * this.accountMultypliers[index]),0); |
Nice job doing code review @diasbruno ! Hey @mateusbw can you check @diasbruno suggestions? |
Thanks for the review @diasbruno!! @hyanmandian sure, i'll do that!! |
Glad to help! Also, and this is the big one,...In OOP, we only instantiate object of classes if we are encapsulating data and providing behavior. Since the only method |
@@ -0,0 +1,12 @@ | |||
import { BankValidator } from "./bankValidator"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an implementation, not a test.
*/ | ||
export function isValidAccount(bankCode:BankCode, accountNumber: string, agencyNumber?: string) { | ||
const bank = new BANKS[bankCode](); | ||
return bank.isValid(accountNumber, agencyNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of more banks in Brazil...It would be nice to provide something where we can add more banks to BANKS.
addBankValidator(BankCode, Validator);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know if we need that... I think if some bank appears and we don't have support for it, we just can integrate and publish another library version, probably if someone needs that and we don't have it, they can open an issue telling that to us and we just add it. What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only case where I'm a pessimist...It's more likely that people will implement on their end and don't make a PR. 😞
So, it'd be nice to have that, but it's not mandatory.
As requested on #171, this PR adds validation to the following banks:
Banco do Brasil, Santander, CEF, Bradesco, Itau, Real, HSBC, Citibank
The validations goes according to this provided documentation
http://177.153.6.25/ercompany.com.br/boleto/laravel-boleto-master/manuais/Regras%20Validacao%20Conta%20Corrente%20VI_EPS.pdf