Skip to content

Implement exercise - luhn#249

Merged
arcuru merged 6 commits intoexercism:masterfrom
junming403:master
Apr 8, 2019
Merged

Implement exercise - luhn#249
arcuru merged 6 commits intoexercism:masterfrom
junming403:master

Conversation

@junming403
Copy link
Copy Markdown
Contributor

No description provided.

@junming403
Copy link
Copy Markdown
Contributor Author

Implement new exercise luhn, ready for review.

Copy link
Copy Markdown
Contributor

@arcuru arcuru left a comment

Choose a reason for hiding this comment

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

The tests look good, but a few things with the formatting and the example need fixing :)

Comment thread config.json
Comment thread exercises/luhn/luhn_test.cpp Outdated
Comment thread exercises/luhn/example.cpp Outdated
Comment thread exercises/luhn/luhn_test.cpp Outdated
Comment thread exercises/luhn/example.cpp Outdated
Comment thread exercises/luhn/example.cpp Outdated
Comment thread exercises/luhn/example.cpp Outdated
@siebenschlaefer
Copy link
Copy Markdown
Contributor

Currently all the valid numbers in the tests have an even number of spaces or all the digits are 0. Therefore the following (invalid) attempt passes:

bool luhn::valid(const std::string& str) {
    int sum = 0, digit_count = 0, factor = 1 + str.size() % 2;
    for (auto i = 0u; i < str.size(); ++i) {
        if (str[i] == ' ') continue;
        if (str[i] < '0' || '9' < str[i]) return false;
        ++digit_count;
        factor = 3 - factor;
        int digit = (str[i] - '0') * factor;
        if (digit > 9) digit -= 9;
        sum += digit;
    }
    return digit_count > 1 && sum % 10 == 0;
}

Please add a test where the the number is valid and it has an an odd number of spaces, e.g.

BOOST_AUTO_TEST_CASE(valid_number_with_an_odd_number_of_spaces)
{
    const bool actual = luhn::valid("095 245  88");
    const bool expected {true};
    BOOST_TEST(expected == actual);
}

@arcuru
Copy link
Copy Markdown
Contributor

arcuru commented Apr 4, 2019

Hold on. @junming403 please keep the original test that matches the problem specifications https://github.com/exercism/problem-specifications/blob/master/exercises/luhn/canonical-data.json

@siebenschlaefer that does seem like it might be a problem with the tests, however we do try to keep the test suites here identical the main problem specifications. Please change the canonical test suite in the problem-specifications repo so all the other tracks can benefit too :)

@siebenschlaefer
Copy link
Copy Markdown
Contributor

@patricksjackson Thanks, I didn't realize the rules are that tight. I submitted a PR to the problem-specifications repo: exercism/problem-specifications#1500

@arcuru
Copy link
Copy Markdown
Contributor

arcuru commented Apr 4, 2019

@siebenschlaefer It's not really that the rules are tight, it's that I don't want to be in a position to maintain a good set of tests, so it's easier to let the central team do it. There are actually some places where we might have an extra test for a C++ specific thing, but those are the only exceptions we generally make.

@junming403 I'll come back and review your updates when I get the chance, it might take until the weekend.

Comment thread exercises/luhn/example.cpp Outdated
if (*c == ' ') {
continue;
} else if (isdigit(*c)) {
int digit = (int)*c - '0';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

digit should be const

Comment thread exercises/luhn/example.cpp
@junming403
Copy link
Copy Markdown
Contributor Author

Updated according to review

@arcuru arcuru mentioned this pull request Apr 8, 2019
48 tasks
@arcuru arcuru merged commit 6801164 into exercism:master Apr 8, 2019
@arcuru
Copy link
Copy Markdown
Contributor

arcuru commented Apr 8, 2019

Thanks!

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