Смирнов Илья by IljarSmirnov · Pull Request #2 · ValentinV95/mp2-2023-lab5-tables

IljarSmirnov · 2024-04-29T06:55:39Z

No description provided.

IljarSmirnov · 2024-04-29T08:07:22Z

@KorotinEgor, @kermanglobe, хоть с CMake и проблемы, но сам код можете проверять

kermanglobe

если мне не изменяет память, то должна ещё быть упорядоченная таблица на массиве

kermanglobe · 2024-04-30T09:41:09Z

+	std::vector<Node*> memorysearch(std::string name);
+	int balance(Node* nd);
+	void smallleftrot(Node* xparent, Node* x);
+	void smallrightrot(Node* xparent, Node* x);
+	void bigleftrot(Node* xparent, Node* x);
+	void bigrightrot(Node* xparent, Node* x);


я бы эти методы в private поместил

kermanglobe · 2024-04-30T10:00:09Z

+	void clear(std::string name);
+	polynoms Arithmetic(std::string strexpr);


в чём резон помещать метод для вычисления выражений внутрь таблиц?

kermanglobe · 2024-04-30T10:14:35Z

+	~AVLTreeTable();
+	polynoms search(std::string name);


ключ-строку нужно передавать как const std::string& (иначе каждый раз при вызове функции будет происходить копирование)

kermanglobe · 2024-04-30T10:18:13Z

+		}
+		polynoms entered(entmon);
+		table.add(name, entered);
+		cout << "UnorderedTable: add. Operations: " << counttable << endl;
+		tree.add(name, entered);
+		cout << "AVLTreeTable: add. Operations: " << counttree << endl;
+		hashtable.add(name, entered);


если я правильно понял, то пользователь должен выбирать, в какую из таблиц он хочет положить полином

В задании написано "Работа происходит сразу с таблицами всех типов.", как я и сделал

kermanglobe · 2024-04-30T10:51:52Z

+file(GLOB hdrs "*.h*" "../include/*.h")
+file(GLOB srcs "*.cpp")


зачем убрал "../src/*.cpp" ?

Да что с ним, что без него - не работает =)

kermanglobe · 2024-04-30T11:11:18Z

+
+static int counttree = 0;


так counttree работать не будет, ты либо прописываешь эту переменную в хедере как extern int counttree и в исходниках пишешь int counttree, либо помещаешь внутрь класса
при текущей реализации программа-сэмпл выводит 0 операций при вставке, поиске и удалении (что, очевидно, не так)

kermanglobe · 2024-04-30T11:16:19Z

+	}
+	try {
+		search(name);
+	}
+	catch (std::logic_error& e) {
+		Node* cur = head;
+		std::vector<Node*> path = memorysearch(name);


интересная, конечно, схема с try/catch блоком...
если уж на то пошло, то в catch должно быть const std::exception& e

search выбрасывает logic_error, поэтому и ловится именно он

kermanglobe · 2024-04-30T11:17:52Z

+
+polynoms AVLTreeTable::search(std::string name) {


неплохо бы возвращать не собственно полином, а указатель на cur->data.second
можно будет как минимум сделать проще проверку на наличие элемента в таблице

kermanglobe · 2024-04-30T11:40:35Z

+		if (i >= 1) {
+			if (s[i] == 'x' && (s[i - 1] == '0' || s[i - 1] == '1' || s[i - 1] == '2' || s[i - 1] == '3' || s[i - 1] == '4' || s[i - 1] == '5' || s[i - 1] == '6' || s[i - 1] == '7' || s[i - 1] == '8' || s[i - 1] == '9')) {


Suggested change

if (i >= 1) {

if (s[i] == 'x' && (s[i - 1] == '0' || s[i - 1] == '1' || s[i - 1] == '2' || s[i - 1] == '3' || s[i - 1] == '4' || s[i - 1] == '5' || s[i - 1] == '6' || s[i - 1] == '7' || s[i - 1] == '8' || s[i - 1] == '9')) {

if (i >= 1) {

if (s[i] == 'x' && s[i - 1] >= '0' && s[i - 1] <= '9') {

так компактнее будет

kermanglobe · 2024-04-30T11:52:01Z

+}
+TEST(AVLTreeTable, can_clear) {
+	list<pair<int, double>>a;
+	a.push_back(make_pair(100, 1));
+	polynoms A(a);
+	AVLTreeTable T;
+	T.add("pol", A);
+	ASSERT_NO_THROW(T.clear("pol"));
+}
+TEST(AVLTreeTable, clear_is_correct) {
+	list<pair<int, double>>a;
+	a.push_back(make_pair(100, 1));
+	polynoms A(a);
+	AVLTreeTable T;
+	T.add("pol", A);
+	T.clear("pol");
+	ASSERT_ANY_THROW(T.search("pol"));
+}


можно добавить тесты на удаление с различными сценариями (удаление листа, вершины с одним или двумя потомками, головы с потомками или без потомков)

KorotinEgor

Методы с арифметикой я не проверял, скажем, что они правильные изначально, а если это не так, то виноват госопдин проверяющий.

KorotinEgor · 2024-04-30T23:23:43Z

+
+class UnorderedTable {
+	std::vector<std::pair<std::string, polynoms>> table;
+	size_t size;


А в чём вообще смысл этой переменной? Ты хранишь размер, чтобы, если что, выдать количество элементов пользователю за О(1), но с данной функцией прекрасно спровляется return table.size()

KorotinEgor · 2024-04-30T23:25:21Z

+		this->add(rec.first, rec.second);
+	}
+}
+void UnorderedTable::add(std::string name, polynoms pol) {


В некоторых методах деревьев вроде бы поправил, а тут - нет. Лучше передавать что строку, что полином по ссылке, чтобы не копировать одно и то же по 10 раз
В хэшах и тех методах деревьев, что не попали под "некоторые" аналогично

KorotinEgor · 2024-04-30T23:29:57Z

+	polynoms search(std::string name);
+	void add(std::string name, polynoms pol);
+	void clear(std::string name);
+	void resize();


А разве не логичнее будет засунуть resize в private? А так, это нарушение принципов ООП

KorotinEgor · 2024-04-30T23:34:46Z

+	int res, strsum = 0;
+	for (int i = 0; i < name.size(); i++) {
+		strsum += int(name[i]);
+	}
+	res = strsum % size;
+	return res;


А зачем res вообще существует?, то же можно сказать и про Hash2.

KorotinEgor · 2024-04-30T23:58:33Z

+		if (table[(Hash1(name) + i * Hash2(name)) % size] != nullptr) {
+			if (table[(Hash1(name) + i * Hash2(name)) % size]->state == 0 || table[(Hash1(name) + i * Hash2(name)) % size]->state == -1) {
+				table[(Hash1(name) + i * Hash2(name)) % size] = new Rec(name, pol);
+				return;


Утечка памяти, если элемент явяется удалённым ты просто присваиваешь в Table[...] новый указатель, в то время как старый элемент, на который там раньше был указатель остаётся где-то непонятно где, но точно не удаляется.

KorotinEgor · 2024-05-01T00:36:02Z

+		while (true) {
+			counttree++;
+			if (name < cur->data.first) {
+				if (cur->left) {
+					cur->h += 1;
+					cur = cur->left;
+				}
+				else {
+					break;
+				}
+			}
+			else {
+				if (cur->right) {
+					cur->h += 1;
+					cur = cur->right;
+				}
+				else {
+					break;
+				}
+			}
+		}


Не понял смысл этого действия, разве нельзя просто пройтись по path и всем там h++ сделать?

KorotinEgor · 2024-05-01T00:44:00Z

+			else {
+
+				if (balance(path[i]) == 2) {
+					if (balance(path[i]->left) >= 0) {
+						if (i == 0) smallrightrot(nullptr, path[i]);
+						else smallrightrot(path[i - 1], path[i]);
+					}
+					else if (balance(path[i]->left) < 0) {
+						if (i == 0) bigrightrot(nullptr, path[i]);
+						else bigrightrot(path[i - 1], path[i]);
+					}
+				}
+				if (balance(path[i]) == -2) {
+					if (balance(path[i]->right) <= 0) {
+						if (i == 0) smallleftrot(nullptr, path[i]);
+						else smallleftrot(path[i - 1], path[i]);
+					}
+					else if (balance(path[i]->right) > 0) {
+						if (i == 0) bigleftrot(nullptr, path[i]);
+						else bigleftrot(path[i - 1], path[i]);
+					}
+				}
+				seth(path[i]);
+


Эта часть бесполезна, как и весь этот огромный if, его единтсенное теоретичсекое предназначение - чтоб не залесть, куда не надо, но в твоём случае, это опбеспечивает метод balance (в него уже встроена проверка на то, являются ли 1 или 2 потомка нулевыми или нет)

KorotinEgor · 2024-05-01T01:43:29Z

+	void smallleftrot(Node* xparent, Node* x);
+	void smallrightrot(Node* xparent, Node* x);
+	void bigleftrot(Node* xparent, Node* x);
+	void bigrightrot(Node* xparent, Node* x);


повороты должны быть в private, тк они могут изменить дерево так, что оно перестанет быть AVL-деревом или, могут уронить программу(при попытке прочитать не принадлежащую им память), чего ты, как разработчик классов на C++, допускать не должен.

KorotinEgor · 2024-05-01T01:45:45Z

+				else if (minval->left != nullptr && minval->right == nullptr) {
+					path[path.size() - 2]->right == minval->left;
+					counttree++;
+				}


опять бесполезная часть, после цикла while у cur никогда нет потомков слева, т.е. minnval->left=nullptr всегда

KorotinEgor · 2024-05-01T01:49:32Z

+			else {
+				counttree++;
+				if (path[path.size() - 2]->left == cur) {
+					path[path.size() - 2]->left = cur->left;
+				}
+				else {
+					path[path.size() - 2]->right = cur->left;
+				}
+			}


Не рассмтариваешь случай, когда у тебя дерево это голова с 1м левым потомком и ты хочешь удалить голову. Тогда path.size()=1, path[path.size() - 1]->h!=0, cur->right==nullptr, ты попадаешь в это место и деалешь path[path.size() - 2]

Кроме того, если ты попал сюда, то ты даже не удаляешь cur, а просто исключешь его из дерева, т.е. ещё одна утечка памяти

IljarSmirnov · 2024-05-01T20:05:59Z

@ValentinV95, Здравствуйте, код готов к проверке

ValentinV95

Лабораторная принята, часть справидливых замечаний от одногруппников осталась не исправленной и не прокомментирована

add lab

9fd24d2

IljarSmirnov changed the title ~~add lab~~ Смирнов Илья Apr 29, 2024

IljarSmirnov added 4 commits April 29, 2024 12:49

test

3f6effe

update

e7c51fc

update

06c02eb

test

e569e8c

kermanglobe reviewed Apr 30, 2024

View reviewed changes

IljarSmirnov added 2 commits April 30, 2024 15:23

update

e49af10

update

3c3dbf7

KorotinEgor reviewed May 1, 2024

View reviewed changes

update

a7dd392

ValentinV95 reviewed May 20, 2024

View reviewed changes

ValentinV95 added the good first issue Good for newcomers label May 20, 2024

		void clear(std::string name);
		polynoms Arithmetic(std::string strexpr);

		file(GLOB hdrs ".h" "../include/*.h")
		file(GLOB srcs "*.cpp")

		if (i >= 1) {
		if (s[i] == 'x' && (s[i - 1] == '0' \|\| s[i - 1] == '1' \|\| s[i - 1] == '2' \|\| s[i - 1] == '3' \|\| s[i - 1] == '4' \|\| s[i - 1] == '5' \|\| s[i - 1] == '6' \|\| s[i - 1] == '7' \|\| s[i - 1] == '8' \|\| s[i - 1] == '9')) {

Conversation

IljarSmirnov commented Apr 29, 2024

Uh oh!

IljarSmirnov commented Apr 29, 2024

Uh oh!

kermanglobe 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!

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!

KorotinEgor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KorotinEgor Apr 30, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KorotinEgor May 1, 2024 • 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!

KorotinEgor May 1, 2024 • 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!

KorotinEgor May 1, 2024 • 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!

IljarSmirnov commented May 1, 2024

Uh oh!

ValentinV95 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KorotinEgor Apr 30, 2024 •

edited

Loading

KorotinEgor May 1, 2024 •

edited

Loading

KorotinEgor May 1, 2024 •

edited

Loading

KorotinEgor May 1, 2024 •

edited

Loading