Skip to content

Conversation

@alienx5499
Copy link
Contributor

Description

This PR adds the missing twelve-days practice exercise to the C++ track, addressing issue #985.

What this PR does

  • ✅ Adds complete twelve-days practice exercise implementation
  • ✅ Includes all required exercise files with proper structure
  • ✅ Provides working example solution in .meta/ directory
  • ✅ Updates config.json with exercise metadata
  • ✅ All tests pass with example solution

Exercise Details

Exercise: Twelve Days of Christmas song lyrics generator
Difficulty: 3 (Medium)
Author: alienx5499

Functions Implemented:

  • verse(int day) - Returns lyrics for a specific day
  • lyrics(int start_day, int end_day) - Returns multiple verses
  • lyrics(int end_day) - Returns verses 1 through end_day

Files Added:

exercises/practice/twelve-days/
├── twelve_days.h              # Header with function declarations
├── twelve_days.cpp            # Stub implementation for students
├── twelve_days_test.cpp       # Complete test suite (15 test cases)
├── CMakeLists.txt            # Build configuration
├── .meta/
│   ├── config.json           # Exercise metadata
│   ├── example.h/.cpp        # Working reference solution
│   └── tests.toml           # Canonical test mapping
├── .docs/
│   └── instructions.md       # Exercise instructions
└── test/
    ├── catch.hpp            # Test framework
    └── tests-main.cpp       # Test runner

Testing

  • ✅ Built successfully with CMake
  • ✅ All 15 test cases pass with example solution
  • ✅ Verified with ./bin/configlet sync -e twelve-days (up-to-date)
  • ✅ Follows standard Exercism C++ exercise structure

Implementation Notes

  • Uses vector-based approach for ordinals and gifts storage
  • Follows canonical test data exactly for string matching
  • Implements proper newline handling between verses
  • Stub implementation guides students with TODO comments

Related Issue

Addresses #985 (adds one of the missing practice exercises)


Ready for review! This exercise follows all Exercism C++ track conventions and is fully functional.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2025

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Jul 7, 2025
@alienx5499 alienx5499 mentioned this pull request Jul 7, 2025
55 tasks
@marcelweikum
Copy link
Contributor

Asking for review #985 (comment) @ahans @vaeng @siebenschlaefer

@alienx5499
Copy link
Contributor Author

Hi all — could I please get a review on this PR adding the twelve-days practice exercise (addresses #985)?

Status:

  • ✅ Builds with CMake
  • ✅ Example solution passes all 15 tests
  • ./bin/configlet sync -e twelve-days OK
  • ✅ Follows standard Exercism C++ track layout

Would appreciate if a maintainer could review and merge.

cc @siebenschlaefer @vaeng @ahans

@ahans ahans reopened this Aug 28, 2025
Copy link
Contributor

@ahans ahans 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 contribution and sorry for the long wait! Just some comments to get this more in line with the other exercises ...

@alienx5499
Copy link
Contributor Author

All requested changes have been addressed ✅
Ready for another review 👍

Copy link
Contributor

@ahans ahans 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 addressing my comments. However, it appears to me you mostly vibe code this. Please don't do this blindly. It leads to inconsistencies and broken code. Also please test your solution locally first. Thank you for your understanding.

@ahans
Copy link
Contributor

ahans commented Aug 29, 2025

I also approved the CI runs now, you'll see that they fail.

@alienx5499
Copy link
Contributor Author

Thanks for the review! I’ll admit I used AI in some cases to speed things up, but I didn’t vibe code blindly, I tried to follow the review points. I see now that some of those changes introduced issues, so I’ll rework them carefully, clean up formatting, and make sure everything passes before pushing again.

@alienx5499
Copy link
Contributor Author

When I use constexpr std::string_view expected = ... in tests, I get this linker error:

Undefined symbols for architecture arm64:
  "Catch::StringMaker<std::__1::basic_string_view<char, std::__1::char_traits<char>>, void>::convert(...)"

That's why in my latest commit I used std::string expected = ... for the tests to compile and run.

@alienx5499 alienx5499 requested a review from ahans August 30, 2025 05:49
@ahans
Copy link
Contributor

ahans commented Aug 30, 2025

When I use constexpr std::string_view expected = ... in tests, I get this linker error:

Undefined symbols for architecture arm64:
  "Catch::StringMaker<std::__1::basic_string_view<char, std::__1::char_traits<char>>, void>::convert(...)"

That's why in my latest commit I used std::string expected = ... for the tests to compile and run.

That is surprising, but is probably due to our ancient version of Catch2. That old version doesn't support string_view properly yet, it seems. The issue here is with Catch2 code that is used to print differences when test cases fail. Anyway, sticking to std::string is probably the way to go then.

The reason I advocate for std::string_view and the like in tests is that I think it is good to leave students as many options as possible. In the present case, where the exercise is about generating strings, std::string is the only viable choice (at least as long as we only support C++17), so it's acceptable to have that explicitly in the tests. You could still compare it against a std::string_view or even const char*, so students could still return that. There are other exercises, where a student function needs to accept a string. In that case leaving more options and not hinting at some particular choice via the tests is more important.

Copy link
Contributor

@ahans ahans left a comment

Choose a reason for hiding this comment

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

We're getting there! Thanks for you patience!

@alienx5499
Copy link
Contributor Author

I have implemented all your suggested changes: template argument deduction for std::array, updated namespace structure, and added const qualifiers to expected variables. All changes committed and pushed successfully.

@alienx5499 alienx5499 requested a review from ahans August 30, 2025 09:09
Copy link
Contributor

@ahans ahans left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution. I will fix the redundancy in the example and then merge.

Comment on lines 27 to 69
if (start_day == end_day) {
// Single verse case
auto result = std::string{"On the "} +
std::string{ordinals[start_day]} +
" day of Christmas my true love gave to me: ";

for (int i = start_day; i >= 1; --i) {
if (i == start_day) {
result += std::string{gifts[i]};
} else if (i == 1) {
result += std::string{", and "} + std::string{gifts[i]};
} else {
result += std::string{", "} + std::string{gifts[i]};
}
}

result += ".\n";
return result;
} else {
// Multiple verses case
std::string result = "";
for (int i = start_day; i <= end_day; ++i) {
if (i > start_day) {
result += "\n";
}

auto verse = std::string{"On the "} + std::string{ordinals[i]} +
" day of Christmas my true love gave to me: ";

for (int j = i; j >= 1; --j) {
if (j == i) {
verse += std::string{gifts[j]};
} else if (j == 1) {
verse += std::string{", and "} + std::string{gifts[j]};
} else {
verse += std::string{", "} + std::string{gifts[j]};
}
}

verse += ".\n";
result += verse;
}
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a bit of redundancy in here. You had that better in a previous iteration. But don't worry, I will take care of it.

@ahans ahans added x:action/create Work on something from scratch x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation labels Aug 30, 2025
@ahans ahans merged commit 2bb55dc into exercism:main Aug 30, 2025
8 checks passed
@alienx5499
Copy link
Contributor Author

@ahans Thank you for your time, review, and for merging! 🤍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:action/create Work on something from scratch x:module/practice-exercise Work on Practice Exercises x:rep/large Large amount of reputation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants