Mohammed almakki exercies#88
Conversation
|
|
||
| std::vector<std::string> tokenizeFile(const std::string &file) | ||
| { | ||
| std::regex re("[-\\s]"); |
There was a problem hiding this comment.
is the inent of this that the experssion match a hyphen of a whitespace, but a double backslash exits the special character.
There was a problem hiding this comment.
This regex is used to split the string by two delimiters, the space, and the hyphen. It will match a space or a hyphen. After that, the expression is used in a reverse way, to get all the words that don't match it i.e. the split words.
There was a problem hiding this comment.
A Hyphen is also a special character and will need to be exited as well
There was a problem hiding this comment.
Yes, it will be removed. For example, the string "lines are hard-coded" will be transformed into a vector of ["lines", "are", "hard", "coded"] by this expression.
| word.begin(), word.end(), word.begin(), | ||
| [](unsigned char c) | ||
| { return std::tolower(c); }); | ||
| word.erase(std::remove_if(word.begin(), word.end(), ispunct), word.end()); |
There was a problem hiding this comment.
The task specifies only needing to consider .,?'"!(): you might have considered using regex to strip these away and replacing them with a whitespace at the same time as doing the hyphen.
There was a problem hiding this comment.
As I understood, the comparison should be punctation insensitive. So for example, "what's" should be equal to "whats" but replacing it with whitespace will result in the word not being included (not longer than 4). So in this line, I just remove every punctuation mark.
There was a problem hiding this comment.
In that case I think you may be right
| word.erase(std::remove_if(word.begin(), word.end(), ispunct), word.end()); | ||
| if (word.length() > 4) | ||
| { | ||
| wordsCounts[word]++; |
There was a problem hiding this comment.
you should have a check to see if word is currently in the wordCounts map
There was a problem hiding this comment.
Hmmm, why is it needed?
If there is a get operation, then I need a check, but this is just used as a counter.
There was a problem hiding this comment.
Ultimatly it is not required for it to work, but it is better practice to have the check when performing an action on an object in a map where it is not certain that the object will exist.
| { | ||
| wordsCountsPairs.push_back(pair); | ||
| } | ||
| std::sort( |
MialLewis
left a comment
MialLewis
left a comment
There was a problem hiding this comment.
Good job - a few comments on the second exercise. Some of the comments apply in more than one place, but I've only pointed out the first occurrence.
| Shape(int noOfSides) : noOfSides{noOfSides} {} | ||
|
|
||
| private: | ||
| int noOfSides; |
There was a problem hiding this comment.
In this exercise, this could be const as there's no requirement to mutate the shapes after construction.
| int noOfSides; | ||
| }; | ||
|
|
||
| class Square : public Shape |
There was a problem hiding this comment.
I'd have been tempted to have square inherit from rectangle to avoid some code duplication. Override functions where necessary.
| return "Square"; | ||
| } | ||
|
|
||
| int getSidesCount() |
There was a problem hiding this comment.
Could we have this defined in the Shape parent class, simply returning its member noOfSides? We then would not have to redefine it in every child.
There was a problem hiding this comment.
Yeah, that is really bad. Doesn't know why I did it like this.
|
|
||
| void printShapesWithSides(const int &sidesCount) | ||
| { | ||
| for (auto shape : shapes) |
There was a problem hiding this comment.
Likely needless complication in this instance, but could be interesting to see if you could reduce code duplication (use of function pointers?)
There was a problem hiding this comment.
Yes, we can make a general function that prints shapes that meet a specific condition. The function will accept a lambda filter function as a parameter. The lambda function will accept a parameter of type shape and return a Boolean value. Also, we can get rid of the for loop and use for_each. But I think it is too complex for this small example.
| return left->getArea() > right->getArea(); | ||
| }); | ||
|
|
||
| for (auto shape : sortedShapes) |
There was a problem hiding this comment.
As per the above - try to refactor to reduce the code duplication here.
Change types to enum instead of raw strings
Reduce code duplication by abstracting a general printing function
MialLewis
left a comment
MialLewis
left a comment
There was a problem hiding this comment.
Thanks for the improvements - your solution using the lambda filter functions is really neat.
Thanks for your great feedback! |
3 participants
This PR contains the solution of c++ exercise 1 and 2