-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Конина Анастасия #229
base: master
Are you sure you want to change the base?
Конина Анастасия #229
Conversation
public class CircularCloudLayouter : ITagCloudLayouter | ||
{ | ||
private readonly Point cloudCenter; | ||
private readonly IList<Rectangle> _rectangels = new List<Rectangle>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Снова о нейминге полей.
Нужно использовать единый подход. Либо для всех полей по всем проекте использовать префикс_
, либо без него, но при использовании полей использоватьthis
.
Мешать эти подходы не нужно. -
Не замечание, но у нас в проекте принято выравнивать поля лесенкой. Тогда было вот так, что, на мой взгляд, симпатичнее. Но это чистой воды вкусовщина.
private readonly Point _cloudCenter;
private readonly SpiralPointsGenerator _spiralPointsGenerator;
private readonly IList<Rectangle> _rectangels = new List<Rectangle>();
public CircularCloudLayouter(Point cloudCenter) | ||
{ | ||
this.cloudCenter = cloudCenter; | ||
spiralPointsGenerator = new SpiralPointsGenerator(cloudCenter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= new SpiralPointsGenerator(cloudCenter);
делает твой класс жестко связанным с SpiralPointsGenerator
. Как правило, подобного рода зависимости должны инжектиться, т.е. внедряться через конструктор.
Почитай про принцип инверсии зависимостей, там немного, но полезно и интересно.
Ну а если совсем коротко, то твой генератор реализует интерфейс IPointsGenerator
, его и нужно использовать в полях и конструкторе.
continue; | ||
|
||
_rectangels.Add(rectangle); | ||
return rectangle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Перед return
принято оставлять пустую строку.
return new Rectangle(cloudCenter, rectangleSize); | ||
} | ||
|
||
static void Main(string[] args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Зачем здесь нужен метод
Main
? - Всегда и везде указывай модификатор доступа. Кроме членов интерфейса - там не надо, сейчас у тебя с этим всё как надо.
- Для подобных заглушек необязательно переносить строки, можно было бы написать
private static void Main(string[] args) {}
.
this.radiusStep = radiusStep; | ||
} | ||
|
||
///<exception cref="System.OverflowExeption"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почти, но не совсем. Описание метода должно быть отдельно - в summary
. Исключение отдельно - в <exception>
. А вот <exception cref="System.OverflowException">сюда уже можно добавить, почему может вываливаться исключение</exception>
rectangles = new List<Rectangle>(); | ||
for (var i = 0; i < 100; i++) | ||
{ | ||
rectangles.Add(cloud.PutNextRectangle(rectangleSize)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Все прямоугольники тут одинакового размера. Т.е. твое облако тегов в таком случае так же должно состоять из слов одинакового размера, что является пограничным случаем. К тому же у тебя все прямоугольники в виде столбцов var rectangleSize = new Size(10, 50);
, а не строк, а алгоритм раскладки не отдает предпочтения горизонтальному расположению.
Использовать Random
можно, но возникает вероятность создать тест, который будет то падать, то проходить. Лучше подумать над тем, как создать источник данных, который бы не менялся при запусках тестов, но при этом мог создавать различные прямоугольники. Но это всё в идеале, конечно же.
using System.Collections.Generic; | ||
using TagsCloudVisualization; | ||
|
||
public class CircularCloudLayouter : ITagCloudLayouter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
К вопросу о методе Main
добавляется вопрос, почему этот файл называется Program
?
foreach (var point in spiralPointsGenerator.GetPoints()) | ||
{ | ||
var rectangle = new Rectangle(point, rectangleSize); | ||
if (_rectangels.Any(x => x.IntersectsWith(rectangle))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Переделывать не надо, но есть ощущение (смотря на твои изображения), что генератор не отдает предпочтения горизонтальному расположение прямоугольников - хотя большая часть слов располагаться будет все же горизонтально.
rectangle.Should().BeEquivalentTo(expectedRectangle); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Думаю, стоит добавить тестов на генератор точек. Например, я могу написать var pointGenerator = new SpiralPointsGenerator(center, -10000);
Что случится с твоим кодом? В теста ответа нет.
var gr = Graphics.FromImage(bitmap); | ||
gr.FillRectangle(Brushes.Black, 0, 0, 500, 500); | ||
gr.FillRectangles(Brushes.White, rectangles); | ||
bitmap.Save(fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Что будет, если вывалится IOException
?
No description provided.