-
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
Andronov Alexander #178
base: master
Are you sure you want to change the base?
Andronov Alexander #178
Conversation
{ | ||
public static AppOptions CreateOptions(Options clOptions, IConfiguration defaultConfig) | ||
{ | ||
var tagCloudOptions = CreateTagCloudOptions(clOptions, defaultConfig); |
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.
ну ладно(
|
||
var result = textLoader.Load(inp) | ||
.Then(text => tagCloud.CreateCloud(text)) | ||
.Then(image => imageStorage.Save(image, outp)) |
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.
Лайк за Fluent-интерфейс
public static class DependencyInjection | ||
{ | ||
public static IServiceCollection AddDomain | ||
(this IServiceCollection services, |
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.
Снова сомнительное форматирование
} | ||
|
||
services.AddScoped<IWordsFilter, LengthFilter>(); | ||
services.AddScoped<IWordsFilter, MorphologicalFilter>(); |
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
?
{ | ||
var dict = new Dictionary<T, int>(); | ||
|
||
foreach (var item in 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.
Попробуй обойтись Linq, в таком случае сама надобность в интерфейсе и классе может отпасть
|
||
var parsed = result.Select(ParseResult).ToArray(); | ||
|
||
var filtered = parsed.Where(x => partsSpeech.HasFlag(x.partSpeech)).Select(x => x.word).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.
HasFlag
принимает тип Enum
. Можешь сказать, чем это чревато и можно ли сделать лучше?
}; | ||
} | ||
|
||
(string word, PartSpeech partSpeech) ParseResult(string result) |
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 enum PartSpeech | ||
{ | ||
None = 0, | ||
Adjective = 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.
Я бы воспользовался смещением / степенью
|
||
var dict = builder.Build(words); | ||
|
||
dict.Should().BeEquivalentTo(correctDict); |
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.
Как уже говорил, можно было впихнуть это в linq. Думается, что интерфейс, класс и тесты избыточны для подсчета частотности.
@@ -0,0 +1,32 @@ | |||
using FluentAssertions; | |||
|
|||
namespace TagCloud.Tests |
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
No description provided.