Skip to content
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

Муканов Арман #223

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Муканов Арман #223

wants to merge 18 commits into from

Conversation

MNYOU
Copy link

@MNYOU MNYOU commented Dec 5, 2023

@the_homeless_god

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как считаешь можно ли хранить файлы в директории с кодом?
Если нет, то какое решение можно придумать?
Если да, то почему?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Думаю, это не очень хорошая практика. Возможные решения:

  1. Выделить директорию в проекте
  2. Хранить файлы в отдельном репозитории/пространстве

Copy link

@the-homeless-god the-homeless-god Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай назовём папку а-ля assets/images resources/images и также присвоим названия картинкам более осмысленные?

Comment on lines 3 to 7
![plot](./TagCloud2.jpeg)

![plot](./TagCloud3.jpeg)

![plot](./TagCloud4.jpeg)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как считаешь нужен ли уникальный alt у изображений?

Стоит ли подумать о caption или он не нужен?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alt отображается в случае, если изображение не может быть показано. Он поможет понять, что было на изображении, поэтому его стоит использовать.
Caption тоже будет полезным, потому что caption даёт дополнительную информацию об изображении и контексте.


public void SaveImage(string file, ImageFormat format)
{
bitmap.Save(file, format);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Какие есть идеи в случае появления ошибок в Save?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try catch...

}
}

public void SaveImage(string file, ImageFormat format)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как считаешь нужен ли тут String?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Думаю, что нужен, потому что будет неочевидно, куда сохраняется изображение

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Речь про string vs String

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string - это же просто alias для String. Поэтому я не вижу преимуществ в использовании String

using var g = Graphics.FromImage(bitmap);
using var brush = new SolidBrush(Color.White);

var pen = new Pen(brush, 3); ;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Есть ли решение по поводу магического числа 3?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Инкапсулировать в свойство.


public class LayouterVisualizer
{
private readonly Bitmap bitmap;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Что думаешь насчёт применения свойств здесь?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Думаю стоит переделать под свойство. Это даст больше гибкости будущем. Можно выполнять дополнительные действия при обращении или изменении объекта.

@the-homeless-god
Copy link

Дедлайн продлён на неделю по просьбе @MNYOU

}
}

public void SaveImage(string file, ImageFormat format)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Речь про string vs String

namespace TagsCloudVisualization;

[TestFixture]
public class Tests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Стоит иметь на каждый класс свой отдельный файл тестов

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ок


private void Dispose(bool fromDisposeMethod)
{
if (isDisposed) return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как считаешь что лучше?

if (isDisposed)
{
  return;
}

или

if (isDisposed) return;

или

if (!isDisposed) 
{
  ...
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 вариант, на мой взгляд лучше. Более читаемый, отсутствует вложенность.

}

currentAngle += GetAngleStep();
if (currentAngle > Math.PI * 2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше в отдельные функции унести содержимое условий
А для чисел завести константы

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ок

Copy link

@the-homeless-god the-homeless-god left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай уберем пробелы в названиях файлов картинок и вольем

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants