MayssaraOmar-3 (add assignment 3 solution) #19

MayssaraOmar · 2018-06-19T14:01:00Z

This is the first time I use unique pointers instead of raw pointers to build a data structure. I want to know if this is the best way to use them, what are the privileges of using smart pointers in general and when should I use smart pointers over raw pointers and vice versa, whether in building data structures or in general.

flerdacodeu · 2018-06-23T22:53:46Z

Really sorry for the delay on this! I had messed up with my comments.

In terms of the use of smart points to manage your data structure, I think you did a great job.

The main advantage of smart pointers (in my opinion) is that you avoid accidentally forgetting to delete things.

In this specific case, you could simply ignore the destructor (which could have ended up as a complex piece of code traversing the data structure) and your remove methods are a bit simpler to.

The advantages are usually more evident where you have large data structures that you might need to clean up in non-trivial ways, if there are various ways to modify them, or in case of exceptional behavior. The last one, exceptional behavior, is something you did not have here, so let me give you more details.

Let's assume that you need to allocate an object for your function: you might end up storing the object by the end of the function, but you might also return early or throw an exception; if you do have this behavior (something that happens only on special cases), then you need to free allocated object in these special code paths, but it is really easy to forget to do so.

bool findWords(const Grid& grid, vector<string>* words) {
  Dictionary* dic = new Dictionary();
  if (!loadDictionary(dic)) {
    return false;
  }
  return findWordsInDirectionary(grid, *dic, words);
}

Now, if you cannot load the dictionary object, you will leak that object, as you forgot to call delete. The same code with a smart pointer will work correctly by deleting the object. This is not a perfect example (you could have had the dictionary instead of a pointer to id), but in some cases it is not always possible to do so.

Essentially smart pointers are a way to simplify your code, as the smart pointer takes care of many of the things that are needed for you.

flerdacodeu · 2018-06-23T22:13:54Z

MayssaraOmar/assignment3/Word search/.cproject

@@ -0,0 +1,99 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>


It seems you accidentally added a number of files that are configuration files for your IDE.
I would suggest you delete those.

flerdacodeu · 2018-06-23T22:14:53Z

MayssaraOmar/assignment3/Word search/src/Word search.cpp

+using namespace std;
+
+void get_words_from_grid(vector<vector<char> > &grid, int row, int col,
+		string word, map<pair<int, int>, bool> &vis,


Optional: I would avoid shortening variable names like "visited" to "vis", because it makes the code harder to read.

flerdacodeu · 2018-06-23T22:15:34Z

MayssaraOmar/assignment3/Word search/src/Word search.cpp

+
+void get_words_from_grid(vector<vector<char> > &grid, int row, int col,
+		string word, map<pair<int, int>, bool> &vis,
+		map<pair<pair<int, int>, string>, bool> &memo, dictionary &dic,


"memo" is not very clear in this context: what is this supposed to represent?
You can add this as a comment to the function.

For example the following grid with the word "match" in dictionary:
a a r
t m d
c h x
We can obtain the word "match" by 2 ways:
1- by going from 'm' to 'a' upwards and then 't', 'c', 'h'
2- by going from 'm' to 'a' diagonally and then 't', 'c', 'h'
Here the ('t', 'c', 'h') is the repeated work as we call the function with the same input (row = 1, col = 0, word = "ca") twice.
We avoid repeating work by marking (1,0,"ca") true, in the second call, (1,0,"ca") will be true and the function will return.

flerdacodeu · 2018-06-23T22:16:15Z

MayssaraOmar/assignment3/Word search/src/Word search.cpp

+void get_words_from_grid(vector<vector<char> > &grid, int row, int col,
+		string word, map<pair<int, int>, bool> &vis,
+		map<pair<pair<int, int>, string>, bool> &memo, dictionary &dic,
+		unordered_set<string> &words) {


Style: A common convention is to make output parameters (parameters that are outputs or modified by a function) pointers rather than references.

flerdacodeu · 2018-06-23T22:17:46Z

MayssaraOmar/assignment3/Word search/src/Word search.cpp

+		return;
+	}
+
+	if (dic.isWord(word + grid[row][col])) {


This depends on exactly how isPrefix() works, but it is possible it would not return true for a completed word that is a prefix of another word. In that case, I would suggest checked isWord() first.

flerdacodeu · 2018-06-23T22:33:58Z

MayssaraOmar/assignment3/Word search/src/main.cpp

+
+#include <iostream>
+#include "test_dictionary.h"
+#include "test_wordSearch.h"


Optional: it is often the case that tests are put in the same file as the main function that invoked them.

flerdacodeu · 2018-06-23T22:36:03Z

MayssaraOmar/assignment3/Word search/src/test_dictionary.cpp

+	empty_dictionary();
+}
+void test_dictionary::insert_oneword() {
+	dic.insert("hello");


It is often the case that in your test you want to check that things behaved correctly.
For example, here you could say:

dic.insert("hello"); EXPECT_TRUE(dic.isWord("hello")); EXPECT_TRUE(dic.isPrefix("h")); EXPECT_TRUE(dic.isPrefix("he")); EXPECT_TRUE(dic.isPrefix("hel")); EXPECT_TRUE(dic.isPrefix("hell")); EXPECT_TRUE(dic.isPrefix("hello"));

Otherwise your test runs the code but it does not check that things worked.
This shows how it is not always possible to test functions completely in isolation.

flerdacodeu · 2018-06-23T22:38:41Z

MayssaraOmar/assignment3/Word search/src/test_dictionary.cpp

+
+dictionary test_dictionary::dic;
+
+void test_dictionary::run_all_tests() {


One problem with the current test setup is that all tests share the same dictionary, which means the order you run the tests has an effect on the result.
An alternative would be to make the test class have non-static methods and for each test you create an instance.

It would then look something like:

void test_dictionary::run_all_tests() { new test_directionary().insert_oneword(); new test_directionary().insert_vector(); ... }

flerdacodeu · 2018-06-23T22:39:59Z

MayssaraOmar/assignment3/Word search/src/test_dictionary.h

+
+#include "dictionary.h"
+
+class test_dictionary {


Style: Generally classes are named in CamelCase, so I would suggest DictionaryTests for this class.

flerdacodeu · 2018-06-23T22:42:48Z

MayssaraOmar/assignment3/Word search/src/test_wordSearch.cpp

+	dic.insert("cat");
+	dic.insert("m");
+}
+void test_wordSearch::run_all_tests() {


Similarly to above, it would make sense to run all the tests on a different instance to avoid interactions between them.

Since you have a setup method you could:

call setup() in the constructor

call setup() explicitly for each test

For the latter on, you could use:

test_wordSearch& test_WordSearch::setup() { ... return *this; } ... void test_wordSearch::run_all_tests() { new test_wordSearch().setup().empty_grid(); new test_wordSearch().setup().one_cell_grid_with_match_in_dictionary(); ... }

Does this make sense?

This is what I understood:

1- If I call setup() in the constructor, then all tests still share the same dictionary and there are interactions between tests(unless I'm making a new instance for each test).

2- I don't need to make a different instance for each test if each test is completely independent (has its own dictionary), for example calling setup(dictionary*) explicitly for each test with its own dictionary.

I think method 2 is simpler as I don't have the responsibility of freeing the memory I allocated for each test.

ssancho2

Nice! You used unique_ptr ! One more thing to add to what Flavio said: unique_ptr has a strong semantic connotation. If you see a raw pointer, who knows when (or even if) it will be deleted, who is responsible for doing that, who owns the object? The unique_ptr makes all that explicit.

ssancho2 · 2018-06-26T16:30:28Z

MayssaraOmar/assignment3/Word search/src/WordSearchTest.cpp

+	dictionary dic;
+	setup(&dic);
+	// build grid
+	vector<vector<char> > grid(3);


This could be done like:
vector<vector<char> > grid = { {'a', 'a', 'r'}, {'t', 'c', 'D'}} ;

My code doesn't compile with this kind of vector initialization :/ I'll leave it as it is for now and I'll look into the problem

ssancho2 · 2018-06-26T16:41:09Z

MayssaraOmar/assignment3/Word search/src/Word search.cpp

+static void get_words_from_grid(const vector<vector<char> > &grid, int row, int col,
+		string prefix, map<pair<int, int>, bool>* visited,
+		map<pair<pair<int, int>, string>, bool>* cache, const dictionary &dic,
+		unordered_set<string>* words) {


Some comments on the function / parameters:
I like the cache, avoiding checking twice the same thing, but why do you think is important? How likely it's that you will hit it?
I think the visited parameter will be slightly better (more performant) as a vector<vector>

@ssancho2 I think it depends on the type of inputs, if there is a lot of duplicates nearby in the grid, then the cache might be useful to optimize time. However, in all other types of input, it reserves unnecessary memory.

ssancho2 · 2018-06-26T16:41:35Z

MayssaraOmar/assignment3/Word search/src/Word search.cpp

 		for (int j = -1; j <= 1; j++) {
-			if (!vis[make_pair(row + i, col + j)]) {
-				vis[make_pair(row + i, col + j)] = true;
+			if (!(*visited)[make_pair(row + i, col + j)]) {


Maybe you can keep make_pair(row + i, col + j) in a variable.

ssancho2 · 2018-06-26T16:43:17Z

MayssaraOmar/assignment3/Word search/src/Word search.cpp

-		words.insert(word + grid[row][col]);
+	// word exits in dictionary
+	if (dic.isWord(prefix + grid[row][col])) {
+			words->insert(prefix + grid[row][col]);


You are doing prefix + grid[row][col] in several places, may be easier to do prefix += grid[row][col]; before this if

add assignment 3 solution

9814351

flerdacodeu self-requested a review June 21, 2018 12:56

flerdacodeu requested changes Jun 23, 2018

View reviewed changes

update solution

ff93980

ssancho2 approved these changes Jun 26, 2018

View reviewed changes

flerdacodeu approved these changes Jun 27, 2018

View reviewed changes

minor changes

18ee828

		@@ -0,0 +1,99 @@
		<?xml version="1.0" encoding="UTF-8" standalone="no"?>


		dictionary test_dictionary::dic;

		void test_dictionary::run_all_tests() {

MayssaraOmar-3 (add assignment 3 solution) #19

Are you sure you want to change the base?

MayssaraOmar-3 (add assignment 3 solution) #19

Uh oh!

Conversation

MayssaraOmar commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flerdacodeu commented Jun 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssancho2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MayssaraOmar Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MayssaraOmar commented Jun 19, 2018 •

edited

Loading

MayssaraOmar Jul 2, 2018 •

edited

Loading