Gui pereira cpp exercises #91
Conversation
…ion for the simple task of removing punctuation from a text file. Still missing part on counting number of ocurrences on a vector of elements.
| @@ -0,0 +1,107 @@ | |||
| /** | |||
There was a problem hiding this comment.
You should probably get this working with cmake. CMake is the industry standard make tool for c++, and mantid uses this. Mantids cmake workflow is.... complicated... so it's good get a grounding in this early.
| } | ||
| buffer.str(line); // Write line to buffer | ||
| } | ||
| return *this; // Don't really understand this yet, returns pointer to the object itself? |
There was a problem hiding this comment.
this returns a pointer to the instance, the * before this dereferences it. Combined a reference to the object itself is returned.
|
|
||
| buffer.clear(); | ||
|
|
||
| // Apply punctuaction extraction filter |
There was a problem hiding this comment.
Again, if you find yourself wanting to write a comment like this, refactor into a function called AppyPunctuationFilter or something.
| while (!(buffer >> word_out)) { // Output here | ||
|
|
||
| // if (buffer.bad()) break; // Probably needs something like this, but I am too tired to think about which cases | ||
| if (!ist.good()) break; // !good() covers bad(), fail() and eof() |
There was a problem hiding this comment.
I always prefer retaining the curly braces for clarity. I've seen so many bugs caused by these one line if statements.
| } | ||
| } | ||
| // Count ocurrences of a word | ||
| std::map <std::string, int> counts = CountOcurrencesInVector(word_list); |
There was a problem hiding this comment.
Given your implementation, you could use std::unordered_map which is just a straight up hash table so more efficient.
There was a problem hiding this comment.
We're looping over the words again here. Could we have counted them during the previous loop?
| return ist.good(); // It makes more sense to be buffer.good() but that doesn't work | ||
| } | ||
|
|
||
| std::map<std::string, int> CountOcurrencesInVector(std::vector<std::string> all_words) { |
There was a problem hiding this comment.
You're passing by value here, which makes a copy of the vector. This can be expensive, so you will want to pass by reference or pointer if possible (ideally the former).
In addition, if you know you will not want to mutate the passed object in the function, you will want to pass a cost object.
There was a problem hiding this comment.
It's worth reviewing your use of variables throughout, after reading up on "const correctness". If it doubt, use a const.
|
|
||
| std::map<std::string, int> counts; | ||
|
|
||
| for (std::string word : all_words) { |
There was a problem hiding this comment.
Typically in range based for loops we prefer to use auto, and given that we don't want to create a new copy or change the item, we can just auto const &: for (auto const &word : all_words)
| int base; | ||
| int radius; | ||
| std::string type; | ||
| Polygon (int h, int b) : height {h}, base {b} {}; |
There was a problem hiding this comment.
Using this design, the variable radius is uninitialized. This could lead to unpredictable behaviour, and in fact most linting tools (including ours) will not let it through.
| class Rectangle: public Polygon { | ||
| public: | ||
| Rectangle (int h, int b) : Polygon(h, b) {type = "Rectangle";}; | ||
| double get_area () {return height*base;}; |
There was a problem hiding this comment.
There is an implicit cast here from int to double, as you are multiplying two ints (returning and int), then assigning it to a double.
Again, linters and compilers will likely complain about this.
By not using CMake your compiler probably isn't configured to output these warnings?
If you look in Cmake/CommonSetup.cmake there is this code block:
if(MSVC)
add_compile_options(/W3 /WX) # W3 avoids std lib warnings
else()
add_compile_options(-Wall -Wextra -pedantic -Werror)
endif()
| class Circle : public Polygon { | ||
| public: | ||
| Circle (int r) : Polygon(r) {type = "Circle";}; | ||
| double get_area () {return 3.14*radius*radius;}; |
There was a problem hiding this comment.
If our function does not mutate any of the class member variables, we can make it a const function:
double get_area () const {return 3.14*radius*radius;};
Installed cpplinter from Google and formatted my code accordingly. Added const throughout function arguments and function attributes. Split some big functions into separate functions. Passed arguments either by reference or pointers.
Description of work
Purpose of work
Second PR for the two cpp exercises.
Summary of work
Added 4 new files in total (following the instructions): two main.py files, one text input file (holmes.txt) and one text output file (for the word counts). The two main.cpp files were compiled using a simple g++ command, did not use CMake. Note that in the first exercise, if using CMake, the CMake source directory needs to be the same as the main.cpp file.
To test:
Nothing