New Extension for Portugal #164
Conversation
There was a problem hiding this comment.
Hi @jpassarellijr, thank you very much for this pull request. 🇵🇹 It looks like a great addition! Good job! 👍 I look forward to merging this.
However, I do have a few concerns, they are highlighted below. Let me know if you have any questions!
Thanks,
Brian
| return p.context[Key] as string; | ||
| } | ||
|
|
||
| var id = new int[1] { NifIdentify[new Random().Next(0, NifIdentify.Length)] }; |
There was a problem hiding this comment.
Could you change new Random().Next(... call here? Preferably, it would be p.Random.Number(0, length - 1) or p.Random.Int(0, length - 1). Generally, the source of all randomness needs to come from an existing Bogus Randomizer managed by Bogus to ensure some level of sanity when it comes to providing deterministic behavior. #100 and #101 more details if you're intrested.
When we create new Random objects like this, we break the deterministic behavior for users especially when there is no seed for new Random(). Ultimately, we need to avoid using new Random() objects in Bogus and stick to using an existing Randomizer.
Hope this makes sense.
| /// <param name="c">Object will receive the NIPC value</param> | ||
| public static string Nipc(this Company c) | ||
| { | ||
| var id = new int[1] { NipcIdentify[new Random().Next(0, NipcIdentify.Length)] }; |
There was a problem hiding this comment.
Line here too should change: new Random().Next( with c.Random.Int(0, length-1) or c.Random.Number(0, length-1). See previous comment. 👍
|
First, thank you for acknowledgement, i´m so glad! |
Some name refactoring in the API extension. Bug fix out of bounds array length - 1.
Use Random.ArrayElement utility helper. Updated README extensions docs.
|
Congrats on your first pull request @jpassarellijr ! 👍 😎 Your changes are now live in Thanks again for your help! |
Generate random Tax identification number (NIF and NIPC) for Portugal. Added Facts in the ExtensionTest.