Hw2 bor by AnNyiiik · Pull Request #3 · AnNyiiik/HWSecondTerm

AnNyiiik · 2023-03-09T22:06:23Z

No description provided.

viktoriia-fomina · 2023-03-11T21:27:39Z

+    }
+    public int Size()


Между методами нужна пустая строка

viktoriia-fomina · 2023-03-11T21:27:56Z

+    }
+    private bool AddToVertex(Vertex vertex, string element, int position)


Снова не хватает пустой строки между методами

viktoriia-fomina · 2023-03-11T21:30:10Z

+        var vertex = FindVertex(prefix);
+        if (FindVertex(prefix) != null)


В if стоит vertex передать, а не второй раз FindVertex(prefix) вычислять :)

viktoriia-fomina · 2023-03-11T21:33:03Z

+        return true;
+    }
+
+    public bool Add(string element)


В публичный метод могут передать что угодно. Стоит проверить, не null ли строка.
Ваш класс кто-то может решить использовать в своем проекте, например. Будет неприятно свалиться с NullReferenceExpression при передаче некорректного значения в метод класса :)

viktoriia-fomina · 2023-03-11T21:51:34Z

+        public Dictionary<char, Vertex> NextVertices;
+
+        public int NumberOfTerminalVertices;
+
+        public bool IsTerminal;


Если поле назвать с заглавной буквы от этого оно не станет свойством :) А публичные поля - зло.

Вам подойдут свойства со стандартным getter и setter:

public bool IsTerminal { get; set; }

viktoriia-fomina · 2023-03-11T22:04:04Z

+        return isAdded;
+    }
+
+    public bool Contains(string element)


Сюда тоже в качестве аргумента могут передать null. Нужна проверка на то, что переданный аргумент не null

А проверка так и не появилась... Теперь нужно еще добавить тесты, где вы в качестве аргументов в публичные методы Бора передаете null :)

viktoriia-fomina · 2023-03-11T22:10:42Z

+        return current;
+    }
+
+    private Tuple<bool, bool> RemoveFromVertex(Vertex vertex, string element, int position)


Не очень очевидно без копания в коде, что вообще вы возвращаете из этого метода. Во все неочевидные на ваш взгляд места добавляйте, пожалуйста, комментарии. Разработчик, который будет сопровождать написанный вами код, не будет вас поминать плохим словом в таком случае :)
Если кратко, то по названию метода RemoveFromVertex непонятно, что он делает. Может быть Remove что-то FromVertex? И еще сложно понять, что же метод возвращает. Полезны будут комментарии

viktoriia-fomina · 2023-03-11T22:23:56Z

+        if (vertex.NextVertices.ContainsKey(element[position]))
+        {
+            try
+            {
+                var isAdded = AddToVertex(vertex.NextVertices[element[position]], element, ++position);
+                if (isAdded)
+                {
+                    ++vertex.NumberOfTerminalVertices;
+                    return true;
+                }
+            }
+            catch
+            {
+                return false;
+            }


Блок try ... catch используется, когда вы ожидаете, что вызываемый код может выбросить исключение. Ваш метод никакие исключения не бросает.
Представьте, вы написали метод не очень корректно и на самом деле падаете периодически с NullReferenceExpression, но из-за блока try ... catch вы можете долго пытаться понять в чем проблема

Все еще не понимаю: зачем вам здесь try ... catch? :)

viktoriia-fomina · 2023-03-11T22:45:30Z

+    return;
+}
+
+Console.WriteLine("To add an element press 1\nTo remove press 2\nTo check if the particular word contains press 3\n" +


Можно:

Console.WriteLine(""" To add an element press 1 To remove press 2 To check if the particular word contains press 3 To count words with the same prefix press 4 To quit press 0 """);

viktoriia-fomina · 2023-03-11T22:48:38Z

+            var word = Console.ReadLine();
+            if (word == null)
+            {
+                Console.WriteLine("A null-reference");
+                return;
+            }
+            var isAdded = trie.Add(word);
+            if (isAdded)
+            {
+                Console.WriteLine("A new word has been just added");
+            }
+            else
+            {
+                Console.WriteLine("A word hasn't been added, it already exists");
+            }


А здесь пользователь может ввести Enter, в таком случае Console.ReadLine вернет пустую строку. А вы напишите: A word hasn't been added, it already exists. Что не является правдой. Поправьте, пожалуйста

viktoriia-fomina · 2023-03-19T08:03:19Z

+        if (vertex.NextVertices.ContainsKey(element[position]))
+        {
+            try
+            {
+                var isAdded = AddToVertex(vertex.NextVertices[element[position]], element, ++position);
+                if (isAdded)
+                {
+                    ++vertex.NumberOfTerminalVertices;
+                    return true;
+                }
+            }
+            catch
+            {
+                return false;
+            }


Все еще не понимаю: зачем вам здесь try ... catch? :)

viktoriia-fomina · 2023-03-19T08:06:36Z

+    ///<summary> Recursively checks if the given element exists in the tree. If it does, returns a pair of bool values: 1st - 
+    ///      true, 2nd - if there is no other elements after the current. If the second value is true, we can delete a
+    ///      branch. </summary>


У вас тут комментарий поехал, нужно:

/// <summary> /// Recursively checks if the given element exists in the tree. If it does, returns a pair of bool values: 1st - /// true, 2nd - if there is no other elements after the current. If the second value is true, we can delete a branch. /// </summary>

viktoriia-fomina · 2023-03-19T08:17:27Z

+    ///<summary> Recursively checks if the given element exists in the tree. If it does, returns a pair of bool values: 1st - 
+    ///      true, 2nd - if there is no other elements after the current. If the second value is true, we can delete a
+    ///      branch. </summary>
+    private Tuple<bool, bool> RemoveWordFromVertex(Vertex vertex, string element, int position)


Этот метод берет на себя слишком много: и удаляет слово, и еще какую-то дополнительную информацию возвращает, не относящуюся напрямую к операции удаления. В общем, он разве что кофе не варит :)
Метод все-таки должен идейно что-то одно делать: пускай он удаляет слово и возвращает bool - индикатор успешности проведенной операции.
Декомпозируйте, пожалуйста :)

viktoriia-fomina · 2023-03-21T18:27:53Z

+        return isAdded;
+    }
+
+    public bool Contains(string? element)


Из-за того, что вы позволяете в качестве аргумента в метод Contains передавать null, nullability-анализ не выдаст пользователю warning при попытке передачи null в качестве аргумента.
Это плохо, потому что в целом аргумент Contains не должен быть null, хорошо бы, чтобы nullability-анализ имел возможность пользователю об этом напомнить

viktoriia-fomina · 2023-03-21T18:30:35Z

+        {
+            Console.WriteLine("Not a number");
+            return;
+        }


Нужно все на 4 пробела влево сместить

viktoriia-fomina · 2023-03-21T18:48:05Z

+        public Dictionary<char, Vertex> NextVertices
+        {
+            get => this.nextVertices;
+        }


Стоит использовать auto-implemented properties

viktoriia-fomina · 2023-03-21T18:48:29Z

+        public int NumberOfTerminalVertices
+        {
+            get => numberOfTerminalVertices;
+            set => numberOfTerminalVertices = value;
+        }


И даже тут :)

viktoriia-fomina · 2023-03-21T18:50:58Z

+        if (prefix.Length == 0)
+        {
+            return _sizeOfTrie;
+        }


С чего бы? :) Пустая строка в Боре не содержится

Все еще пустая строка в Боре не содержится. Поправьте, пожалуйста

viktoriia-fomina · 2023-03-21T18:51:50Z

+        return isDeleted.Item1;
+    }
+
+    public int HowManyStartsWithPrefix(String prefix)


А тут вам null могут передать, нужна проверка на null

viktoriia-fomina · 2023-03-21T18:53:12Z

+        return new Tuple<bool, bool>(false, false);
+    }
+
+    public bool Remove(string? element)


Тут то же самое, что и в методе Contains, нужно string element вместо string? element

viktoriia-fomina · 2023-03-26T18:52:24Z

+        _sizeOfTrie = 0;
+        Size = _sizeOfTrie;


Так поле _sizeOfTrie на самом деле вам не нужно (кроме того, инициализировать поле нулем было не нужно, в .NET итак все объекты имеют значения по умолчанию, у int это как раз 0). А не нужно потому, что auto-implemented property можно создать так:
public int Size { get; private set; }.
С приватным set вы сможете поддерживать значение свойства Size в актуальном состоянии

viktoriia-fomina · 2023-03-26T18:57:14Z

+        return true;
+    }
+
+    public bool Add(string? element)


Из-за того, что вы позволяете в качестве аргумента в метод Add передавать null, nullability-анализ не выдаст пользователю warning при попытке передачи null в качестве аргумента.
Это плохо, потому что в целом аргумент Add не должен быть null, хорошо бы, чтобы nullability-анализ имел возможность пользователю об этом напомнить.

Кстати, уже была пара комментариев об этом :) Все-таки ожидается, что вы внимательно изучаете комментарии и исправляете свои ошибки

viktoriia-fomina · 2023-03-26T19:00:06Z

+        return isAdded;
+    }
+
+    public bool Contains(string element)


А проверка так и не появилась... Теперь нужно еще добавить тесты, где вы в качестве аргументов в публичные методы Бора передаете null :)

viktoriia-fomina · 2023-03-26T19:02:04Z

+        if (prefix.Length == 0)
+        {
+            return _sizeOfTrie;
+        }


Все еще пустая строка в Боре не содержится. Поправьте, пожалуйста

AnNyiiik added 5 commits February 20, 2023 08:36

changed

730c861

wrote methods Add & Remove

29875e7

added method Contains and HowManyStartsWithPrefix

bced85f

added tests and iput output

5abfb54

fixed

e85dc58

viktoriia-fomina reviewed Mar 11, 2023

View reviewed changes

correct mistakes

32dd44e

viktoriia-fomina reviewed Mar 21, 2023

View reviewed changes

corrected some mistakes

a938a06

viktoriia-fomina reviewed Mar 26, 2023

View reviewed changes

AnNyiiik added 2 commits March 28, 2023 00:21

fixed

1994989

added function DeleteBranch, changed RemoveFromVertex

af470ed

		}
		private bool AddToVertex(Vertex vertex, string element, int position)

		var vertex = FindVertex(prefix);
		if (FindVertex(prefix) != null)

Conversation

AnNyiiik commented Mar 9, 2023

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants