Interactive mode for working with the parse tree #41
Conversation
Changing the function for tree output
a flaw has been changed (0 is a valid number)
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
Название ветки и пулреквеста ужасно :) И программа не объясняет себя, а просто работает, так нельзя. Писать надо не для себя, а для людей, которые это читать будут.
| struct Node* parent; | ||
| char value; | ||
| int number; | ||
| char help; |
There was a problem hiding this comment.
Не называйте переменные "help", это мило, но не мнемонично.
| return root->number; | ||
| } | ||
|
|
||
| bool compare(char symbol) |
There was a problem hiding this comment.
Тоже название не очень. compare чего с чем? Тем более тут проверяется, является ли символ оператором, так что я бы isOperator это назвал
| || symbol == '*' || symbol == '/'; | ||
| } | ||
|
|
||
| Node* buildTree(char* array) |
There was a problem hiding this comment.
const char* array
Чтобы явно сказать всем, что мы не собираемся менять элементы массива, и разрешить, в частности, принимать строковые литералы сюда, что сделает работу с этой функцией в сотню раз более удобной
| Node* newRoot = (Node*)calloc(1, sizeof(Node)); | ||
| if (newRoot == NULL) | ||
| { | ||
| return NULL; |
There was a problem hiding this comment.
Так всё построенное до этого дерево потеряется
| char value; | ||
| int number; |
There was a problem hiding this comment.
Ну тоже переменные названы замечательно, непонятно, кто из них кто.
| { | ||
| Node* firstTree = createTree(); | ||
| firstTree = buildTree("- * + 4 3 5 * 2 7"); | ||
| firstTree = returnHead(firstTree); |
There was a problem hiding this comment.
Разве не buildTree должна была бы это делать?
| Node* rightSon(Node* root); | ||
|
|
||
| // Function for accessing the left son of the current root | ||
| Node* leftSon(Node* root); | ||
|
|
||
| // Function for accessing the parent of the current root | ||
| Node* parent(Node* root); |
There was a problem hiding this comment.
Не слишком ли много дерево раскрывает о себе? Сокрытие деталей реализации тут не очень, и принцип минимальности интерфейса абстракции тоже страдает
| } | ||
| char expression[100] = { '\0' }; | ||
| return buildTree(fgets(expression, 100, file)); | ||
| fclose(file); |
| #include "../../ParsingTree/ParsingTree/ParsingTree.h" | ||
| #include <stdio.h> | ||
|
|
||
| Node* readExpression(Node* tree, const char* fileName, int* error) |
There was a problem hiding this comment.
tree не используется, что намекает на утечку памяти
| tree = returnHead(tree); | ||
| findAnswer(tree); | ||
| const int answer = returnAnswer(tree); | ||
| printf("Meaning of the expression : %d\n", answer); |
There was a problem hiding this comment.
"meaning" --- это "значение" в смысле "смысл". Тут правильно "value" или "result" или что-то такое.
changing functions to add an element, changing tests
MinyazevR
changed the title
MinyazevR
changed the title
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
Утечки памяти тоже надо поправить
There was a problem hiding this comment.
На самом деле, был ли узел посещён — это состояние обхода, а не свойство узла, потому что если мы два обхода параллельно запустим, всё сломается. Да даже если два последовательно, сбросится ли статус isVisitedNode? Но если хотите так, то можно не править.
There was a problem hiding this comment.
У дерева нет головы, у него есть корень. Но если бы мы назвали эту функцию returnRoot, было бы не очень, потому что параметр у неё называется root. Это плохо.
There was a problem hiding this comment.
Вот, например, тут — это же не корень, это узел
There was a problem hiding this comment.
Эта строка — самое нелогичное, что я видел сегодня. Если оператор, то узел не посещён, иначе посещён, что?
There was a problem hiding this comment.
Тут можно уже без else, выше ветка на return заканчивается. Это сэкономит уровень отступа.
There was a problem hiding this comment.
Очень длинная строчка, надо перенести на несколько
There was a problem hiding this comment.
Лучше было выводить в строку, потому что иначе эту функцию не протестировать, но ладно, пусть так
There was a problem hiding this comment.
Тесты проверяют не тот формат записи, что просили в условии, то есть не доказывают, что программа работает как надо :)
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
Почти всё ок, но надо поддержать целые числа
There was a problem hiding this comment.
Так "целые числа в качестве аргументов" не получится поддержать. Например, "-15".
There was a problem hiding this comment.
Да ну, проще было "если оператор, распечатать скобку, знак, вызваться рекурсивно от левого и правого сына, распечатать закрывающую скобку. Если операнд, просто число распечатать".
There was a problem hiding this comment.
Тоже не уверен на самом деле, что это надо хранить в дереве. Всё равно ведь пересчитываете каждый раз. return findAnswer(root->leftSon, error); тоже было бы ок.
2 participants
No description provided.