Skip to content

feat: remove twinPrime #1641

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

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

vil02
Copy link
Member

@vil02 vil02 commented Mar 8, 2024

Open in Gitpod know more

Describe your change:

This PR removes the function twinPrime as suggested in: #1641 (comment)

This PR adds a test covering this line:

return -1

And reorganizes existing tests.

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new JavaScript files are placed inside an existing directory.
  • All filenames should use the UpperCamelCase (PascalCase) style. There should be no spaces in filenames.
    Example:UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not
  • All new algorithms have a URL in their comments that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.92%. Comparing base (4a4ed57) to head (9456fd6).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1641      +/-   ##
==========================================
- Coverage   83.94%   83.92%   -0.02%     
==========================================
  Files         377      376       -1     
  Lines       19727    19698      -29     
  Branches     2917     2911       -6     
==========================================
- Hits        16559    16532      -27     
+ Misses       3168     3166       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vil02 vil02 marked this pull request as ready for review March 8, 2024 21:58
Copy link
Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

This improves coverage of the existing implementation, but I think the existing implementation has a bug: According to Wikipedia

A twin prime is a prime number that is either 2 less or 2 more than another prime number

So the (a) twin prime of 7 should be 5.

@vil02
Copy link
Member Author

vil02 commented Mar 9, 2024

This improves coverage of the existing implementation, but I think the existing implementation has a bug: According to Wikipedia

A twin prime is a prime number that is either 2 less or 2 more than another prime number

So the (a) twin prime of 7 should be 5.

What should then be the twinPrime(5)?

I have to admit that I did not get this function is supposed to do. It would be more natural to have a function like isTwinPrime.

@appgurueu
Copy link
Collaborator

What should then be the twinPrime(5)?

I think either 3 or 7 would be fine.

We may also remove this function entirely, or replace it with an "is twin prime" variant as you suggest, IMO. It's just confusing (and trivial) as is.

@vil02
Copy link
Member Author

vil02 commented Mar 11, 2024

What should then be the twinPrime(5)?

I think either 3 or 7 would be fine.

We may also remove this function entirely, or replace it with an "is twin prime" variant as you suggest, IMO. It's just confusing (and trivial) as is.

I will remove it.

@vil02 vil02 requested a review from appgurueu March 11, 2024 03:45
@vil02 vil02 changed the title tests: add missing test of twinPrime feat: remove twinPrime Mar 11, 2024
@raklaptudirm raklaptudirm merged commit 0204198 into TheAlgorithms:master Mar 11, 2024
@vil02 vil02 deleted the add_missing_test_TwinPrime branch March 12, 2024 18:43
vil02 added a commit to vil02/JavaScript that referenced this pull request Mar 17, 2024
* tests: add missing test of `twinPrime`

* feat: remove `twinPrime`
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.

4 participants