Prolly needs refactoring #30
Conversation
ssancho2
left a comment
ssancho2
left a comment
There was a problem hiding this comment.
Looks Good, several style/readability comments, and two main ones marked with '(*)'
|
|
||
| class Dictionary { | ||
| public: | ||
| Dictionary(); |
There was a problem hiding this comment.
Please try to keep indentation consistent.
| class Dictionary { | ||
| public: | ||
| Dictionary(); | ||
| Dictionary(std::string); |
There was a problem hiding this comment.
Even if nod needed by the compiler, putting names for the parameters helps readers understand what parameters are.
| void Fill(std::string); | ||
| bool isPrefix(const std::string&); | ||
|
|
||
| std::unordered_set<std::string> words; |
There was a problem hiding this comment.
Those probably don't need to be public.
| if (file.is_open()) { | ||
| while (std::getline(file, line)) { | ||
| // Add to word dictionary | ||
| this->words.insert(line); |
|
|
||
| Dictionary::Dictionary() {}; | ||
| Dictionary::Dictionary(std::string path): | ||
| path(path) |
There was a problem hiding this comment.
It helps readability having different naming conventions for fields and parameters. On the Google Style for example, we end fields with '_'.
| row = i / rows; | ||
| for ( int j = 0; j < cols; j++) { | ||
| col = j % cols; | ||
| board.push_back(cell(row, col)); |
There was a problem hiding this comment.
(*)
I think here row is 0 and col is j.
You can use i and j directly, right?
| row = sq.row + row_shift; | ||
| col = sq.col + col_shift; | ||
| if (row >= 0 & row < rows & col >= 0 & col < cols & !(row_shift == 0 & col_shift == 0)) { | ||
| sq.neighbours.push_back(row * rows + col); |
There was a problem hiding this comment.
(*)
row * rows + col should be either:
row + rows * col or row * cols + col
One way to think about it is (for the first one):
When col = 0, this goes from 0 -> rows -1, so the next available number when col = 1 is rows.
In that case, if n = row + rows * col
You can extract them back (using that row < rows, and they are both positive integers):
n / rows -> (row + rows * col) / rows -> row / rows + (rows * col) / rows -> 0 + col -> col
n % rows -> (row + rows * col) % rows -> row % rows + (rows * col) % rows -> row + 0 -> row
| for (int col_shift : shift) { | ||
| row = sq.row + row_shift; | ||
| col = sq.col + col_shift; | ||
| if (row >= 0 & row < rows & col >= 0 & col < cols & !(row_shift == 0 & col_shift == 0)) { |
There was a problem hiding this comment.
it works, but you probably should use logical and && instead of bitwise and &
| visited.insert(position); | ||
|
|
||
| // If the string is a word, add it to the found words | ||
| if (dict.words.find(string) != dict.words.end()) { |
There was a problem hiding this comment.
This could be a function isWord() in the dictionary class, and the code bellow (dict.prefixes.find(string) != dict.prefixes.end()) ==> isPrefix()
| private: | ||
| struct cell { | ||
| std::string value; | ||
| int row; |
There was a problem hiding this comment.
It's ok to store them, but they are technically not needed.
As I explain in more detail in a comment above, you can from int index to coordinates and back 'easily'
Using a int to refer to the index of a cell in the board vector is ok, but a few comments will help :)
No description provided.