Routers #9
Routers #9
Conversation
Routers/GraphTests/GraphTests.cs
Outdated
| [Test] | ||
| public void DataFromFileShouldBeParsedCorrectly() | ||
| { | ||
| var inputFile = new StreamWriter(File.Open(inputFilePath, FileMode.Create)); |
There was a problem hiding this comment.
Правильнее using var. А ещё правильнее потом файл удалить, чтобы избежать зависимости тестов друг от друга.
| @@ -0,0 +1,9 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
Надо сложить всё в один проект, не плодите без нужды .dll-ки. Сборка суть единица развёртывания, а не единица логической декомпозиции (к сожалению). Используйте вместо этого неймспейсы.
Routers/Routers/Graph.cs
Outdated
|
|
||
| public int MaxNodeNumber { get; private set; } | ||
|
|
||
| public int[,] Matrix { get; private set; } |
There was a problem hiding this comment.
Даже если у неё приватный сеттер, враги могут залезть внутрь и её поменять — сеттер не даёт поменять ссылку на матрицу, но сама матрица — это вполне себе мутабельный объект, который может изменять каждый, кто имеет на него ссылку. Константность в C# неглубокая. Сделали бы лучше индексовое свойство с только геттером, или возвращали бы копию (что было бы плохо с точки зрения скорости работы).
Routers/Routers/Graph.cs
Outdated
|
|
||
| public int[,] Matrix { get; private set; } | ||
|
|
||
| public Dictionary<(int, int), int> Edges { get; set; } |
There was a problem hiding this comment.
То же со словарями. Тут вообще стрёмно, потому что Matrix и Edges, наверное, должны находиться в какой-то логической зависимости друг от друга, но класс не может это гарантировать.
|
|
||
| private void Parse(string inputFilePath) | ||
| { | ||
| using StreamReader reader = new StreamReader(inputFilePath); |
There was a problem hiding this comment.
| using StreamReader reader = new StreamReader(inputFilePath); | |
| using var reader = new StreamReader(inputFilePath); |
Это как заикание, "using StreamReader-StreamReader" :)
Routers/Routers/RoutersUtility.cs
Outdated
| } | ||
|
|
||
| graph.Matrix[firstNode, secondNode] = graph.Edges[key]; | ||
| graph.Matrix[secondNode, firstNode] = graph.Edges[key]; |
There was a problem hiding this comment.
Такое чувство, что Edges должно поддерживаться тут, а не в графе, а граф был бы тогда просто матрицей плюс парой методов для её удобной модификации и анализа
Routers/Routers/RoutersUtility.cs
Outdated
|
|
||
| int firstNode = key.Item1; | ||
| int secondNode = key.Item2; | ||
| graph.DeleteEdge(firstNode, secondNode); |
There was a problem hiding this comment.
Алгоритм Краскала наоборот — удаляем рёбра, пока граф связный? :) Хорошо, но требует гонять DFS на каждом шагу, что как-то не очень с точки зрения скорости работы (тем более что граф остаётся связным, то есть на каждом шагу надо посетить все вершины). Честный Краскал с СНМ работал бы заметно быстрее.
| using System.Runtime.Serialization; | ||
|
|
||
| [Serializable] | ||
| public class WrongInputException : Exception |
There was a problem hiding this comment.
Исключению тоже нужен комментарий
| Assert.AreEqual(outputFile.ReadLine(), "3: 5(17)"); | ||
| outputFile.Close(); | ||
|
|
||
| inputFile = new StreamWriter(File.Open(inputFilePath, FileMode.Create)); |
There was a problem hiding this comment.
Много тестовых сценариев => много тестов. И тоже надо бы собрать все тесты в один проект.
Routers/Routers/Program.cs
Outdated
|
|
||
| string inputFilePath = "../../../input.txt"; | ||
| string outputFilePath = "../../../output.txt"; | ||
| RoutersUtility.GenerateConfig(inputFilePath, outputFilePath); |
There was a problem hiding this comment.
А заказывали ещё ругань на несвязность графа, а не сделано. Да и пути в качестве параметров не сделаны. То есть в реальной жизни это провал приёмочных испытаний и доработка за счёт разработчика (плюс, возможно, неустойка), а сейчас минус балл :)
No description provided.