-
Notifications
You must be signed in to change notification settings - Fork 277
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
Азанов Андрей #225
base: master
Are you sure you want to change the base?
Азанов Андрей #225
Conversation
} | ||
|
||
[Test] | ||
public void DensityTest() |
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.
Одно из требований — высокая плотность размещения прямоугольников. Может быть так и назвать?
@@ -0,0 +1,31 @@ | |||
| |||
Microsoft Visual Studio Solution File, Format Version 12.00 | |||
# Visual Studio Version 17 |
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.
Как считаешь какую версию IDE стоит использовать и почему?
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.
Последнюю стабильную версию, как и с большинством другого ПО. У меня нет причин выбирать предыдущие версии. У 2019-ой студии вроде как поддержка в 2024-ом заканчивается, для редакции Community уж наверняка. Не очень перспективно выходит
using System.Drawing; | ||
using TagsCloudVisualization; | ||
|
||
var rnd = new Random(); | ||
var sizes = new Size[500]; | ||
for (var i = 0; i < sizes.Length; i++) | ||
{ | ||
sizes[i] = new(10 + rnd.Next(40), 1 + rnd.Next(40)); | ||
} | ||
var tagCloudLayouter = new CircularCloudLayouter(new(500, 500)); | ||
foreach (var size in sizes) | ||
{ | ||
tagCloudLayouter.PutNextRectangle(size); | ||
} | ||
TagCloudSaver.Save(tagCloudLayouter.TagCloud, "image.png"); |
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.
Не нужно. Использовалось для проверки визуализации
{ | ||
DrawRectangle(rect, bitmap); | ||
} | ||
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.
Что делать если будет IO ошибка?
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.
Ну, неуправляемые ресурсы должны освободиться благодаря using, а проблемы из-за которых произошла ошибка, наверное, должна решать вызывающая сторона
|
||
public static void Save(List<Rectangle> rectangles, string fileName) | ||
{ | ||
using var bitmap = new Bitmap(1000, 1000); |
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.
Нужно вычислить габариты облака и сделать расчёт параметров изображения
private static void DrawRectangle(Rectangle rectangle, Bitmap bitmap) | ||
{ | ||
var color = Color.FromKnownColor((KnownColor)(1 + random.Next(175))); | ||
for (var i = rectangle.Left; i <= rectangle.Right; i++) |
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.
Такое может случиться, если rectangle.Right == int.MaxValue
bitmap.SetPixel(i, rectangle.Top, color); | ||
bitmap.SetPixel(i, rectangle.Bottom, color); |
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.
Да. Можно воспользоваться встроенным методом Graphics.DrawRectangles и не мучиться. Или можно было вложить один цикл в другой, добавить условий и оставить один SetPixel
bitmap.SetPixel(rectangle.Left, i, color); | ||
bitmap.SetPixel(rectangle.Right, i, color); |
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.
Есть идеи что делать с дублированием кода?
|
||
namespace TagsCloudTest | ||
{ | ||
public class Tests |
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.
Как считаешь название файла должно быть таким же как имя класса?
И имя класса Tests не слишком ли обширно?
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.
Имя типа должно совпадать с именем класса. Я хотел все тесты разместить в классе Tests. Есть ли смысл его в таком случае переименовывать?
{ | ||
sizes.Add(new(10 + rnd.Next(40), 1 + rnd.Next(40))); | ||
} | ||
circularLayouter = new CircularCloudLayouter(new(500, 500)); |
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.
Да. Можно было бы отловить зацикливание
@the_homeless_god