ConfigLoader - add additional params #13
Conversation
| @@ -14,29 +14,26 @@ public class SnipeConfigLoader | |||
| private readonly string _url; | |||
| private readonly IApplicationInfo _appInfo; | |||
|
|
|||
| private string _requestParamsJson; | |||
| public SnipeConfigLoader(string projectID, IApplicationInfo appInfo) | |||
There was a problem hiding this comment.
отделяй методы пустой строкой
|
|
||
| public void Reset() | ||
| { | ||
| _config = null; |
There was a problem hiding this comment.
сначала нужно остановить загрузку (которая теоретически может выполняться в данный момент) через CancellationToken. иначе она завершится в параллельном потоке и присвоит своё значение конфигу вне зависимости от того, что тут ему присваивается null
| public SnipeConfigLoader(string projectID, IApplicationInfo appInfo) | ||
| { | ||
| _projectID = projectID; | ||
| _appInfo = appInfo; | ||
| _url = "https://config.snipe.dev/api/v1/configStrings"; | ||
| } | ||
|
|
||
| public async UniTask<Dictionary<string, object>> Load() | ||
| public void SetRequestParams(string requestParams) |
There was a problem hiding this comment.
а тут целый ряд проблем
- это публичный метод, значит вызвать его может кто угодно с какими угодно параметрами, следовательно, требуется проверка валидности входящих значений
- насколько я вижу, этот метод вызывается всего один раз и при этом непосредственно перед вызовом Load. в этом случае в данном методе нет никакой необходимости - можно просто передавать аргументы в сам метод Load
- параметры запроса содежат необходимые значения и дополнительные. нельзя, чтобы какой-то внешний класс мог затереть обязательные параметры. Т.е. метод BuildRequestParamsJson более уместен в этом классе, а не во внешнем сервисе
итого можно сделать что-то типа такого:
Load(Dictionary<string, object> additionalParams)
{
string requestParamsJson = BuildRequestParamsJson(additionalParams);
//...There was a problem hiding this comment.
а тут целый ряд проблем
- это публичный метод, значит вызвать его может кто угодно с какими угодно параметрами, следовательно, требуется проверка валидности входящих значений
- насколько я вижу, этот метод вызывается всего один раз и при этом непосредственно перед вызовом Load. в этом случае в данном методе нет никакой необходимости - можно просто передавать аргументы в сам метод Load
- параметры запроса содежат необходимые значения и дополнительные. нельзя, чтобы какой-то внешний класс мог затереть обязательные параметры. Т.е. метод BuildRequestParamsJson более уместен в этом классе, а не во внешнем сервисе
итого можно сделать что-то типа такого:Load(Dictionary<string, object> additionalParams) { string requestParamsJson = BuildRequestParamsJson(additionalParams); //...
Согласен, перенесу BuildRequestParamsJson() в SnipeConfigLoader(), а additionalParams буду передавать в метод Load()
| @@ -20,12 +24,14 @@ public class SnipeConfigLoadingService : ISnipeConfigLoadingService | |||
| private SnipeConfigLoader _loader; | |||
| private readonly string _projectID; | |||
|
|
|||
| private CancellationTokenSource _loadingCancellation; | |||
There was a problem hiding this comment.
не всё так просто...
если класс содерит disposable поля, то и сам класс должен быть IDisposable со всеми вытекающими
| return _config; | ||
| } | ||
|
|
||
| _loadingCancellation ??= new CancellationTokenSource(); |
There was a problem hiding this comment.
во-первых, нельзя игнорировать уже имеющийся токен
_loadingCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);во-вторых, у тебя тут есть проверка на случай, если _loadingCancellation != null это хорошо, но нет никакого действия. Если у нас уже есть какой-то ранее созданный CTS, значит мы уже запустили загрузку и она сейчас уже в процессе - требуется принять решение, как с ней поступить: либо отменить старую, либо отменить новую и ждать окончания старой
| _loading = true; | ||
|
|
||
| if (_loader == null) | ||
| { | ||
| await UniTask.WaitUntil(() => SnipeServices.IsInitialized, PlayerLoopTiming.Update, cancellationToken) | ||
| await UniTask.WaitUntil(() => SnipeServices.IsInitialized, PlayerLoopTiming.Update, _loadingCancellation.Token) |
There was a problem hiding this comment.
так можно, но только если используется LinkedTokenSource (см. выше), иначе мы игнорируем внешний cancellationToken, а он по логике главней нашего внутреннего
| .SuppressCancellationThrow(); | ||
|
|
||
| if (cancellationToken.IsCancellationRequested) | ||
| if (_loadingCancellation.Token.IsCancellationRequested) |
There was a problem hiding this comment.
сохрани токен в локальную переменную вместо того, чтобы каждый раз вызывать свойство _loadingCancellation.Token
обращения к свойствам - это фактически вызов метода, а это значит, что под капотом выполняется целая куча промежуточных действий с сохранением стека и сменой контекста - можно оптимизировать одной строкой
Теперь параметры храняться в в отдельном поле и заполняется в методе
SetRequestParams(), Вынес логику формирования запроса запроса на сервер вSnipeConfigLoadingService.cs, а также теперь список формируется с доп.параметрами.Добавлен метод
Reset()для сброса текущих значений и повторной выгрузки конфига.