Conversation
SquirrelLeonid
left a comment
SquirrelLeonid
left a comment
There was a problem hiding this comment.
В целом все гуд, но оставил несколько комментариев для правок
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
Тут есть неиспользуемые зависимости
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
И тут тоже неиспользуемые зависимости
| { | ||
| var result = personConfig.PrintToString(testPerson); | ||
|
|
||
| Assert.That(result, Contains.Substring("Person")); |
There was a problem hiding this comment.
Здесь оставлю комментарий, но он относится ко всем вызовам Contains.Substring в рамках тестов.
При таком сравнении строк есть проблема: если PrintingConfig начнет возвращать что-то вроде "Person abracadabra Name = John Doe", то тесты не отловят это изменение.
При таких сценариях я обычно предпочитаю формировать эталонную строку и использовать ее для сравнения.
Подозреваю, что такой способ ты выбрал, чтобы обработать случаи с разными признаками конца строки (Enviroment.NewLine).
Тут можно поступить так - эталонную строку сформировать в какой-то одной нотации (пусть, например, новая строка это \r\n), а перед сравнением сделать подмену этих символов на Environment.NewLine.
Так ты однозначно зафиксируешь формат, который ожидается на выходе
| var result = nodeConfig.PrintToString(node1); | ||
|
|
||
| Assert.That(result, Contains.Substring("Cyclic reference detected")); | ||
| Assert.DoesNotThrow(() => nodeConfig.PrintToString(node1)); |
There was a problem hiding this comment.
Я бы сказал, что эту проверку можно вообще опустить. Действие выше уже предполагает, что код не должен падать.
Либо эта проверка должна идти до получения result
| } | ||
|
|
||
| [TestFixture] | ||
| public class TypeExcludeTests |
There was a problem hiding this comment.
Комментарий относится к этому классу и другим подклассам с тестами.
Наличие нескольких классов в одном файле усложняет навигацию (в обозревателе будет один ObjectPrinterAcceptanceTests). Иногда такая структура бывает уместна, например в сценариях, когда у класса есть какое-то внутреннее представление, которое не несет пользы для внешнего кода.
Кроме того, хорошо просматривается дублирование кода в телах методов Setup. Это дело можно вынести в общий класс, через наследование
| return "null" + Environment.NewLine; | ||
|
|
||
| var finalTypes = new[] | ||
| // �������� �� ����������� ������ |
There was a problem hiding this comment.
Тут что-то с кодировкой у тебя случилось. Стоит поправить и использовать одну и ту же во всех файлах
| return finalTypes.Contains(obj.GetType()); | ||
| } | ||
|
|
||
| private string HandleFinalType(object obj) |
There was a problem hiding this comment.
Стоит переименовать obj в finalTypeObj. Это даст еще больше ясности в код
|
|
||
| private string HandleFinalType(object obj) | ||
| { | ||
| var type = obj.GetType(); |
There was a problem hiding this comment.
Ты в трех местах используешь определение типа через рефлексию (в рантайме).
При этом сценарий позволяет тебе единожды запросить тип объекта и передавать его в нужные методы.
Так стоит сделать, потому что определение типа в рантайме может дорого стоить.
Есть альтернативный способ через оператор typeof. Он определяет типы на этапе компиляции, но здесь, если не ошибаюсь, его не получится применить в полной мере (хотя в определенных местах можно и заиспользовать)
| private string HandleCollection(IEnumerable collection, int nestingLevel) | ||
| { | ||
| var sb = new StringBuilder(); | ||
| var identation = new string('\t', nestingLevel + 1); |
| } | ||
| else | ||
| { | ||
| sb.AppendLine("Collection"); |
There was a problem hiding this comment.
Не хочешь попобовать уточнить Collection до List или Array. Может даже до List<ИмяТипа> и Array[ИмяТипа].
В целом, наверное, нам не важно какая именно из двух эта коллекция. Но в тоже время мы и словарь можем рассматривать как коллекцию из пар ключ-значение, но все таки различаем его как отдельный тип.
|
Ага, стало лучше, но отмечу еще два момента.
Это рутинная работа, но такие проблемы не должны появляться в коде. Хорошо, что IDE может определить подходящую кодировку, но в худшем случае файл может стать нечитаемым, если знание о кодировке утеряно. Второй. Тесты стали чище, после того как ты унес SetUp и TearDown в базовый класс. Гуд. Но у меня все еще вызывает сомнения использование подклассов для разделения тестов. Вложенные классы стоит использовать тогда, когда внешний к ним класс зависит от их функциональности. Или если эти классы инкапсулируют какую-то часть логики, которая актуальна только для внешнего класса. Как пример могу привести реализацию какой-нибудь древесной структуры. Снаружи у тебя будет доступен класс Tree, а внутри себя он инкапсулирует часть логики в классе TreeNode. public class Tree
{
// Какой-то код, с логикой дерева, публичные методы
private class TreeNode
{
// Код с логикой обработки конкретного узла дерева.
// Или просто внутреннее представление узла, которым пользуется класс выше, но наружу это пользы не несет
}
}В тестах же у тебя такой зависимости нет. Каждый из классов самостоятелен в том смысле, что может запускать размещенные в нем тесты и не полагается на соседние. К чему я веду - такое разделение на подклассы в тестах выглядит, на мой взгляд, искусственно. Да, в обозревателе тестов их смотреть удобно, но в обозревателе решения мы по прежнему будем видеть один класс ObjectPrinterAcceptanceTests.cs. Я не буду требовать этого изменения и оставлю решение за тобой, но все же предложу два варианта, как на мой взгляд было бы лучше:
|
|
Гуд. Полное решение засчитываю |
2 participants
@SquirrelLeonid