Skip to content
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

Fix: Do not search for wilaya by zip code if it's invalid #24

Closed
Fcmam5 opened this issue Sep 30, 2020 · 11 comments · Fixed by #26
Closed

Fix: Do not search for wilaya by zip code if it's invalid #24

Fcmam5 opened this issue Sep 30, 2020 · 11 comments · Fixed by #26
Assignees
Labels
bug Something isn't working first-timers-only good first issue Good for newcomers Hacktoberfest

Comments

@Fcmam5
Copy link
Collaborator

Fcmam5 commented Sep 30, 2020

In Algeria all ZIP codes are numeric in a form of WILAYA_CODE_XXXX. For example Oran city's code is 31, and it provinces have codes like 31000, 310094 ...

In this line :

const wilaya = data.find(w => w.postalCodes.includes(zipCode));

We are directly looking for the given zip code even if it's not a valid number. I suggest at least checking if the parameter is a number before searching.

@Fcmam5
Copy link
Collaborator Author

Fcmam5 commented Sep 30, 2020

@xxKeefer this could be an issue you can work on 😄 !

@InternetRamen
Copy link

Hey! This seems interesting. Could my friend @makebread and I get assigned this?

@InternetRamen
Copy link

Or could I work with @xxKeefer ?

@Fcmam5
Copy link
Collaborator Author

Fcmam5 commented Sep 30, 2020

@InternetRamen it would be great if you create a validation method (isValidZipCode) and export it like we did with isValidWilayaCode se we can either use it internally or as a utility function. see isValidWilayaCode.

@xxKeefer
Copy link
Contributor

Hey @Fcmam5 I did a bit a of research as I'm not familiar with the Algerian postal system (i'm from Australia :D) it looks like the function getWilayaByZipCode takes a number, my research tells me that some of the zip codes like those with Adrar have a leading zero in the zip code. Does that mean that say if were looking at the zip code for Abani for example ('01027') its looking to receive the number 1027?

@Fcmam5
Copy link
Collaborator Author

Fcmam5 commented Sep 30, 2020

Hey @Fcmam5 I did a bit a of research as I'm not familiar with the Algerian postal system (i'm from Australia :D) it looks like the function getWilayaByZipCode takes a number, my research tells me that some of the zip codes like those with Adrar have a leading zero in the zip code. Does that mean that say if were looking at the zip code for Abani for example ('01027') its looking to receive the number 1027?

Yes it make sense! So, the first two digits refers to the Wilaya (city code), and for Adrar it's 01 with the leading zero. But I think that we still need to verify if it's a valid number at least (even though the parameter can be a string).

So we'd expect:

  • zipCode='01027' or zipCode=31000 to be valid, and we call our search functions.
  • 'zipCode=xy123' to be invalid, so we don't need to run an O(n^2) search :/

Maybe we can for that only using Number.isNaN().


Glad to have you here helping us @xxKeefer ! That's an honour that our project reached Australia 😄 !

@InternetRamen
Copy link

@InternetRamen it would be great if you create a validation method (isValidZipCode) and export it like we did with isValidWilayaCode se we can either use it internally or as a utility function. see isValidWilayaCode.

Thank you for responding. I will try to create the method.

@Fcmam5
Copy link
Collaborator Author

Fcmam5 commented Sep 30, 2020

@InternetRamen @xxKeefer already started working on it in #26, would you help us there?

@InternetRamen
Copy link

@Fcmam5 Got it

@Fcmam5
Copy link
Collaborator Author

Fcmam5 commented Sep 30, 2020

@Fcmam5 Got it

@InternetRamen I just created #27 , I'll add a description there, that one should be a bit challenging as well 😄

@InternetRamen
Copy link

@Fcmam5 I'll try my best! Thanks for the opportunity.

@Fcmam5 Fcmam5 linked a pull request Oct 2, 2020 that will close this issue
3 tasks
@Fcmam5 Fcmam5 closed this as completed in #26 Oct 2, 2020
@ZibanPirate ZibanPirate moved this to Backlog in DzCode i/o Feb 13, 2022
@ZibanPirate ZibanPirate moved this from Backlog to Released in DzCode i/o Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working first-timers-only good first issue Good for newcomers Hacktoberfest
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

3 participants