-
Notifications
You must be signed in to change notification settings - Fork 201
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
Бабин Георгий #168
base: master
Are you sure you want to change the base?
Бабин Георгий #168
Conversation
TagsCloudResult/README.md
Outdated
# Примеры раскладки с разными параметрами | ||
## Rectangles = 500; deltaAngle = 5°; distance = 2 | ||
![example1](https://github.com/tripples25/tdd/blob/8b58d8f0592e3592d28af7ce6bfe3c817a716d16/TagsCloudVisualization/layoutImages/new%20500%20rects%20angle%205%20distance%202.Png?raw=true) | ||
## Rectangles = 100; deltaAngle = 5°; distance = 2 | ||
![example1](https://github.com/tripples25/tdd/blob/8b58d8f0592e3592d28af7ce6bfe3c817a716d16/TagsCloudVisualization/layoutImages/new%20angle%205%20distance%202.Png?raw=true) | ||
## Rectangles = 100; deltaAngle = 10°; distance = 2 | ||
![example2](https://github.com/tripples25/tdd/blob/8b58d8f0592e3592d28af7ce6bfe3c817a716d16/TagsCloudVisualization/layoutImages/new%20angle%2010%20distance%202.Png?raw=true) | ||
## Rectangles = 100; deltaAngle = 5°; distance = 0.5 | ||
![example3](https://github.com/tripples25/tdd/blob/8b58d8f0592e3592d28af7ce6bfe3c817a716d16/TagsCloudVisualization/layoutImages/new%20angle%205%20distance%200.5.Png?raw=true) | ||
## Rectangles = 100; deltaAngle = 1°; distance = 0.5 | ||
![example4](https://github.com/tripples25/tdd/blob/8b58d8f0592e3592d28af7ce6bfe3c817a716d16/TagsCloudVisualization/layoutImages/new%20angle%201%20distance%200.5.Png?raw=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.
Спасибо за README, рекомендую отформатировать только его и перенести из заголовков второго уровня информацию про параметры, можно использовать таблички
if (dialog == DialogResult.OK) | ||
imageHolder.RecreateImage(imageSettings); | ||
} | ||
} |
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.
В конце файлов нужны пустые строки, можно настроить EditorConfig
public void Perform() | ||
{ | ||
var dialog = SettingsForm.For(imageSettings).ShowDialog(); | ||
if (dialog == DialogResult.OK) |
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 MenuCategory Category => MenuCategory.Settings; | ||
public string Name => "Источник..."; |
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.
Я бы утащил все тексты в Resources Dictionary
// container.RegisterType<TxtTextReader>().Keyed<TextReader>(FileExtension.Txt); | ||
// container.RegisterType<DocTextReader>().Keyed<TextReader>(FileExtension.Doc).Keyed<TextReader>(FileExtension.Docx); |
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 class None | ||
{ | ||
private None() | ||
{ | ||
} | ||
} |
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 в разных файлах
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 я брал из примера, что предоставили нам в Контуре. None в данном случае используется для представления возвращаемого значения void-методов. Какой-то дополнительной логики этот класс более не несёт.
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.
в названии файлов не следует использовать пробелы, также .png вместо .Png
TagsCloudResult/README.md
Outdated
|
||
Приложение для создания облак тегов на основе текста. | ||
|
||
![example1](https://github.com/tripples25/fp/blob/1a4585180188efc9c6a35736650295a4e470f916/TagsCloudResult/layoutImages/sample.png?raw=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.
В квадратных скобках переименовать название, у самого файла сделать адекватный путь и название без camelCase к примеру
Refresh(); | ||
Application.DoEvents(); |
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.
Думал запускать цикл при отрисовке тегов, чтобы отрисовывать в real-time. В целом, он теперь не нужен, да и не рекомендуем к использованию, если верить документации и форумам, поэтому убрал.
return items; | ||
} | ||
|
||
private static ToolStripMenuItem CreateTopLevelMenuItem(MenuCategory category, IList<IUiAction> items) |
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.
Ну, я не рассчитывал на создание других клиентов или других типов/видов кнопок, поэтому пренебрёг пластичностью и просто создал удобные для себя статические расширения.
Создал фабрику, правда, в качестве результата там пока ToolStripItem. Это позволит создавать другие типы кнопок, унаследованные от ToolStripItem, но не подменять их другими классами не связанными с ним.
|
||
container.RegisterAssemblyTypes(typeof(IUiAction).Assembly).AsImplementedInterfaces(); | ||
|
||
container.RegisterType<PictureBoxImageHolder>().As<PictureBoxImageHolder, IImageHolder>().SingleInstance(); |
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 Point MovePointAlongXAxis(Point point, Size rectangleSize) | ||
{ | ||
var offsetToCenter = new Size(-rectangleSize.Width / 2, -rectangleSize.Height / 2); | ||
var offsetX = Math.Sign(Center.X + offsetToCenter.Width - point.X); | ||
while (Center.X + offsetToCenter.Width - point.X != 0 && offsetX != 0) | ||
{ | ||
var newPoint = point.WithOffset(new Size(offsetX, 0)); | ||
if (!IsPlaceSuitableForRectangle(new Rectangle(newPoint, rectangleSize))) | ||
break; | ||
|
||
point = newPoint; | ||
} | ||
|
||
return point; | ||
} | ||
|
||
private Point MovePointAlongYAxis(Point point, Size rectangleSize) | ||
{ | ||
var offsetToCenter = new Size(-rectangleSize.Width / 2, -rectangleSize.Height / 2); | ||
var offsetY = Math.Sign(Center.Y + offsetToCenter.Height - point.Y); | ||
while (Center.Y + offsetToCenter.Height - point.Y != 0 && offsetY != 0) | ||
{ | ||
var newPoint = point.WithOffset(new Size(0, offsetY)); | ||
if (!IsPlaceSuitableForRectangle(new Rectangle(newPoint, rectangleSize))) | ||
break; | ||
|
||
point = newPoint; | ||
} | ||
|
||
return point; |
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.
Дублирование кода, магические цифры, не хватает SRP
if (!tagDraw.IsSuccess) | ||
return tagDraw; |
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.
Потому что по задумке метод должен вернуть Result с ошибкой на первом теге, который не сумел отрисоваться. Переписал на LINQ, дополнительно выделив методы, убрал часть вложенности.
|
||
private Color GetTagColor(Tag tag) | ||
{ | ||
return tag.Coeff > 0.75 ? tagsSettings.PrimaryColor : tag.Coeff > 0.35 ? tagsSettings.SecondaryColor |
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.
Переписал, использовав константы и оператор switch.
public DocTextReader(SourceSettings settings) : base(settings) | ||
{ | ||
} |
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 TxtTextReader(SourceSettings settings) : base(settings) | ||
{ | ||
} |
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.
Красивое 🚀
… simplified methods
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.
LGTM 🚀 само приложение не потестил, вопросов нет
TagsCloudResult/README.md
Outdated
|
||
Приложение для создания облак тегов на основе текста. | ||
|
||
![tagcloudsample](layoutImages/sample.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.
исправить tagcloudsample на tag_cloud_sample
исправить layoutImages/ на layout/images/
var res = dialog.ShowDialog(); | ||
|
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 res = dialog.ShowDialog(); | |
var res = dialog.ShowDialog(); |
Внесённые изменения
@the-homeless-god