Skip to content

Conversation

@kim00425
Copy link

Extract gender from infant last name

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #490 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   94.06%   94.09%   +0.02%     
==========================================
  Files         120      120              
  Lines        2056     2066      +10     
==========================================
+ Hits         1934     1944      +10     
  Misses        122      122              
Impacted Files Coverage Δ
src/Services/Air/AirFormat.js 97.91% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 925754a...2cdeb50. Read the comment docs.

@kim00425
Copy link
Author

Extract gender from infant last name

@kim00425 kim00425 closed this Sep 18, 2020
@kim00425 kim00425 reopened this Sep 18, 2020
Copy link
Member

@dchertousov dchertousov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR!

In case of non-INF Gender could be missing (as still could be parsed from name), but for the INF passengers you propose to always include gender based on First name.

IMO, we should to stick for the similar behaviour in all cases.

Also we try not to mutate function params, so I would like to see the code slightly modified:

  • if gender field exists, always take the gender from it
  • if gender field is absent, take the gender from the name
  • if name does not include strings like MR, RS, MSTR MISS etc, set gender to null
  • use separate variable when detecting gender, using rules above instead of modifying function params

Please also make sure, you have included all possible variants of MR/MRS strings and your reg exp is valid.

Current regex has some flaws:

(/MSTR|MISS$/gi).test('MSTRME/NAME')

this one will return true, while it should not. This would return false correctly:

(/(?:MSTR|MISS)$/gi).test('MSTRME/NAME')

@kim00425
Copy link
Author

Thanks for your PR!

In case of non-INF Gender could be missing (as still could be parsed from name), but for the INF passengers you propose to always include gender based on First name.

IMO, we should to stick for the similar behaviour in all cases.

Also we try not to mutate function params, so I would like to see the code slightly modified:

  • if gender field exists, always take the gender from it
  • if gender field is absent, take the gender from the name
  • if name does not include strings like MR, RS, MSTR MISS etc, set gender to null
  • use separate variable when detecting gender, using rules above instead of modifying function params

Please also make sure, you have included all possible variants of MR/MRS strings and your reg exp is valid.

Current regex has some flaws:

(/MSTR|MISS$/gi).test('MSTRME/NAME')

this one will return true, while it should not. This would return false correctly:

(/(?:MSTR|MISS)$/gi).test('MSTRME/NAME')

i did PR again check please

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kim00425 kim00425 requested a review from dchertousov November 24, 2020 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants