Skip to content

model builders added#4

Closed
DedSec256 wants to merge 12 commits into
masterfrom
ber.a/models
Closed

model builders added#4
DedSec256 wants to merge 12 commits into
masterfrom
ber.a/models

Conversation

@DedSec256

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread Alice.NET/Images/ImagesApi.cs Outdated

namespace Alice.Images
{
public class ImagesApi

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Тут бы я, на самом деле, выделил интерфейс.

Comment thread Alice.NET/Images/ImagesApi.cs Outdated
public ImagesApi(string skillId, string token)
{
_apiUrl = $"https://dialogs.yandex.net/api/v1/skills/{skillId}/images";
Client.DefaultRequestHeaders.Add("Host", "https://dialogs.yandex.net");

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Тут, увы, потенциальная бага, ибо, если создать несколько экземпляров класса - у их статического HttpClient будет попытка повторного добавления тех же header-ов. Хз пока что, насколько критично

{
public class TipButtonBuilder : ButtonBuilder<TipButton>
{
public ButtonBuilder<TipButton> Hide()

@DedSec256 DedSec256 Aug 6, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

тут должен быть TipButtonBuilder, конечно же. А вообще, нужно подумать про вынесение интерфейса билдера и возвращать IModelBuilder

{
private readonly string _token;
private readonly string _apiUrl;
private readonly IJsonSerializer _jsonSerializer = new JsonSerializer.JsonSerializer(); //чёт не нравится такая интеграция зависимостей

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Захуярим в ioc и в конструктор.
Надо будет на его подключение отдельное issue написать, чтоб в board отображать активность.

@DedSec256 DedSec256 Aug 6, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@MaxVortman. При инициализации ioc нужно будет сконфигурировать его, чтобы он просматривал только сборку Alice.NET, иначе загрузка пользовательского приложения может быть долгой. В любом случае, он лишь создаст накладные расходы, а нужен не в таком уж и большом количестве мест. Нужно ли нам тянуть за собой груз в виде ioc в таком небольшом проекте, а, к тому же, это будет неочевидно для пользователей, что там где-то конейнер что-то инициализирует? Нужно подумать.

@mshamrai mshamrai Aug 6, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@DedSec256 В рамках нашей сборки просматривать ничего не надо. Можем пихать нативно. Не думаю, что их много будет.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@MaxVortman, я уже и отвык что-то вручную пихать :D
Тогда пойдёт.

Comment thread Alice.NET/Images/ImagesApi.cs Outdated
public ImagesApi(string skillId, string token)
{
_token = token;
_apiUrl = $"https://dialogs.yandex.net/api/v1/skills/{skillId}/images";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Мне не нравится, что стринг открытый инитит.
Наверное, стоит сделать класс конфигов, который тоже через контейнер подключим. В нем все константы можно писать. Если их много расплодится, подумаем насчет xml конфига.

{
protected Card(CardType type)
{
Type = type;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Вообще это не нравится. Поле можно инитить и так:

public CardType Type { get; set; } = CardType.BigImage;

Ради одного поля создавать базовый класс? Чет не очень.
К тому же BigImageCard и CardItem дублируют код друг друга (и как следствие их билдеры тоже). Уж лучше сделать базовый класс под их поля, дабы избежать дублирования.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@MaxVortman, мне тоже не очень нравится, я подумаю, что можно сделать ещё покрасивее.

Comment thread Alice.NET/Models/Cards/Card.cs Outdated
/// <summary>
/// Тип карточки
/// </summary>
public enum CardType

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Хочу отдельную директорию Enums, где такое лежать будет.
А то не ясно, где искать в случае чего.

public ItemsListCardBuilder WithFooter(Footer footer)
{
Card.Footer = footer;
return this;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Не нравится такой способ задания хедера и футера.
Считаю, что лучше бы так:

public ItemsListCardBuilder WithFooter(Func<FooterBuilder, Footer> buildFooterFunc)
{
    Card.Footer = buildFooterFunc(new FooterBuilder());
    return this;
}

Тогда пользователь будет пользоваться более привлекательным вариантом:

cardBuilder.WithFooter(fb =>
   fb....
    .Build();
);

@DedSec256 DedSec256 Aug 6, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@MaxVortman, тоже подумал об этом. Тогда нужно будет то же самое провернуть везде и для Button. И я бы не убирал WithFooter(Footer footer), а лишь добавил бы твою перегрузку, ибо хз, как юзер собирается добавлять их.

where TCard : Card
{
_response.Card = card;
return this;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Тут тоже, что и с футером.
Мне не хотелось бы отдельно использовать много билдеров.
Кстати, их конструкторы можно скрыть как internal.

set {
foreach (var item in value)
{
item.Type = null;

@DedSec256 DedSec256 Aug 18, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Не очень нравится пока что, но зато теперь у нас есть инвариантный контракт, который гарантирует надёжность)!0!))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Но осталась проблема ссылочного типа - один из объектов массива могут поменять случайно позже. Хуёво, чё.

namespace Alice.Models.Builders.Cards
{
public class BigImageCardBuilder : CardBuilder<BigImageCard>
public interface IBigImageCardBuilder : IImageCardBuilder

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

чтобы не создавать его интерфейс и реализацию, можно в IImageCardBuilder добавить CreateBigImageCard(). Но тогда мы не сможем вообще различить билдер для обычного CardItem и BigImageCard

@DedSec256 DedSec256 closed this Feb 14, 2021
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