Skip to content

Conversation

justinmeiners
Copy link
Owner

fix #48 for @aharrison24

@aharrison24
Copy link
Contributor

aharrison24 commented Dec 30, 2021

Looks good!

Making binary_counter::counter public for testing is understandable, but I wonder whether there's a cleaner way round the debugging requirement. You could consider providing cbegin and cend methods on binary_counter (returning const_iterator) to allow callers to iterate over the elements directly. i.e. counter.cbegin() rather than the uglier counter.counter.begin().

show_counter is essentially a std::copy into a std::ostream_iterator<char>(std::cout, " "). You could call std::copy directly from main and avoid needing to write the show_counter function. On the other hand, having the function raises the level of abstraction in main. And if you were going to replace the loop in the function with a std::copy then you'd need to start using std::iterator_traits<I>::value_type to deduce the underlying element type. On balance I suspect the raw for-loop in a function (as you have it) is cleaner!

@aharrison24
Copy link
Contributor

Sorry, should have said that this is a really good example of how the binary counter should be used and what it does internally. Would have helped me a lot in my early understanding. Thank you!

@justinmeiners
Copy link
Owner Author

justinmeiners commented Dec 30, 2021

public.

I agree it's not great. I was trying to avoid making any modifications to the code, but I think mine is worse since Alex explicitly mentions it being private. I have added begin/end (I believe the cbegin/cend convention is C++11?).

std::copy

I like this better, and chose to remove the function and make it inline.

@justinmeiners justinmeiners merged commit 386e891 into master Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants