-
Notifications
You must be signed in to change notification settings - Fork 305
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
Горбатов Александр #212
base: master
Are you sure you want to change the base?
Горбатов Александр #212
Conversation
… rectangles in CircularCloudLayouter.cs
…dded in CircularCloudLayouterTests.cs
do | ||
{ | ||
var x = center.X + radius * Math.Cos(angle); | ||
var y = center.Y + radius * Math.Sin(angle); | ||
rectangle.Location = new Point((int) x, (int) y); | ||
|
||
angle += angleIncrement; | ||
|
||
if (!(angle >= 2 * Math.PI)) | ||
continue; | ||
|
||
angle = 0; | ||
radius += radiusIncrement; | ||
} while (IntersectsWithAny(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.
Давай вынесем алгоритм по построению спирали в отдельный класс
Допустим чтобы можно было его удобно протестировать и посмотреть твой код при разных условиях
public Rectangle GetCloudBorders() | ||
{ | ||
var left = PlacedRectangles.Min(r => r.Left); | ||
var right = PlacedRectangles.Max(r => r.Right); | ||
var top = PlacedRectangles.Min(r => r.Top); | ||
var bottom = PlacedRectangles.Max(r => r.Bottom); | ||
|
||
var width = right - left; | ||
var height = bottom - top; | ||
return new Rectangle(left, top, width, height); | ||
} |
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 bitmap = new Bitmap(borders.Width * scale, borders.Height * scale); | ||
var graphics = Graphics.FromImage(bitmap); | ||
|
||
var rectanglesWithShift = layouter.PlacedRectangles | ||
.Select(r => | ||
new Rectangle( | ||
(r.X - borders.X) * scale, | ||
(r.Y - borders.Y) * scale, | ||
r.Width * scale, | ||
r.Height * scale)); | ||
|
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.
Давай тоже вынесем в отдельный класс
[TestCase(true)] | ||
[TestCase(false)] |
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.
Посмотри на [Values], с ним намного проще будут выглядеть
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.
Остальные тесты можно на него тоже переделать
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.
Тесты в этот раз особо не глядел, следующий раз посмотрю
{ | ||
Assert.Throws<ArgumentException>(() => drawer.DrawRectangles(null!, new Rectangle(1,1,1,1))); | ||
} |
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 GetCloudBorders() | ||
{ | ||
if (PlacedRectangles is null || PlacedRectangles.Count == 0) | ||
throw new InvalidOperationException("The list of placed rectangles cannot be null or empty"); | ||
|
||
var left = PlacedRectangles.Min(r => r.Left); | ||
var right = PlacedRectangles.Max(r => r.Right); | ||
var top = PlacedRectangles.Min(r => r.Top); | ||
var bottom = PlacedRectangles.Max(r => r.Bottom); | ||
|
||
var width = right - left; | ||
var height = bottom - top; | ||
return new Rectangle(left, top, width, height); | ||
} |
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 radius = 0d; | ||
var angle = 0d; | ||
|
||
do | ||
{ | ||
var x = Center.X + radius * Math.Cos(angle); | ||
var y = Center.Y + radius * Math.Sin(angle); | ||
rectangle.Location = new Point((int) x, (int) y); | ||
|
||
angle += AngleIncrement; | ||
|
||
if (!(angle >= 2 * Math.PI)) | ||
continue; | ||
|
||
angle = 0; | ||
radius += RadiusIncrement; |
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.
Кажется ты каждый раз начинаешь считать от центра
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.
Плюс ты вынес вот этот отдельный клас, но допустим мы захотим рисовать по кругам, то нам все еще придется написать довольно много кода заново
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.
Кажется ты каждый раз начинаешь считать от центра
Да, но если это изменить, облако получается чуть менее плотным и менее красивым
public void GetCloudBorders_ThrowsInvalidOperationException_WhenListOfRectanglesIsNullOrEmpty( | ||
[Values(false, true)] bool isNull) |
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.
можно просто [Values] bool isNull
public class TagsCloudDrawer | ||
{ | ||
private readonly ICloudLayouter layouter; | ||
private readonly IRectangleDrawer drawer; |
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.
глобальный вопрос а чем сейчас занимается этот класс?
@Pasha0666