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

Implementation of isogram, secret-handshake, collatz-conjecture, binary-search #192

Closed
wants to merge 9 commits into from
Closed

Implementation of isogram, secret-handshake, collatz-conjecture, binary-search #192

wants to merge 9 commits into from

Conversation

SahilTara
Copy link

@SahilTara SahilTara commented Jul 21, 2018

Hey, I decided to implement some of the exercises needing to be implemented still.
Tell me if anything needs to be changed.
Updated to conform with exercism/exercism#4110 for collatz-conjecture.

@SahilTara SahilTara changed the title Implementation of isogram, secret-handshake, collatz-conjecture Implementation of isogram, secret-handshake, collatz-conjecture, binary-search Jul 26, 2018
@SahilTara
Copy link
Author

Hey, @patricksjackson or @LegalizeAdulthood can one of you please tell me what I need to add/change so that this PR can be merged?

@@ -0,0 +1,23 @@
#include "binary_search.h"

int binary_search::find(const std::vector<int>& arr, int val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we return an iterator?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the late reply, I agree to this completely, and if others want an iterator instead we could just use the built in function in std. I was just thinking we'd have the same return type as some other languages when I implemented this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to do like other languages. The idea is to learn the language, and the best practice is to return an iterator. I don't want to just use the std function because it teaches less.

letters.insert(ch);
}
// character is a member of the alphabet
else if (alphabet.find(ch) != alphabet.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The ifs should be the other way round. Also, shouldn't we use isalpha?

@arcuru
Copy link
Contributor

arcuru commented Mar 2, 2019

Obviously we (well, I) am very behind here, since this has been open 6+ months without me addressing it. Sorry.

There are really two larger issues here before we can move forward with this PR.

  1. This has 4 exercises in it. It's a lot easier if we address each of them individually, so please break this up.
  2. 2 of the exercises (isogram and binary search) already had open existing PRs (one of those I've now merged). Yes, they were old (my fault) but they were still open so I assume they take precedence.

@arcuru
Copy link
Contributor

arcuru commented Mar 23, 2019

Closing this for now, feel free to reopen/split this up.

@arcuru arcuru closed this Mar 23, 2019
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