Skip to content

Hamming exercise submission to exercism/xcpp#1

Closed
sunzenshen wants to merge 11 commits intoexercism:masterfrom
sunzenshen:master
Closed

Hamming exercise submission to exercism/xcpp#1
sunzenshen wants to merge 11 commits intoexercism:masterfrom
sunzenshen:master

Conversation

@sunzenshen
Copy link
Copy Markdown

This first exercise submission will hopefully spur some discussion regarding how the xCpp track should be implemented. The hamming exercise folder seems a bit heavy as is, so there's no doubt that it could be streamlined. And the choice of unit test framework is likely up for debate.

I chose CppUTest as the unit testing framework, partly because it had a large number of stars on GitHub. Support for apt-get and brew didn't hurt either.

This is how I set up my environment on OSX:

  • Installed cmake and cpputest with homebrew
brew install cmake
brew install cpputest
  • The CPPUTEST_HOME environment variable needs to be set to the cpputest directory. In my case, it was at:
/usr/local/Cellar/cpputest/3.5
  • To build the unit tests, create a "build" directory in the hamming exercise folder
mkdir build
cd build
cmake ..
make all
  • To actually run the unit tests, run them with
./bin/RunAllTests

(Where RunAllTests is located in xcpp/hamming/build/bin/)

Sources of research:

@sunzenshen
Copy link
Copy Markdown
Author

For some reason, I forgot to check the forks of xcpp beforehand. So I didn't notice that @DarthStrom already has examples ready, including the hamming exercise. Oh well, I learned some new things and hopefully this CppUTest implementation will be useful to compare with a Google Test one.

@DarthStrom
Copy link
Copy Markdown

No worries. I haven't touched it in a while - feel free to grab anything from it that you think is helpful. The only reason I chose google test is because that's what I'm currently using at work.

@kytrinyx
Copy link
Copy Markdown
Member

I like that both brew and apt-get are available for CppUnit. Is that the case for Google test also?

@DarthStrom
Copy link
Copy Markdown

@kytrinyx
Copy link
Copy Markdown
Member

Ok, in that case I think I'd rather go with CppUnit (unless there are other compelling reasons to choose google test).

@DarthStrom Would you mind giving this PR a review? I don't write C++, so I'm kind of useless here.

@DarthStrom
Copy link
Copy Markdown

Sure, no problem.

@DarthStrom
Copy link
Copy Markdown

Looks good to me. The one suggestion I would make is to maybe try to get rid of the complex directory structure and get the Hamming.cpp and HammingTest.cpp into the same directory.

Consequence of moving Hamming.cpp and HammingTest.cpp to the same
directory.
@sunzenshen
Copy link
Copy Markdown
Author

Really like the idea of simplifying the directory structure. The CMakelists organization is now less modular than when it was split into separate folders, but this is less of a concern given the small scope of these exercises. Hopefully having all the source files in one place is not too overwhelming?

@DarthStrom
Copy link
Copy Markdown

I like that a lot better. Can we also get rid of the .h and just include the .cpp directly?

Idea is to have participants submit a single .cpp file to exercism.
@sunzenshen
Copy link
Copy Markdown
Author

I'm a little worried that including the .cpp file will set a bad example for inexperienced C++ learners, who don't know about the preprocessor. So I added a short warning in HammingTest.cpp.

Hopefully this approach will be more of a learning opportunity than the corrupting of innocent minds. At least now participants only need to worry about a Hamming.cpp file, and not the header file.

@DarthStrom
Copy link
Copy Markdown

Yeah, I guess it's ultimately up to @kytrinyx whether the focus should be on the code itself or if project setup idioms should be a focus as well. Otherwise, I think this looks great!

@DarthStrom
Copy link
Copy Markdown

On second thought, you're right. It's a tradeoff between best practice and simplicity, but I think it's probably better to teach it right in this case.

This rolls back to commit f2174a3.
@sunzenshen
Copy link
Copy Markdown
Author

Okay, let's revert back to providing a header file.

One idea that came to mind was maybe omitting the header file too. However, it seems like a common convention in other exercise repositories to have the participant create one file, and upload that as a submission. For example, the xscala exercises involve creating some folders in the "src" directory, but they involve creating only one exercise file.

I'd prefer to provide a header file, so that it's more explicit what a participant needs to do. Defining the header files hopefully won't limit the creativity and expression of coding styles. Or to put it another way, there should be enough ways to shoot oneself in the foot with just a cpp file.

@kytrinyx
Copy link
Copy Markdown
Member

I think we should prefer right over simple. The idioms and practices are different in each language, and part of being a good citizen in a language is knowing how to follow the principle of least surprise.

@sunzenshen
Copy link
Copy Markdown
Author

In the spirit of teaching idioms, there would be more discussion points if the participant had to define the header file. One thing to point out with CppUTest is that the prototype is not limited in the tests. If we leave out the header file (i.e. renaming Hamming.h to example.h), then it would be up to the user to define what types they want to use.

For example, my example implementation returns an int:

int Compute(std::string, std::string);

Another submission could have defined the return value as an unsigned int:

unsigned int Compute(std::string, std::string);

Both of those prototypes satisfy the tests, which don't specify the types of the input and output:

CHECK_EQUAL(0, Compute("A", "A"));

The only sticking point is that as far as I know about exercism, you can submit only one file. The question of whether a header file needs to be submitted may become an FAQ in of itself. I'm probably over thinking this though. In the C++ exercise documentation, we could ask that only the cpp file be submitted, and the contents of the header file could be inferred. The blind spot with that approach is if the absent header files have mistakes in them, such as a lack of macro guard.

@kytrinyx
Copy link
Copy Markdown
Member

Yeah, I've been meaning to make it possible to submit multiple files, but I haven't made the change yet.

@DarthStrom
Copy link
Copy Markdown

👍

@sunzenshen
Copy link
Copy Markdown
Author

With or without support for submitting multiple files, I don't think it will hurt to begin building the exercise count for this repository. It should be easy to "remove" header files from the exercise if that approach is chosen, and that decision can be postponed until there are 10+ exercises available.

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

I've been doing the exercises for C++ here: https://github.com/LegalizeAdulthood/xcpp/tree/legalize

I'm pretty far along and sent Katrina an email and she pointed me to this thread.

I chose Boost.Test for the unit testing framework because it is widely available for different platforms, can be installed prebuilt easily on Unix and on Windows and you will need other libraries from Boost in order to do some of the exercises anyway. (Like gigasecond, which requires date arithmetic.)

Libraries in Boost often migrate into the C++ standard, so encouraging people to get familiar with Boost while they do the C++ track is a good idea, IMO. I also used CMake for my build but I didn't include that in the github repo. I haven't yet reviewed all the comments on this thread, but I wanted to at least drop in and let people know what I was doing. I'll review this thread in its entirety this weekend.

@sunzenshen
Copy link
Copy Markdown
Author

I'm happy to go with whatever unit testing framework. At first I kinda objected to the idea of bringing in a 3rd party library, but Boost is so commonly used it can't hurt to ask users to install it. As another data point, Homebrew for OSX also supports a "boost" recipe.

Regarding CMake files, I think there should be a discussion about whether or not users should also worry about getting the unit tests to compile. Is there a possibility of exercises becoming more about makefile organization than the actual exercises? Sites like Hacker Rank handle the compilation of C++ code, and it may be helpful to provide similar ease of use. On the other hand, making it the participant's responsibility to get compilation working would remove the maintenance effort that comes with providing makefiles.

At the very least, there should be some instructions on how to compile with (insert unit test framework here), if related files are not included.

@sunzenshen
Copy link
Copy Markdown
Author

This thread looks like it could be going towards a similar discussion regarding user accessibility:
exercism/go#97

If users could get confused figuring out how to run a simple Go program, getting C++ to compile is likely to be an even bigger project in itself.

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 3, 2014

Part of the difficulty here is that some users are completely new to programming, and all the various pieces of setup can seem very arbitrary to them. With people familiar with programming, understanding all the necessary pieces of setup/compilation/whatever is just another part of doing it right. I don't know what the right answer is.

I suspect that the most important thing is to improve all the tools, explanations, READMEs, etc so that it's easier to get an early success with the first problem.

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

Is it possible to have files pushed to the language specific directory when they run the exercism command to start the first exercise for a language track? If so, then I could push a CMakeLists.txt to that directory that solves most of their build problems. Take a look at the instructions I put in my SETUP.md: https://github.com/LegalizeAdulthood/xcpp/blob/legalize/SETUP.md
If I can't give them the file directly, I can provide the recipe and instructions for customization in the SETUP.
Also, I'm looking for a way to "beta test" this language track before making it live. Is there a way to do that?

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

I recently did a workshop on TDD using Boost.Test. In my dry run for the workshop, I had people configure their own build environment before starting. This took approximately 30 minutes just to get a simple static library linked against the console test executable. It was a giant distraction. For the actual deployment of the workshop, I gave them a CMake recipe similar to the one I described in the SETUP.md file. This got them going in about 5-10 minutes after they had the necessary stuff installed (CMake + Boost). CMake gives you IDE workspsaces for Xcode and Visual Studio, as well as generating Makefiles if you're using Unix. So I really think CMake is the best solution to "getting all that build stuff out of the way" and focusing on writing some C++.

Test-Driven Development with Boost.Test

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 3, 2014

We can make it possible to send along a build file, it should be pretty straight-forward. We could add a section to the config.json that tells the API what to send along (filename) and then just deliver it with every exercise and only write it if it is missing. Or something :)

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

I like the idea of writing the file if it is missing. I need someone to beta test my proposed recipe :). My boost setup is a little odd because I've got dev trees and stuff to support me contributing to boost.

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 3, 2014

I'm afraid I'm not the right person to beta test the recipe unless you want a complete C++ n00b ❤️

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

Actually, that's perfect! If a n00b can do it, then we did it right :)

@sunzenshen
Copy link
Copy Markdown
Author

Agreed, complete C++ n00bs would be the perfect individuals for test driving any build recipes. Since we already have example solutions available, they don't even need to know how to write C++. ;)

I would be happy to beta test the recipe, with the caveat that I already have most things installed from exploring C++ unit test frameworks. In absence of exercism support (eg: the language specific directory), maybe there could be a small temporary repo, with a proposed layout of files for a first time user? An example exercism/cpp directory would also clarify how to implement a new config.json entry.

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

FYI, as per an email discussion with Katrina, I will be pushing to the master branch of my clone of the repo. I'm discontinuing use of the 'legalize' branch I had created.

If a pull request is honored for xcpp, does that mean the exercises will show up to anyone using exercism currently?

@sunzenshen
Copy link
Copy Markdown
Author

That works. I'm totally fine with closing this pull request, as the legalize branch looks great. Excited that the xcpp track is making forward progress. 😄

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 3, 2014

FYI, as per an email discussion with Katrina, I will be pushing to the master branch of my clone of the repo.

I actually prefer that you use branches, not master, for pull requests: One branch per exercise. If you found the response confusing, please email me so we can clarify.

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

Gack, I misread what you wanted and inverted the sense. This is the first time I've issued a pull request on github, so I'm a github n00b!

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 4, 2014

No worries, I'm not always very good at explaining things, so this one is on me.

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

I think you did fine, I just confused myself being a github n00b :). OK, I updated description in SETUP.md to say how to get cmake to generate your project setup (i.e. run cmake -G ...).

@kytrinyx
Copy link
Copy Markdown
Member

kytrinyx commented Aug 4, 2014

OK, cool. I'm closing this PR.

@sunzenshen and @DarthStrom - I'll ping both of you to help review the PRs when @LegalizeAdulthood submits them, if that's OK.

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