Шипицын Павел#275
Conversation
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class CircularCloudLayouter |
There was a problem hiding this comment.
Необходимо вынести интерфейс лейаутера
| private bool TryMoveTowardsCenterX(ref Rectangle rectangle) | ||
| { | ||
| var centerX = rectangle.X + rectangle.Width / 2; | ||
| var direction = Math.Sign(_center.X - centerX); | ||
|
|
||
| if (direction == 0) | ||
| return false; | ||
|
|
||
| var movedRectangle = rectangle; | ||
| movedRectangle.X += direction; | ||
|
|
||
| if (IntersectsWithAny(movedRectangle)) | ||
| return false; | ||
|
|
||
| rectangle = movedRectangle; | ||
| return true; | ||
| } | ||
|
|
||
| private bool TryMoveTowardsCenterY(ref Rectangle rectangle) | ||
| { | ||
| var centerY = rectangle.Y + rectangle.Height / 2; | ||
| var direction = Math.Sign(_center.Y - centerY); | ||
|
|
||
| if (direction == 0) | ||
| return false; | ||
|
|
||
| var movedRectangle = rectangle; | ||
| movedRectangle.Y += direction; | ||
|
|
||
| if (IntersectsWithAny(movedRectangle)) | ||
| return false; | ||
|
|
||
| rectangle = movedRectangle; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Методы TryMoveTowardsCenterX и TryMoveTowardsCenterY очень похожи. Попробуй объединить их в один общий метод.
| return true; | ||
| } | ||
|
|
||
| private IEnumerable<Point> GetSpiralPoints() |
There was a problem hiding this comment.
Лучше вынести в отдельный PointsProvider
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class GeneratorTagCloud |
There was a problem hiding this comment.
Странное название. Лучше TagCloudGenerator
| public class GeneratorTagCloud | ||
| { | ||
| private readonly TagCloudVisualizer _cloudVisualizer; | ||
| private readonly Func<Point, CircularCloudLayouter> _layouterFactory; |
There was a problem hiding this comment.
_layouterFactory должна быть снаружи генератора.
| public string GenerateFileName(string fileName) | ||
| { | ||
| var timestamp = DateTime.Now.ToString("yyyyMMdd_HHmmss"); | ||
| return $"{fileName}_{timestamp}.png"; | ||
| } |
There was a problem hiding this comment.
Генерация имени файла - тоже не ответственность визуализатора.
| public string OutputFileName { get; set; } | ||
| public Size? ImageSize { get; set; } |
There was a problem hiding this comment.
Этих настроек не должно быть здесь, нужно оставить только параметры, необходимые для генерации прямоугольников. Настройки, связанные с визуализацией лучше вынести в отдельный конфиг.
| { | ||
| center = new Point(100, 100); | ||
| layouter = new CircularCloudLayouter(center); | ||
| var dir = Directory.CreateDirectory($"../../../../TagsCloudVisualizationTests/test_results"); |
There was a problem hiding this comment.
Такие относительные пути имеют шанс сломаться. Лучше использовать AppDomain.CurrentDomain.BaseDirectory. К тому же, после разделения ответственностей здесь не должно остаться логики, связанной с сохранением в файл и визуализацией. Поэтому на визуализатор нужно писать отдельные тесты.
| for (var i = 0; i < 5; i++) | ||
| layouter.PutNextRectangle(new Size(45, 15)); | ||
|
|
||
| var rectangles = layouter.PlacedRectangles.ToList(); |
There was a problem hiding this comment.
В текущей реализации лейаутера PlacedRectangles итак коллекция, поэтому дополнительно перечислять её в лист - бессмысленно.
|
|
||
| var maxCount = quadrants.Max(); | ||
| var minCount = quadrants.Min(); | ||
| (maxCount <= minCount * 2).Should().BeTrue(); |
There was a problem hiding this comment.
Что здесь проверяется? Не очевидно, почему это условие должно выполняться. Поэтому лучше вынести в отдельный тест.
sln не может подгрузить проекты
|
в csproj ничего не лежит по папкам, сложно ориентироваться в файлах
|
|
Не могу прогнать тесты, из-за проблем с солюшеном |
| _minDimension = int.MaxValue; | ||
| } | ||
|
|
||
| public IReadOnlyList<Rectangle> PlacedRectangles => _placedRectangles.AsReadOnly(); |
There was a problem hiding this comment.
Это свойство не нужно, PutNextRectangle возвращает прямоугольник. Обращений к нему быть не должно более
| private readonly Point _center; | ||
| private readonly List<Rectangle> _placedRectangles; | ||
| private readonly ISpiralPointsProvider _pointsProvider; | ||
| private int _minDimension; |
There was a problem hiding this comment.
Это поле нужно для адаптивного изменения плотности точек спирали: чем меньше прямоугольники, тем плотнее спираль; чем больше прямоугольники, тем реже точки.
| if (rectangleSize.Width <= 0 || rectangleSize.Height <= 0) | ||
| throw new ArgumentException("Rectangle size must have positive dimensions"); | ||
|
|
||
| UpdateMinDimension(rectangleSize); |
| if (IntersectsWithAny(candidateRectangle)) | ||
| continue; | ||
|
|
||
| CompactRectangle(ref candidateRectangle); |
There was a problem hiding this comment.
Рефы не следует использовать без веской на то причины. Здесь причины их использовать нет
| private const double AngleStep = 0.05; | ||
| private const double RadiusStepFactor = 0.05; | ||
|
|
||
| private double _currentRadius; | ||
| private double _currentAngle; |
There was a problem hiding this comment.
Почему эти поля не задаются через конструктор?
|
|
||
| public interface IImageSaver | ||
| { | ||
| string GenerateFileName(string baseName); |
There was a problem hiding this comment.
Не используется в основном проекте, можно убрать
|
|
||
| public interface IVisualizer | ||
| { | ||
| Bitmap CreateVisualization(IEnumerable<Rectangle> rectangles, Point center, TagCloudVisualizationConfig config); |
|
|
||
| private bool IntersectsWithAny(Rectangle rectangle) | ||
| { | ||
| return _placedRectangles.Any(rect => rect.IntersectsWith(rectangle)); |
There was a problem hiding this comment.
Может лучше начинать с конца проверять?
| var centerCoord = axis == Axis.X ? rectangle.X + rectangle.Width / 2 : rectangle.Y + rectangle.Height / 2; | ||
|
|
||
| var targetCoord = axis == Axis.X ? _center.X : _center.Y; |
| for (var i = 0; i < 50; i++) | ||
| layouter.PutNextRectangle(new Size(45, 15)); | ||
|
|
||
| const double minDensityRatio = 0.3; |
There was a problem hiding this comment.
Не маловато ли значение? По-хорошему должно быть больше 0.6-0.7. Попробуй тут большее количество прямоугольников расположить
2 participants
@tripples25