Conversation
| public class TrieTests | ||
| { | ||
| [Test] | ||
| public void Initialization_Create256Nodes() |
There was a problem hiding this comment.
Одного теста недостаточно. В идеале, должно быть по несколько тестов на каждую функцию: на обычное поведение и на граничные случаи.
| { | ||
| public static class LZW | ||
| { | ||
| public static int[] Compress(byte[] input) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
LZWAlgorithm/LZWAlgorithm/LZW.cs
Outdated
| { | ||
| var trie = new Trie(); | ||
| var codes = new List<int>(); | ||
| trie.Reset(); |
There was a problem hiding this comment.
Зачем вызывать Reset сразу после инициализации? Нельзя всё необходимое сделать в конструкторе?
LZWAlgorithm/LZWAlgorithm/LZW.cs
Outdated
| } | ||
| } | ||
|
|
||
| if (trie.GetCurrentCode() != -1) |
There was a problem hiding this comment.
Если GetCurrentCode -- дорогая операция, нужно сохранить её результат во временную переменную, чтобы не вызывать дважды. А если дешёвая, то GetCurrentCode должен быть свойством.
LZWAlgorithm/LZWAlgorithm/LZW.cs
Outdated
|
|
||
| foreach (byte b in input) | ||
| { | ||
| if (!trie.TryGetNextNode(b, out int code)) |
There was a problem hiding this comment.
TryGetNextNode что-то кладёт в out int code только когда возвращает false? Странное решение. Название функции и аргументы очень похожи на TryGet у Dictionary, но он делает наоборот -- out-параметр действителен только если TryGet вернул true. И не только TryGet так делает -- это довольно ожидаемое поведение для всех функций, которые начинаются с Try.
LZWAlgorithm/LZWAlgorithm/LZW.cs
Outdated
| return Array.Empty<byte>(); | ||
| } | ||
|
|
||
| var dictionary = new Dictionary<int, List<byte>>(); |
There was a problem hiding this comment.
Ключи этого словаря -- int, заполняются все ключи сразу, других не добавляется, размер известен заранее. Почему бы не использовать массив?
И не называйте, пожалуйста, переменную dictionary. Думаю, о ней известно больше, чем просто "это словарь". В конце концов, то, что это Dictionary видно из объявления.
LZWAlgorithm/LZWAlgorithm/Program.cs
Outdated
| break; | ||
| } | ||
|
|
||
| string decompressedFile = filePath[..^".zipped".Length] + "chgd"; |
There was a problem hiding this comment.
С помощью System.Path это можно сделать проще, ну или хотя бы понятнее.
LZWAlgorithm/LZWAlgorithm/Program.cs
Outdated
| int[] codes = LZW.Compress(inputBytes); | ||
|
|
||
| using FileStream fs = new (outputPath, FileMode.Create); | ||
| byte[] buffer = new byte[codes.Length * 4]; |
LZWAlgorithm/LZWAlgorithm/Program.cs
Outdated
|
|
||
| byte[] outputBytes = LZW.Decompress(codes); | ||
| File.WriteAllBytes(outputPath, outputBytes); | ||
| } No newline at end of file |
There was a problem hiding this comment.
nit: Файл должен заканчиваться символом новой строки.
LZWAlgorithm/LZWAlgorithm/Program.cs
Outdated
| FileInfo original = new (inputPath); | ||
| FileInfo compressed = new (outputPath); | ||
| double ratio = (double)original.Length / compressed.Length; | ||
| Console.WriteLine($"Коэффициент сжатия: {ratio:F2}"); |
There was a problem hiding this comment.
По-моему здесь неправильно посчитан коэффициент сжатия:
P.S. Это очень маленький файл, на нём мог получиться любой коэффициент, и это не страшно. Но точно не бесконечность.
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(result, Is.True); | ||
| Assert.That(code, Is.EqualTo(-1)); | ||
| Assert.That(trie.CurrentCode, Is.EqualTo(existingByte)); | ||
| }); |
There was a problem hiding this comment.
Не думаю, что здесь сильно нужен Multiple, но дело ваше. Только лучше использовать новый синтаксис.
| using FileStream fs = new (outputPath, FileMode.Create); | ||
| byte[] buffer = new byte[codes.Length * sizeof(int)]; | ||
| Buffer.BlockCopy(codes, 0, buffer, 0, buffer.Length); | ||
| fs.Write(buffer, 0, buffer.Length); |
There was a problem hiding this comment.
nit: При таком использовании using этот FileStream будет жить до конца метода, хотя он нужен только ближайшие 4 строчки. Конкретно для FileStream это не очень страшно, но лучше использовать using с фигурными скобками. И открывать файл раньше времени тоже смысла мало.
| /// <summary> | ||
| /// Resets the current node pointer to the root node. | ||
| /// </summary> | ||
| public void Reset() => this.current = this.root; |
There was a problem hiding this comment.
Я правильно понимаю, что Reset теперь нигде не вызывается?
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the number of children nodes in the root (always 256 after initialization). |
There was a problem hiding this comment.
Если это свойство всегда 256 после инициализации, а до инициализации оно вызвано быть не может, так ли оно нужно?
2 participants
No description provided.