-
Notifications
You must be signed in to change notification settings - Fork 229
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
Попов Захар #201
base: master
Are you sure you want to change the base?
Попов Захар #201
Changes from 19 commits
195211f
741d7f6
ee91491
e920ea9
1ad02b7
aaa88e2
0d4319b
5a8ab71
a3f9304
76f46ac
14d48e8
a518eec
5381142
80b703a
437d5c4
741ea65
98b047f
0235996
3d8eeec
dd9a5cd
c35158a
000025c
866b9c9
eaa377b
2f02a57
51d00af
0a9a7c6
01cc083
1139482
9557b19
42110bb
b2fbe3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,3 +197,4 @@ FakesAssemblies/ | |
*.opt | ||
|
||
*Solved.cs | ||
/TagCloud/Properties |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
using FileSenderRailway; | ||
using FluentAssertions; | ||
using System.Drawing; | ||
using TagCloud.CloudLayouterPainters; | ||
|
||
namespace TagCloud.Tests.CloudLayouterPaintersTest | ||
{ | ||
[System.Diagnostics.CodeAnalysis.SuppressMessage("Interoperability", "CA1416:Проверка совместимости платформы", Justification = "<Ожидание>")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Это подавление сообщения об ограниченности использования класса Bitmap: Можно было сделать так:
Изначально добавлял игнорирование этой ошибки в GlobalSuppressions.cs:
Justification обозначает вроде как причину, почему это подавление добавлено. Студия по умолчания туда прописывает "<Ожидание>" - ожидается. что когда-то там исправят. Я понимаю, что это плохая практика и в продакшене так конечно не надо делать. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ожидается) Так может и поправим?) |
||
internal class CloudLayouterPainterTest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Кажется тут все тесты с одинаковым смыслом - невалидный параметр на входе и проверяется что выкидывается исключение с нужным текстом. Давай объедим все это дело в один тест с testcases. Тогда и setUp можно будет убрать. И еще не хватает проверки позитивных сценариев. Не только что все "как надо" ломается, а еще и что работает правильно |
||
{ | ||
private CloudLayouterPainter painter; | ||
|
||
[SetUp] | ||
public void SetUp() | ||
{ | ||
painter = new CloudLayouterPainter(new Size(1, 1)); | ||
} | ||
|
||
[Test] | ||
public void Draw_ThrowsException_WithEmptyTags() | ||
{ | ||
var expected = Result.Fail<Bitmap>("Список тегов пуст"); | ||
var actual = painter.Draw(new List<Tag>()); | ||
actual.Should().BeEquivalentTo(expected); | ||
} | ||
|
||
[Test] | ||
public void Draw_ThrowsException_WithTagsAsNull() | ||
{ | ||
|
||
var expected = Result.Fail<Bitmap>("Tags передан как null"); | ||
var actual = painter.Draw(null!); | ||
actual.Should().BeEquivalentTo(expected); | ||
} | ||
|
||
[Test] | ||
public void Draw_ThrowsException_WithTooSmallToFitImage() | ||
{ | ||
var expected = Result | ||
.Fail<Bitmap>("Все прямоугольники не помещаются на изображение"); | ||
var actual = painter | ||
.Draw( | ||
new Tag[] | ||
{ | ||
new Tag( | ||
"Test", | ||
new Rectangle(new Point(0, 0), new Size(100, 100))) | ||
}); | ||
actual.Should().BeEquivalentTo(expected); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,188 @@ | ||
using FluentAssertions; | ||
using System.Drawing; | ||
using TagCloud.CloudLayouterPainters; | ||
using TagCloud.CloudLayouters.CircularCloudLayouter; | ||
using TagCloud.CloudLayouterWorkers; | ||
using TagCloud.ImageSavers; | ||
using TagCloud.Tests.Extensions; | ||
|
||
namespace TagCloud.Tests.CloudLayouterTests.CircularCloudLayouterTests | ||
{ | ||
[TestFixture] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Где-то есть TestFixture, а где-то нет. Хочется придерживаться единого стиля. А вообще, в используемой версии nunit TestFixture необязателен |
||
[System.Diagnostics.CodeAnalysis.SuppressMessage("Interoperability", "CA1416:Проверка совместимости платформы", Justification = "<Ожидание>")] | ||
internal class CircularCloudLayouterMainRequirementsTest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Давай объединим CircularCloudLayouterMainRequirementsTest и CircularCloudLayouterTest. Не вижу смысла в выделении отдельного класса для теста с exception. + смущает "MainRequirements" в названии, я бы тоже убрала (если не согласен, то опиши зачем оно) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Я согласен, что конкретно сейчас название "CircularCloudLayouterMainRequirementsTest" не котируется, но оно появилось из первой части задания, как выделение проверки основным требованиям: Из работы с прошлым наставником, я лично подчерпнул, что надо не бояться создавать множество тестовых файлов. Это поможет хорошо структурировать их. И конкретно тут, мне нравится, как идейно сделано, ещё со времён первой части задания - у нас есть класс, отвечающий за основные требования к раскладке и всё остальное, куда вошла только проверка на исключение. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. А так, да. Создавать тестовые файлы не нужно бояться если есть на то причина. Очень советую почитать 9 главу "Микросевисы" Ричардсона |
||
{ | ||
private Point center = new Point(); | ||
private Rectangle[] rectangles; | ||
private List<Tag> tags; | ||
private readonly string failedTestsDirectory = "FailedTest"; | ||
|
||
private readonly ImageSaver imageSaver = new ImageSaver(); | ||
private readonly CloudLayouterPainter cloudLayouterPainter | ||
= new CloudLayouterPainter(new Size(5000, 5000)); | ||
|
||
[OneTimeSetUp] | ||
public void Init() | ||
{ | ||
Directory.CreateDirectory(failedTestsDirectory); | ||
} | ||
|
||
[SetUp] | ||
public void SetUp() | ||
{ | ||
var minRectangleWidth = 30; | ||
var maxRectangleWidth = 70; | ||
var minRectangleHeight = 20; | ||
var maxRectangleHeight = 50; | ||
var rectanglesCount = 1000; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. На мой взгляд, рандомить границы, для того, что бы затем рандомить размер внутри этих границ - оверкил в данном случае, так как в этом тесте мы проверяем соответствие основным требованиям из первой части задания для ICloudLayouter - форма, плотность, отсутствие пересечений прямоугольников. А эти значения используются для вспомогательного класса, которые и выдаёт размеры прямоугольников, которые затем будут расставлены. |
||
|
||
tags = new List<Tag>(); | ||
var circularCloudLayouter = new CircularCloudLayouter(); | ||
|
||
var randomWorker = new RandomCloudLayouterWorker( | ||
minRectangleWidth, | ||
maxRectangleWidth, | ||
minRectangleHeight, | ||
maxRectangleHeight); | ||
foreach (var rectangleProperty in randomWorker | ||
.GetNextRectangleProperties().GetValueOrThrow().Take(rectanglesCount)) | ||
{ | ||
tags.Add( | ||
new Tag( | ||
rectangleProperty.word, | ||
circularCloudLayouter.PutNextRectangle(rectangleProperty.size) | ||
.GetValueOrThrow())); | ||
} | ||
rectangles = tags.Select(x => x.Rectangle).ToArray(); | ||
} | ||
|
||
[TestCase(0.7, 1000)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Есть ли смысл выносить testCase когда он один? |
||
[Repeat(10)] | ||
public void ShouldPlaceRectanglesInCircle(double expectedCoverageRatio, int gridSize) | ||
{ | ||
var maxRadius = rectangles.Max( | ||
x => x.GetMaxDistanceFromPointToRectangleAngles(center)); | ||
var step = 2 * maxRadius / gridSize; | ||
|
||
var occupancyGrid = GetOccupancyGrid(gridSize, maxRadius, step); | ||
|
||
var actualCoverageRatio = GetOccupancyGridRatio(occupancyGrid, maxRadius, step); | ||
actualCoverageRatio.Should().BeGreaterThanOrEqualTo(expectedCoverageRatio); | ||
} | ||
|
||
[TestCase(15)] | ||
[Repeat(10)] | ||
public void ShouldPlaceCenterOfMassOfRectanglesNearCenter(int tolerance) | ||
{ | ||
var centerX = rectangles.Average(r => r.Left + r.Width / 2.0); | ||
var centerY = rectangles.Average(r => r.Top + r.Height / 2.0); | ||
var actualCenter = new Point((int)centerX, (int)centerY); | ||
|
||
var distance = Math.Sqrt(Math.Pow(actualCenter.X - center.X, 2) | ||
+ Math.Pow(actualCenter.Y - center.Y, 2)); | ||
|
||
distance.Should().BeLessThanOrEqualTo(tolerance); | ||
} | ||
|
||
[Test] | ||
[Repeat(10)] | ||
public void ShouldPlaceRectanglesWithoutOverlap() | ||
{ | ||
for (var i = 0; i < rectangles.Length; i++) | ||
{ | ||
for (var j = i + 1; j < rectangles.Length; j++) | ||
{ | ||
Assert.That( | ||
rectangles[i].IntersectsWith(rectangles[j]), | ||
Is.EqualTo(false), | ||
$"Прямоугольники пересекаются:\n" + | ||
$"{rectangles[i].ToString()}\n" + | ||
$"{rectangles[j].ToString()}"); | ||
} | ||
} | ||
} | ||
|
||
[TearDown] | ||
public void Cleanup() | ||
{ | ||
if (TestContext.CurrentContext.Result.FailCount == 0) | ||
{ | ||
return; | ||
} | ||
|
||
var name = $"{TestContext.CurrentContext.Test.Name}.png"; | ||
var path = Path.Combine(failedTestsDirectory, name); | ||
imageSaver.SaveFile(cloudLayouterPainter.Draw(tags).GetValueOrThrow(), path); | ||
Console.WriteLine($"Tag cloud visualization saved to file {path}"); | ||
} | ||
|
||
[OneTimeTearDown] | ||
public void OneTimeCleanup() | ||
{ | ||
if (Directory.Exists(failedTestsDirectory) | ||
&& Directory.GetFiles(failedTestsDirectory).Length == 0) | ||
{ | ||
Directory.Delete(failedTestsDirectory); | ||
} | ||
} | ||
|
||
private (int start, int end) GetGridIndexesInterval( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Хорошо что вынес в отдельные методы, так намного читабельнее |
||
int rectangleStartValue, | ||
int rectangleCorrespondingSize, | ||
double maxRadius, | ||
double step) | ||
{ | ||
var start = (int)((rectangleStartValue - center.X + maxRadius) / step); | ||
var end = (int)((rectangleStartValue | ||
+ rectangleCorrespondingSize - center.X + maxRadius) / step); | ||
return (start, end); | ||
} | ||
|
||
private bool[,] GetOccupancyGrid(int gridSize, double maxRadius, double step) | ||
{ | ||
var result = new bool[gridSize, gridSize]; | ||
foreach (var rect in rectangles) | ||
{ | ||
var xInterval = GetGridIndexesInterval(rect.X, rect.Width, maxRadius, step); | ||
var yInterval = GetGridIndexesInterval(rect.Y, rect.Height, maxRadius, step); | ||
for (var x = xInterval.start; x <= xInterval.end; x++) | ||
{ | ||
for (var y = yInterval.start; y <= yInterval.end; y++) | ||
{ | ||
result[x, y] = true; | ||
} | ||
} | ||
} | ||
return result; | ||
} | ||
|
||
private double GetOccupancyGridRatio(bool[,] occupancyGrid, double maxRadius, double step) | ||
{ | ||
var totalCellsInsideCircle = 0; | ||
var coveredCellsInsideCircle = 0; | ||
for (var x = 0; x < occupancyGrid.GetLength(0); x++) | ||
{ | ||
for (var y = 0; y < occupancyGrid.GetLength(0); y++) | ||
{ | ||
var cellCenterX = x * step - maxRadius + center.X; | ||
var cellCenterY = y * step - maxRadius + center.Y; | ||
|
||
var distance = Math.Sqrt( | ||
Math.Pow(cellCenterX - center.X, 2) + Math.Pow(cellCenterY - center.Y, 2)); | ||
|
||
if (distance > maxRadius) | ||
{ | ||
continue; | ||
} | ||
|
||
totalCellsInsideCircle += 1; | ||
if (occupancyGrid[x, y]) | ||
{ | ||
coveredCellsInsideCircle += 1; | ||
} | ||
} | ||
} | ||
return (double)coveredCellsInsideCircle / totalCellsInsideCircle; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
using FileSenderRailway; | ||
using FluentAssertions; | ||
using System.Drawing; | ||
using TagCloud.CloudLayouters.CircularCloudLayouter; | ||
|
||
namespace TagCloud.Tests.CloudLayouterTests.CircularCloudLayouterTests | ||
{ | ||
[TestFixture] | ||
internal class CircularCloudLayouterTest | ||
{ | ||
[TestCase(0, 100)] | ||
[TestCase(-1, 100)] | ||
[TestCase(100, 0)] | ||
[TestCase(100, -1)] | ||
public void PutNextRectangle_ThrowsException_OnAnyNegativeOrZeroSize( | ||
int width, | ||
int height) | ||
{ | ||
var size = new Size(width, height); | ||
var expected = Result.Fail<Rectangle>( | ||
"Размеры прямоугольника не могут быть меньше либо равны нуля"); | ||
var actual = new CircularCloudLayouter().PutNextRectangle(size); | ||
actual.Should().BeEquivalentTo(expected); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
using FileSenderRailway; | ||
using FluentAssertions; | ||
using System.Drawing; | ||
using TagCloud.CloudLayouterWorkers; | ||
|
||
namespace TagCloud.Tests.CloudLayouterWorkersTests | ||
{ | ||
internal class NormalizedFrequencyBasedCloudLayouterWorkerTest | ||
{ | ||
private readonly Dictionary<string, double> normalizedValues | ||
= new Dictionary<string, double> | ||
{ | ||
{ "three", 0.625 }, | ||
{ "one", 0.25 }, | ||
{ "two", 0.2917 }, | ||
{ "four", 1.0 }, | ||
}; | ||
|
||
[TestCase(0, 100)] | ||
[TestCase(-1, 100)] | ||
[TestCase(100, 0)] | ||
[TestCase(100, -1)] | ||
public void GetNextRectangleSize_ThrowsException_OnAnyNegativeOrZeroSize( | ||
int width, | ||
int height) | ||
{ | ||
var expected = Result | ||
.Fail<IEnumerable<(string word, Size size)>> | ||
($"Переданное числовое значение должно быть больше 0: \"{(width <= 0 ? width : height)}\""); | ||
|
||
var worker = new NormalizedFrequencyBasedCloudLayouterWorker( | ||
width, | ||
height, | ||
normalizedValues); | ||
var actual = worker.GetNextRectangleProperties(); | ||
actual.Should().BeEquivalentTo(expected); | ||
} | ||
|
||
[TestCase(100, 25, false)] | ||
[TestCase(100, 25, true)] | ||
public void GetNextRectangleSize_WorksCorrectly(int width, int height, bool isSortedOrder) | ||
{ | ||
var index = 0; | ||
string[]? keys = null; | ||
if (isSortedOrder) | ||
{ | ||
keys = normalizedValues | ||
.OrderByDescending(x => x.Value).Select(x => x.Key).ToArray(); | ||
} | ||
else | ||
{ | ||
keys = normalizedValues.Keys.ToArray(); | ||
} | ||
|
||
var worker = new NormalizedFrequencyBasedCloudLayouterWorker( | ||
width, | ||
height, | ||
normalizedValues, | ||
isSortedOrder); | ||
foreach (var rectangleSize in worker | ||
.GetNextRectangleProperties().GetValueOrThrow()) | ||
{ | ||
var currentValue = normalizedValues[keys[index]]; | ||
var expected = new Size((int)(currentValue * width), (int)(currentValue * height)); | ||
index += 1; | ||
rectangleSize.size.Should().BeEquivalentTo(expected); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
using FileSenderRailway; | ||
using FluentAssertions; | ||
using System.Drawing; | ||
using TagCloud.CloudLayouterWorkers; | ||
|
||
namespace TagCloud.Tests.CloudLayouterWorkersTests | ||
{ | ||
[TestFixture] | ||
internal class CircularCloudLayouterWorkerTests | ||
{ | ||
[TestCase(0, 100)] | ||
[TestCase(-1, 100)] | ||
[TestCase(100, 0)] | ||
[TestCase(100, -1)] | ||
public void GetNextRectangleSize_ThrowsException_OnAnyNegativeOrZeroSize( | ||
int width, | ||
int height) | ||
{ | ||
var expected = Result | ||
.Fail<IEnumerable<(string word, Size size)>> | ||
($"Переданное числовое значение должно быть больше 0: \"{(width <= 0 ? width : height)}\""); | ||
|
||
var worker = new RandomCloudLayouterWorker(width, width, height, height); | ||
var actual = worker.GetNextRectangleProperties(); | ||
actual.Should().BeEquivalentTo(expected); | ||
} | ||
|
||
[TestCase(50, 25, 25, 50)] | ||
[TestCase(25, 50, 50, 25)] | ||
public void GetNextRectangleSize_ThrowsException_OnNonConsecutiveSizeValues( | ||
int minWidth, | ||
int maxWidth, | ||
int minHeight, | ||
int maxHeight) | ||
{ | ||
var expected = Result | ||
.Fail<IEnumerable<(string word, Size size)>> | ||
("Минимальное значение не может быть больше максимального"); | ||
|
||
var worker = new RandomCloudLayouterWorker(minWidth, maxWidth, minHeight, maxHeight); | ||
var actual = worker.GetNextRectangleProperties(); | ||
actual.Should().BeEquivalentTo(expected); | ||
} | ||
} | ||
} |
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.
А для чего? Заметила что это нужно для обращения к Value класса Result. Но ведь это поле не просто так было сделано internal :) Например, вот здесь есть проверка что при вычислении ширины и высоты не было ошибок
![image](https://private-user-images.githubusercontent.com/187474303/404522353-ac65743c-c61e-4b51-9f38-9839ed5964d5.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzNjk5MzIsIm5iZiI6MTczOTM2OTYzMiwicGF0aCI6Ii8xODc0NzQzMDMvNDA0NTIyMzUzLWFjNjU3NDNjLWM2MWUtNGI1MS05ZjM4LTk4MzllZDU5NjRkNS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjEyJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxMlQxNDEzNTJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1iYzg4ZGU2OGExMzIzNDFhMTY1ZTQxZmJlNTc0Yjc5YjI3OGMwMjk1YjEyYjMzN2UzNTBiNjdiOGJhZTEyY2QxJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Q04BI83D2mimwZlX3YJSeLdjZAfL1vGrc_EIgMvNwCo)
Так почему бы вместо прямого доступа к Value не воспользоваться GetValueOrThrow?
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.
Это только для того, что бы класс Result.cs был виден для моих проектов. Можно было перекопировать этот класс в сам проект, но я решил просто сделать его видимым.
В работе программы нигде GetValueOrThrow не используется, потому что, как я понял, в этом и смысл задания - исключения не должны выбрасываются явно. Если же мы используем GetValueOrThrow, то у нас выбросится исключение на неверных данных.
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.
Не совсем понятно, почему бы публичный класс из другого проекта не будет виден без этого атрибута?)