-
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
Щетников Павел #32
Щетников Павел #32
Conversation
VS has been broken, and i wasn`t able to check that all tests was succsessefull, witch means i was not succsessefull :P
Its not quite TDD, but at least i find use of tests.
yield return act(); | ||
} | ||
|
||
public static int Space(this Size size) => |
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.
Почему Space, а не Area? Если бы метод назвался Area - то было бы понятно, что он возвращает число, а что возвращает метод Space не понятно: занимаемое пространство?
|
||
public Rectangle Bounds { get; private set; } | ||
public List<Rectangle> Body { get; } | ||
public double Fillness => Body.Select(x => x.Size.Space()).Sum() / (double)Bounds.Size.Space(); |
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.
Почему не density? По-моему density - выглядит более понятным словом.
Вместо того, что бы вызывать сначала .Select(), а потом .Sum(), можно один раз вызвать метод .Sum(), которы й принимает лямбду (ты уже делал так в другом файле)
return rect; | ||
} | ||
|
||
if (layout.Sum(x => x.Bounds.Height) < layout.Max(x => x.Bounds.Width) || |
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 static Rectangle Unite(this IEnumerable<Rectangle> enumerable) |
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.
Unite звучит, как математическое объединение множеств. Здесь ты скорей ищишь minimum-area enclosing rectangle.
Я бы лучше назвал метод EnclosingRectangle или BoundingRectangle.
var x2 = coll.Select(x => x.Right).Max(); | ||
var y1 = coll.Select(x => x.Top).Min(); | ||
var y2 = coll.Select(x => x.Bottom).Max(); | ||
return new Rectangle(x1, y1, x2 - x1, y2 - y1); |
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.
Переменным можно дать более понятные названия.
}); | ||
|
||
[Test] | ||
public void BoundsWidthShouldMatchReactanglesSum() |
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.
У тебя название класса CircularCloudLayouter_Should заканчивается на слово should. Если ты выбрал такой стиль наименования тестов, то название класса + название метода должно читаться, как одно предложение на естественном языке, а у тебя название метода читается, как отдельное предложение, которое ни как не связано с названием класса.
|
||
public static Rectangle Unite(this IEnumerable<Rectangle> enumerable) | ||
{ | ||
var coll = enumerable.ToArray(); |
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.
Зачем здесь .ToArray()? Ради производительности ? Что-бы избежать "Possible multiple enumeration"? Можно написать этот метод с упором на производительность, пожертовав качеством, а можно написать красивый код, который работает далеко не оптимально. Сейчас метод и не идеально оптимизирован, и написан не очень понятно.
private const int MaxHeight = 50; | ||
private const int MaxWidthHeightRatio = 10; | ||
private const int Hundred = 100; | ||
private const int Thousand = 100; |
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.
Лол
|
||
namespace TagsCloudVisualization | ||
{ | ||
public static class Extensions |
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.
Для всего этого нужны тесты.
Лучше разбить на много маленьких файлов: например SizeExtensions, RectangleExtensions. Сейчас extension-методы к Rectangle и Size находятся в перемешке.
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 расширяемых объектов. Будет меньше классов с 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.
А чем плохи классы с одним методом? Главное что-бы эти классы соблюдали Single Responsibility Principle. Для extension методов обычно принято создавать новый класс для каждого типа.
} | ||
|
||
[Test] | ||
public void MakeSquareFrom3Bricks1X3() |
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.
Тест очень сильно завязан на конкретную реализацию, не стоит так делать. Вместо этого лучше проверять плотность заполнения. Не стоит пытаться уложить всё слишком плотно, так что-бы плотность была близка к 1, плотности 0.2 будет достаточно.
public static int Area(this Size size) => | ||
size.Height * size.Width; | ||
|
||
public static Size HeightSize(this Size size) => |
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.
По названию метода плохо понятно, что он делает.
Единственное место, где используется этот метод - в методе получения всех точек прямоугольника, почему-бы вместо этого не написать extension метод .AddHeight() для класса Rectangle.
public static Size WidthSize(this Size size) => | ||
new Size(0, size.Width); | ||
|
||
public static Size Divide(this Size size, int by) => |
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.
Я вижу, что этот метод вызывается только с параметром 2, что-бы найти центр прямоугольника. Разумнее сделать метод, который будет сразу возвращать центр прямоугольника.
if (isLeft) | ||
{ | ||
rect = new Rectangle(new Point(Bounds.Left - rectangleSize.Width, Bounds.Top), rectangleSize); | ||
body.Insert(0, rect); |
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.
.Insert - тяжёлая операция, которая работает за O(n), не нашёл ни одного места, где важен порядок прямоугольников в строчке. Можно использовать метод .Add(), амортизированная сложность которого O(1).
{ | ||
rect = new Rectangle(new Point(Bounds.Left - rectangleSize.Width, Bounds.Top), rectangleSize); | ||
body.Insert(0, rect); | ||
Bounds = new []{rect,Bounds}.EnclosingRectangle(); |
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.
Строчку можно вынести из if'а
public IEnumerable<Rectangle> Body => body; | ||
//public double Density => Body.Sum(x => x.Size.Area()) / (double)Bounds.Size.Area(); | ||
|
||
private bool isLeft; |
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 Point Center { get; } | ||
public IEnumerable<Rectangle> Layout => layout.SelectMany(x=>x.Body); |
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 false; | ||
} | ||
|
||
private bool CaseForNewRow(int 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.
Непонятное название
|
||
namespace TagsCloudVisualization.Tests | ||
{ | ||
[TestFixture] |
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 const int MinHeight = 6; | ||
private const int MaxHeight = 50; | ||
private const int MaxWidthHeightRatio = 10; | ||
protected const int Hundred = 100; |
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.
Во первых Hundred.Times имхо смотрится лучше чем 100.Times.
Во вторых сила привычки выносить константы.
|
||
public Rectangle Bounds { get; private set; } | ||
public IEnumerable<Rectangle> Body => body; | ||
//public double Density => Body.Sum(x => x.Size.Area()) / (double)Bounds.Size.Area(); |
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 static Bitmap DrawCloud(this IEnumerable<Tuple<string,int>> wordWeightPairs, | ||
Size bitmapSize, Func<Size,Rectangle> rectanglePutter, | ||
string fontName = "NewTimesRoman",Brush brush = null) |
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.
Зачем каждый раз передавать название шрифта и Brush? Можно один раз передать их в конструкторе.
namespace TagsCloudVisualization | ||
{ | ||
public static class LayoutDrawer | ||
{ |
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, а создавать 2 штуки жаба душит, к тому же они конфликтуют.
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 в поле класса.
Начал организовывать конвеер.
Получилось, что методы конвеера имеют сигнатуру кортежа из 3-4 объектов.
Оказалось что С# не haskell.
Меньше читабельности, больше синтаксиса при работе с такими кортежами.
Вернул статику, как меньшее зло.
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 static class LayoutDrawer | ||
{ | ||
public static Bitmap DrawCloud(this IEnumerable<Tuple<string,int>> wordWeightPairs, |
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.
Некрасиво передавать Tuple<string,int> в качестве параметра, можно использовать ValueTuple с именованными параметрами.
cs/TagsCloudVisualization/Program.cs
Outdated
.Save(".\\btm.png",ImageFormat.Png); | ||
} | ||
|
||
private static IEnumerable<string> FileReadWords(string path) => |
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.
ReadWordsFromFile?
cs/TagsCloudVisualization/Program.cs
Outdated
private static IEnumerable<string> FileReadWords(string path) => | ||
File.ReadAllText(path) | ||
.ToLower() | ||
.Split() |
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.
В метод Split() можно передать массив символов, по которым производить разделение, а также StringSplitOptions.RemoveEmptyEntries
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.
Можно, но это в 2 раза менее лаконично.
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 Rectangle[] GenerateRectangles(int count)=> |
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.
Приватные методы должны располагаться в файле ниже, чем публичные.
{ | ||
[TestFixture] | ||
internal class CircularCloudLayouter_Should : LayouterTestsBase | ||
{ |
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 Should_put_rectangles_with_increasing_heights_in_circle()
{
center = new Point(500, 500);
layouter = new CircularCloudLayouter(center);
for (int i = 10; i < 40; i++)
layouter.PutNextRectangle(new Size(50, i));
var enclosingRectangleSize = layouter.Layout.EnclosingRectangle().Size;
((double)enclosingRectangleSize.Width / (double)enclosingRectangleSize.Height).Should().BeInRange(0.5, 1.5);
}
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 static class CircleCloudDrawer | ||
{ | ||
|
||
public static Bitmap DrawCloud(this IEnumerable<(string, int)> wordWeightPairs,Size bitmapSize, |
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.
Зачем здесь extension метод? IEnumerable<(string, int)> не должен отвечать за то, что-бы уметь рисовать себя. Это ответсвенность класса-сервиса (у тебя уже класс называется CircleCloudDrawer, а не SomethingExtension).
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.
(string word, int count) - можно задать имя полям кортежа здесь, а в foreach использовать var
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.
И не count, а fontSize.
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.
(string word, int count) - можно задать имя полям кортежа здесь, а в foreach использовать var
у меня почему-то это не получилось
if (TryAddNewRow(rectangleSize,out var rect)) | ||
return rect; | ||
|
||
return layout.Where(x => x.Bounds.Height >= rectangleSize.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.
layout - поле, содержащее список RowLayout, по названию поля это непонятно. В лямбда выражении x тоже можно назвать так, что-бы было понятно к какому типу она принадлежит.
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.
Не исправлено. Назови поле layout понятней, что-бы было понятно, что это список строк.
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.
Не исправлено, непонятное название поля layout создаёт большие сложности при попытке понять, что происходит в этой строке.
|
||
if (IsCaseForNewRow(rectangleSize.Height)) | ||
{ | ||
var heights = layout.Select(x => x.Bounds.Width).ToArray(); |
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.
Почему в переменную heights мы положили select по .Width?
{ | ||
|
||
public static Bitmap DrawCloud(this IEnumerable<(string, int)> wordWeightPairs,Size bitmapSize, | ||
Func<Size, Rectangle> rectanglePutter, string fontName = "NewTimesRoman", Brush brush = null) |
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.
Если хочешь абстрагироваться от текущей реализации CircularCloudLayouter создай интерфейс ICircularCloudLayouter и передай его в метод, передавать Func<> в метод здесь выглядит странно.
Func<> обычно хорошо использовать, когда ты передаёшь чистую функцию без сайд эффектов, здесь за функцией прячется stateful сервис CircularCloudLayouter
.Select(x => x - centerSize) | ||
.ToArray(); | ||
|
||
var octogonEdgesDist = new[] |
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.
А почему сейчас метод принимает какие-то векторы?
Я бы декомпозировал так:
Первый метод, принимает множество квадратов и аппрокимирует его восьмиугольником
Второй принимает многоугольник (возможно просто массив точек) и точку, и возвращает расстояния до этой точки.
{ | ||
if (!enumerator.MoveNext()) | ||
return; | ||
var list = new List<TIn>{enumerator.Current}; |
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.
Не понял, мы создаём список с одним элементом enumerator.Current, а потом пробегаемся по нему через foreach?
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.
Хорошо, что исправил, что-бы не приходилось искать все подобные баги глазами я и говорил тебе покрывать все extension методы тестами.
К методу у меня есть ещё придирки, например я не вижу смысла руками создавать .GetEnumerator(), вместо использования foreach.
Но на самом деле метод уже нигде не используется и его можно просто удалить
if (IsCaseForNewRow(rectangleSize.Height)) | ||
{ | ||
var heights = layout.Select(x => x.Bounds.Width).ToArray(); | ||
rect = heights.Take(firstIndex).Sum() > heights.Skip(firstIndex + 1).Sum() ? |
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.
Как я понял по коду, мы разбиваем все строчки на два списка: выше и ниже центральной строчки?
Почему бы тогда сразу не хранить два списка?
Вроде бы в таком случае можно будет избавиться от дублирования кода и некрасивого .Insert(0,new RowLayout(rect)) ниже.
(я не продумал эту идею полностью, если она тебе не нравится можешь оспорить)
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.
Если использовать List то его придется реверсить через linq что не круто. Можно использовать Stack но у него нет общего интерфейса с list и не удастся избавится от дублирования. В идеале нужно создать коллекцию с 2 stateful режимами чтения, но это оверкилл.
cs/TagsCloudVisualization/Program.cs
Outdated
Func<Size,Rectangle> putter = new CircularCloudLayouter(new Point(pictureSize.Divide(2))).PutNextRectangle; | ||
|
||
var pairs = ReadWordsFromFile($"C:\\Users\\Avel\\Desktop\\hamlet.txt") | ||
.CountUnique() |
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.
По названию создаётся впечатление, что метод посчитает число уникальных элементов и вернёт его (тоже самое, что вызвать .Distinct().Count()).
Зачем такой функционал выносить в отдельный метод? Можно написать эти две строчки прямо здесь
.GroupBy(x => x)
.Select(x => (word:x.Key, count: x.Count())
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.
Когда там было больше строк выделение этого метода улучшало читабельность.
cs/TagsCloudVisualization/Program.cs
Outdated
private static IEnumerable<string> ReadWordsFromFile(string path) => | ||
File.ReadAllText(path) | ||
.ToLower() | ||
.AsEnumerable() |
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.
AsEnumerable() лишний
File.ReadAllText(path) | ||
.ToLower() | ||
.AsEnumerable() | ||
.SplitBy(Char.IsPunctuation) |
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.
Зачем создавать сложный метод .SplitBy(), если есть .Split(), который делает тоже самое и даже больше?
Сейчас ты разделяешь строку на char'ы и собираешь заново, .Split() сразу вернёт строки. И вместо двух строчек кода у тебя будет одна.
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.
Я не нашел удобный метод представления строки со знаками пунктуации, чтобы по ней можно было разбить используя .Split(), а писать сложный и непонятный .Split(" .,;!?" ,StringSplitOptions.RemoveEmptyEntries) я не хочу.
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 static class CircleCloudDrawer | ||
{ | ||
|
||
public static Bitmap DrawCloud(this IEnumerable<(string word,int fontSize)> wordWeightPairs,Size bitmapSize, |
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.
Если ты пишешь extension метод для класса Rectangle, который считает площадь, то это хороший метод, потому что считать свою площадь - подходящая ответственность для прямоугольника и результат этого метод предсказуем, если ты будешь считать площадь разными способами, то всегда получится одно и тоже число. Этот метод реализует какую-то сложную бизнес-логику и одного единого правильного способа нарисовать облако тэгов по списку слов нет, плюс это явно не ответственность IEnumerable<(string,int)> уметь себя рисовать. Если хочешь всё сделать красиво, то можешь выделить интерфейс ICircleCloudDrawer с методом DrawCloud.
cs/TagsCloudVisualization/Program.cs
Outdated
var denominator = pairs.Max(x => x.Item2); | ||
pairs.Select(x=>(x.Item1, maxFontSize*x.Item2/denominator)) | ||
.Where(x=>x.Item2>5) | ||
.DrawCloud(pictureSize, layouter) |
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.
.DrawCloud вернул IDisposable, IDisposable всегда нужно оборачивать в using
File.ReadAllText(path) | ||
.ToLower() | ||
.AsEnumerable() | ||
.SplitBy(Char.IsPunctuation) |
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.
Не понял, что тебе не нравится? Массив знаков пунктуации можно выделить в отдельное поле, тебе не нравилось писать все знаки пунктуации в методе?
.Select(x => x - centerSize) | ||
.ToArray(); | ||
|
||
var octogonEdgesDist = new[] |
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.
не исправлено
{ | ||
if (!enumerator.MoveNext()) | ||
return; | ||
var list = new List<TIn>{enumerator.Current}; |
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.
Хорошо, что исправил, что-бы не приходилось искать все подобные баги глазами я и говорил тебе покрывать все extension методы тестами.
К методу у меня есть ещё придирки, например я не вижу смысла руками создавать .GetEnumerator(), вместо использования foreach.
Но на самом деле метод уже нигде не используется и его можно просто удалить
[Test] | ||
public void HaveSameLayoutAsReturnedRectangles() | ||
{ | ||
GenerateRectangles(Hundred).ToArray().Should().BeEquivalentTo(layouter.Layout); |
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.
GenerateRectangles уже вернул массив, зачем здесь .ToArray()?
(комментарий к коду в целом: заметил, что ты очень часто вызываешь .ToArray() там, где это необязательно)
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 Rectangle[] GenerateRectangles(int count)=> | ||
count.Times(RandomSize).Select(layouter.PutNextRectangle).ToArray(); |
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 List<Rectangle> rectangles;
private void PlaceRectangles(int count)
{
for (var i = 0; i < count; i++)
rectangles.Add(layouter.PutNextRectangle(RandomSize()));
}
Обычно в метод .Select() передают чистую функцию. Это стоит воспринимать скорей как рекомендацию, а не как строгое требование. При передаче в .Select() функции с сайд эффектом ленивость тебе не нужна, а только мешает, и после вызова .Select() всегда нужно вызвать какой-нибудь не ленивый метод, например .ToArray(), смысл которого заставаить сайд эффект сработать.
yield return act(); | ||
} | ||
|
||
public static IEnumerable<IEnumerable<T>> SplitBy<T>(this IEnumerable<T> enumerable,Func<T,bool> predicate, bool removeEmpty = true) |
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.
Ты ведь возвращаешь list.ToArray(). Почему-бы не сделать сигнатуру этого метода IEnumerable<T[]>?
В единственном случае, когда ты используешь этот метод, ты опять вызываешь .ToArray().
В рамках этой задачи я не придираюсь, к производительности, но совет на будующее, если ты будешь когда-нибудь писать библиотеку, которая содержит extension-методы для IEnumerable, то стоит задуматься о производительности. (это не намёк на то, что все классы с extension-методами стоит сделать internal, переписывать код не надо)
if (TryAddNewRow(rectangleSize,out var rect)) | ||
return rect; | ||
|
||
return layout.Where(x => x.Bounds.Height >= rectangleSize.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.
Не исправлено. Назови поле layout понятней, что-бы было понятно, что это список строк.
return rect; | ||
|
||
return layout.Where(x => x.Bounds.Height >= rectangleSize.Height) | ||
.OrderBy(x => x.Bounds.Width) |
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.
Ты подключил MoreLinq через Nuget, в этой библиотеке есть метод .MaxBy(), его можно здесь использовать.
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.
Не исправлено, .MinBy(), а не .MaxBy() на самом деле.
|
||
public Rectangle PutNextRectangle(Size rectangleSize) | ||
{ | ||
if (TryAddNewRow(rectangleSize,out var rect)) |
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.
Сейчас код выглядит так: мы пытаемся добавить новую строчку и у нас может не получиться.
На самом деле нам нужно обратное: если не получилось вместить прямоугольник в существующие строки, то мы создаём новую строку.
Как вариант я бы написал так: создаём метод TryFindFittingRow(), если он вернул true добавляем прямоугольник в строчку, которую вернул данный метод. Если он вернул 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.
Скажи пожалуйста хотя-бы почему тебе не нравится эта идея. В том, как написано сейчас мне не нравится то, что выглядит так: мы пытаемся добавить новую строку и у нас может не получиться, а на самом деле не получиться у нас может найти подходящую строку, а добавить новую строку мы всегда можем.
#32 (comment) |
Я пробовал это выделить и мне показалось что это плохо выглядит вне контекста. |
return rect; | ||
|
||
return layout.Where(x => x.Bounds.Height >= rectangleSize.Height) | ||
.OrderBy(x => x.Bounds.Width) |
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.
Не исправлено, .MinBy(), а не .MaxBy() на самом деле.
if (TryAddNewRow(rectangleSize,out var rect)) | ||
return rect; | ||
|
||
return layout.Where(x => x.Bounds.Height >= rectangleSize.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.
Не исправлено, непонятное название поля layout создаёт большие сложности при попытке понять, что происходит в этой строке.
} | ||
|
||
rect = default(Rectangle); | ||
return false; | ||
} | ||
|
||
private bool IsCaseForNewRow(int 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.
По названию непонятно, что делает этот метод, не заглядывая в его код. Если не нравится моя идея с рефакторингом (TryFindFittingRow), лучше вернуть как было и проверять условие layout.Sum(x => x.Bounds.Height) < layout.Max(x => x.Bounds.Width) || layout.Max(x => x.Bounds.Height) < height
в методе PutNextRectangle, читаемость кода можно повысить назвав поле layout понятней.
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.
IsCaseForNewRow всегда был выделен в отдельный метод. Я по большей части отрефакторил TryAddNewRow.
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.
layout кстати тоже переименовал, но райдер слетел и я забыл сделать это во второй раз.
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.
И MinBy() тоже
cs/TagsCloudVisualization/Program.cs
Outdated
.Where(x=>x.Item2>5) | ||
.DrawCloud(pictureSize, layouter) | ||
.Save(".\\btm.png",ImageFormat.Png); | ||
var denominator = countedWords.Max(x => x.Item2); |
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.
Почему x.Item2? Ты ведь дал параметрам кортежа название
No description provided.