model builders added#4
Conversation
|
|
||
| namespace Alice.Images | ||
| { | ||
| public class ImagesApi |
There was a problem hiding this comment.
Тут бы я, на самом деле, выделил интерфейс.
| public ImagesApi(string skillId, string token) | ||
| { | ||
| _apiUrl = $"https://dialogs.yandex.net/api/v1/skills/{skillId}/images"; | ||
| Client.DefaultRequestHeaders.Add("Host", "https://dialogs.yandex.net"); |
There was a problem hiding this comment.
Тут, увы, потенциальная бага, ибо, если создать несколько экземпляров класса - у их статического HttpClient будет попытка повторного добавления тех же header-ов. Хз пока что, насколько критично
| { | ||
| public class TipButtonBuilder : ButtonBuilder<TipButton> | ||
| { | ||
| public ButtonBuilder<TipButton> Hide() |
There was a problem hiding this comment.
тут должен быть TipButtonBuilder, конечно же. А вообще, нужно подумать про вынесение интерфейса билдера и возвращать IModelBuilder
| { | ||
| private readonly string _token; | ||
| private readonly string _apiUrl; | ||
| private readonly IJsonSerializer _jsonSerializer = new JsonSerializer.JsonSerializer(); //чёт не нравится такая интеграция зависимостей |
There was a problem hiding this comment.
Захуярим в ioc и в конструктор.
Надо будет на его подключение отдельное issue написать, чтоб в board отображать активность.
There was a problem hiding this comment.
@MaxVortman. При инициализации ioc нужно будет сконфигурировать его, чтобы он просматривал только сборку Alice.NET, иначе загрузка пользовательского приложения может быть долгой. В любом случае, он лишь создаст накладные расходы, а нужен не в таком уж и большом количестве мест. Нужно ли нам тянуть за собой груз в виде ioc в таком небольшом проекте, а, к тому же, это будет неочевидно для пользователей, что там где-то конейнер что-то инициализирует? Нужно подумать.
There was a problem hiding this comment.
@DedSec256 В рамках нашей сборки просматривать ничего не надо. Можем пихать нативно. Не думаю, что их много будет.
There was a problem hiding this comment.
@MaxVortman, я уже и отвык что-то вручную пихать :D
Тогда пойдёт.
| public ImagesApi(string skillId, string token) | ||
| { | ||
| _token = token; | ||
| _apiUrl = $"https://dialogs.yandex.net/api/v1/skills/{skillId}/images"; |
There was a problem hiding this comment.
Мне не нравится, что стринг открытый инитит.
Наверное, стоит сделать класс конфигов, который тоже через контейнер подключим. В нем все константы можно писать. Если их много расплодится, подумаем насчет xml конфига.
| { | ||
| protected Card(CardType type) | ||
| { | ||
| Type = type; |
There was a problem hiding this comment.
Вообще это не нравится. Поле можно инитить и так:
public CardType Type { get; set; } = CardType.BigImage;Ради одного поля создавать базовый класс? Чет не очень.
К тому же BigImageCard и CardItem дублируют код друг друга (и как следствие их билдеры тоже). Уж лучше сделать базовый класс под их поля, дабы избежать дублирования.
There was a problem hiding this comment.
@MaxVortman, мне тоже не очень нравится, я подумаю, что можно сделать ещё покрасивее.
| /// <summary> | ||
| /// Тип карточки | ||
| /// </summary> | ||
| public enum CardType |
There was a problem hiding this comment.
Хочу отдельную директорию Enums, где такое лежать будет.
А то не ясно, где искать в случае чего.
| public ItemsListCardBuilder WithFooter(Footer footer) | ||
| { | ||
| Card.Footer = footer; | ||
| return this; |
There was a problem hiding this comment.
Не нравится такой способ задания хедера и футера.
Считаю, что лучше бы так:
public ItemsListCardBuilder WithFooter(Func<FooterBuilder, Footer> buildFooterFunc)
{
Card.Footer = buildFooterFunc(new FooterBuilder());
return this;
}Тогда пользователь будет пользоваться более привлекательным вариантом:
cardBuilder.WithFooter(fb =>
fb....
.Build();
);There was a problem hiding this comment.
@MaxVortman, тоже подумал об этом. Тогда нужно будет то же самое провернуть везде и для Button. И я бы не убирал WithFooter(Footer footer), а лишь добавил бы твою перегрузку, ибо хз, как юзер собирается добавлять их.
| where TCard : Card | ||
| { | ||
| _response.Card = card; | ||
| return this; |
There was a problem hiding this comment.
Тут тоже, что и с футером.
Мне не хотелось бы отдельно использовать много билдеров.
Кстати, их конструкторы можно скрыть как internal.
| set { | ||
| foreach (var item in value) | ||
| { | ||
| item.Type = null; |
There was a problem hiding this comment.
Не очень нравится пока что, но зато теперь у нас есть инвариантный контракт, который гарантирует надёжность)!0!))
There was a problem hiding this comment.
Но осталась проблема ссылочного типа - один из объектов массива могут поменять случайно позже. Хуёво, чё.
| namespace Alice.Models.Builders.Cards | ||
| { | ||
| public class BigImageCardBuilder : CardBuilder<BigImageCard> | ||
| public interface IBigImageCardBuilder : IImageCardBuilder |
There was a problem hiding this comment.
чтобы не создавать его интерфейс и реализацию, можно в IImageCardBuilder добавить CreateBigImageCard(). Но тогда мы не сможем вообще различить билдер для обычного CardItem и BigImageCard
No description provided.