Codereview - подскажите что в методе можно улучшить? - .NET

Здравствуйте, Sharov, Вы писали:

S>Такой вопрос, а для кого эти стратегии расчитаны? Т.е. это должен быть один экземпляр приложения,
S>который работает с бд. Как множество микросервисов с SequenceHiLo не пересекутся тогда?
S>Т.е. в рамках одного процесса все это можно контролировать, а если процессов больше?

Нет, не пересекутся. Смотри.

 CREATE SEQUENCE [dbo].[MySequence] START WITH 1 INCREMENT BY 10;

 SELECT NEXT VALUE FOR [dbo].[MySequence]; -- (1) возвращается 1

 SELECT NEXT VALUE FOR [dbo].[MySequence]; -- (2) возвращается 11

Получение NEXT VALUE синхронизованы на уровне движка БД, т.е. там что-то типа такого:

lock(syncLock) {
  var old = current
  current += 10;
  return old;
}

В (1) мы получили значение 1. И мы знаем, что шаг sequence = 10.
Это значит, что значения 1,2,3...10 можно спокойно использовать в качестве идентификаторов, т.к. следующий кто вызовет sequence получит уже 11.
Аналогично после шага (2) можно использовать 11,12...20 и т.д.

Это, к слову, не класический алгоритм HiLo (класический можешь загуглить), но там смысл такой же.

S> Если короче-- каковы сценарии hilo?

Минимизировать взаимодействие в БД при вставке графа объектов.
Если все ID уже назначены на клиентской стороне, то можно открыть транзакцию отправить все SQL INSERT и быстренько закомититься.
Не нужно после каждой вставки получать обратно SCOPE_IDENTITY(), как в случае с IDENTITY-столбцом.
В итоге вставка быстрее, транзакция короче, локов/дедлоков меньше.

Плюс, например, в логи можно писать ID-сущностей, не делая каждый раз SaveChanges.

Или иногда бывает нужна сквозная идентификация всех объектов в БД и поэтому отдельный IDENTITY на каждой таблицы не подходит.

Здравствуйте, Glestwid, Вы писали:

_FR>>Совсем не ясны пляски с _hashgen.EncodeLong(urlInDb.Id) Это такая попытка замаскировать от пользвателя сгенерированный идентификатор?

G>Нет, это пример сервиса short url. Там юзеру выдают хеш реального урла.

Тогда я б ещё вынес сохзранение данных в БД в отдельный метод, который возвращало бы, допустим, айдишник. А в этом самом методе получал бы айдишник и возвращал хеш.

G>Вот, на интервью скинули такой метод на ревью. Что в нем можно улучшить и(или) ускорить, помимо добавления логирования ошибок ?
убрать лишний пробел перед Url, и пустую строку перед throw

Здравствуйте, Glestwid, Вы писали:

G>Нет, это пример сервиса short url. Там юзеру выдают хеш реального урла.

Тогда метод следует назвать CreateHashAsync.

... << RSDN@Home 1.3.17 alpha 5 rev. 62>>

Здравствуйте, RushDevion, Вы писали:

RD>Нет, не пересекутся. Смотри.
RD>

RD> CREATE SEQUENCE [dbo].[MySequence] START WITH 1 INCREMENT BY 10;

RD> SELECT NEXT VALUE FOR [dbo].[MySequence]; -- (1) возвращается 1

RD> SELECT NEXT VALUE FOR [dbo].[MySequence]; -- (2) возвращается 11
RD>

RD>Получение NEXT VALUE синхронизованы на уровне движка БД, т.е. там что-то типа такого:
RD>

RD>lock(syncLock) {
RD>  var old = current
RD>  current += 10;
RD>  return old;
RD>}
RD>

RD>В (1) мы получили значение 1. И мы знаем, что шаг sequence = 10.
RD>Это значит, что значения 1,2,3...10 можно спокойно использовать в качестве идентификаторов, т.к. следующий кто вызовет sequence получит уже 11.
RD>Аналогично после шага (2) можно использовать 11,12...20 и т.д.

Ага, ясно, благодарю. Собственно, я на хабре нашел статью, где обсуждался этот механизм, и там соотв. таблица
(sequence) генерится EF на уровне бд. Соотв. вопрос -- значит при создании контекста бд\сессии он, вновь
созданный контекст, будет неявно ходить в бд для получения нужной последовательности? Иначе как мы узнаем hilo?

S>> Если короче-- каковы сценарии hilo?
RD>Плюс, например, в логи можно писать ID-сущностей, не делая каждый раз SaveChanges.

Кстати, не факт -- получить мы получили, но может быть проблема сохранить все это в бд. Таким образом
у нас появляется неконсистентность -- типа пишем, что есть ид от бд, хотя его в реальности еще нету.
Ну т.е. при попытке сохранить данные может что-то пойти не так.

RD>Или иногда бывает нужна сквозная идентификация всех объектов в БД и поэтому отдельный IDENTITY на каждой таблицы не подходит.

А что это такое? Типа общий, уникальный id для всех объектов в бд?

S>Ага, ясно, благодарю. Собственно, я на хабре нашел статью, где обсуждался этот механизм, и там соотв. таблица
S>(sequence) генерится EF на уровне бд. Соотв. вопрос -- значит при создании контекста бд\сессии он, вновь
S>созданный контекст, будет неявно ходить в бд для получения нужной последовательности? Иначе как мы узнаем hilo?

Ну да. Контекст ходит в БД для получения текущего значения sequence (или текущего Hi-значения в случае HiLo).
Смысл в том, что он ходит раз в N (e.g. 10-100) вставок, а не каждый раз.

S>Кстати, не факт -- получить мы получили, но может быть проблема сохранить все это в бд. Таким образом
S>у нас появляется неконсистентность -- типа пишем, что есть ид от бд, хотя его в реальности еще нету.
S>Ну т.е. при попытке сохранить данные может что-то пойти не так.

Ну и что? В логах ты это увидишь примерно такое:

[Info] Order #100 created
[Info] Order item #101 for Order #100 added
[Info] Order item #102 for Order #100 added
[Info] Saving order #100 to DB
[Error] Db unavailable exception

Смотришь лог. Понимаешь, что БД была недоступна, записи по факту не вставились. Ну бывает, чо.
Это ничем не отличается от случая, если бы вместо int'овых Id ты использовал, скажем, Guid.

> А что это такое? Типа общий, уникальный id для всех объектов в бд?
Да. Для аудита такое иногда просят: чтобы любой объект в БД глобально-уникально идентифицировался по Id.

НС>Тогда метод следует назвать CreateHashAsync.
Не очень понятны требования: нужно ли порождать разные хеши для одного и того же урла.

Здравствуйте, Sinclair, Вы писали:

S>Не очень понятны требования: нужно ли порождать разные хеши для одного и того же урла.

Судя по коду — нужно. По логике, гипотетически, лучше наверное расшаривать.

... << RSDN@Home 1.3.17 alpha 5 rev. 62>>

Здравствуйте, Glestwid, Вы писали:

G>Вот, на интервью скинули такой метод на ревью. Что в нем можно улучшить и(или) ускорить, помимо добавления логирования ошибок ?

G>


G>    public async Task<string> ToHash(CreateHashCommand request, CancellationToken cancellationToken)
G>    {
G>        try
G>        {
G>            if (string.IsNullOrWhiteSpace(request.Url))
G>                throw new InvalidEnumArgumentException();
G>            var urlInDb = new  Url()
G>            {
G>                Created = DateTime.UtcNow,
G>                CreatedBy = "",
G>                OriginalUrl = request.Url
G>            };
G>            await _context.Urls.AddAsync(urlInDb, cancellationToken);
G>            await _context.SaveChangesAsync(cancellationToken);
G>            var urlHash = _hashgen.EncodeLong(urlInDb.Id);

G>            return urlHash;
G>        }
G>        catch (Exception ex)
G>        {

G>            throw;
G>        }
G>    }

G>

static Func<long,string> _hashgen = ....;
public static string ToHash(this Url url) => _hashgen.EncodeLong(url.Id);

async Task<Url> SaveRequest(string requestUrl) 
{
     var urlInDb = new  Url()
            {
               Created = DateTime.UtcNow,
               CreatedBy = "",
               OriginalUrl = requestUrl
            };
           await _context.Urls.AddAsync(urlInDb, cancellationToken);
           await _context.SaveChangesAsync(cancellationToken);
         return urlInDb;
}

app.MapGet("/hash", async (string requestUrl) =>
{
    if (string.IsNullOrWhiteSpace(requestUrl)) return String.Empty;
    var url = await SaveRequest(requestUrl);
    return url.ToHash();
});

if (string.IsNullOrWhiteSpace(requestUrl)) return String.Empty;

так лучше не делать, ибо это узаконивает недокументированное поведение, которе рано или поздно пораждает легенды — типа так то оно так, но вот если...

Здравствуйте, Ночной Смотрящий, Вы писали:
НС>Судя по коду — нужно. По логике, гипотетически, лучше наверное расшаривать.
Там вопросы не только к логике. Это же, по идее, торчит в виде веб-сервиса. А это, в свою очередь, означает возможные глюки с потерей ответов.
Неудачно написанный клиент к такому веб-сервису ~~в считанные часы~~ за конечное время переполнит базу данных дубликатами.
Унификация хешей для одного и того же URL решает, в том числе, и эту проблему.

НС>>Судя по коду — нужно. По логике, гипотетически, лучше наверное расшаривать.
S>Там вопросы не только к логике. Это же, по идее, торчит в виде веб-сервиса. А это, в свою очередь, означает возможные глюки с потерей ответов.
S>Неудачно написанный клиент к такому веб-сервису ~~в считанные часы~~ за конечное время переполнит базу данных дубликатами.
S>Унификация хешей для одного и того же URL решает, в том числе, и эту проблему.

Полагаю, что уникальная ссылка для каждой конвертации это by design.
Чтобы отслеживать переходы по этим ссылкам и пр.
Поле CreatedBy там не просто так.

Посмотрите, как https://tinyurl.com/ работает.
Для нового пользователя (без cookies) новый короткий url.

https://tinyurl.com/27xwht42
https://tinyurl.com/2p9ct2ja

Судя по разделу My URLs в БД как раз url, timestamp и данные о пользователе.
В Pro плане — tracked clicks.

Типично это решается путем создания link table. то есть Urls отдельно, Users отдельно использование UserUrlClick. При этом совершенно непонятно почему createdby пустой, если это конечно, не TODO.

Здравствуйте, ????????G???????, Вы писали:

?G?>Типично это решается путем создания link table. то есть Urls отдельно, Users отдельно использование UserUrlClick.

Ты не понял о чем речь. Есть некий url. Пользователь А хочет считать клики по своей короткой ссылке на этот url, а пользователь В — по своей. Единственный способ различить чья ссылка — сделать для каждого из пользователей уникальный хеш.

... << RSDN@Home 1.3.17 alpha 5 rev. 62>>

OK. Согласен.

Здравствуйте, m2user, Вы писали:
M>Поле CreatedBy там не просто так.
Поле CreatedBy там заполняется "". Это случай анонимного пользователя, для которого не будет никакого подсчёта ссылок, а также возможности удалить шорткат или изменить его назначение.

M>Для нового пользователя (без cookies) новый короткий url.
M>https://tinyurl.com/27xwht42
M>https://tinyurl.com/2p9ct2ja

M>Судя по разделу My URLs в БД как раз url, timestamp и данные о пользователе.
M>В Pro плане — tracked clicks.
Pro план противоречит идее анонимного пользователя.
Очевидно, нам не показали исходник метода, применяемого для аутентифицированного пользователя. Вот в нём вполне можно создавать дубликаты.

Здравствуйте, Ночной Смотрящий, Вы писали:
НС>Ты не понял о чем речь. Есть некий url. Пользователь А хочет считать клики по своей короткой ссылке на этот url, а пользователь В — по своей. Единственный способ различить чья ссылка — сделать для каждого из пользователей уникальный хеш.
Тогда у нас в поле CreatedBy будут различные значения, и это будет две разных строки в таблице с разными ID.

Здравствуйте, Sinclair, Вы писали:

S>Тогда у нас в поле CreatedBy будут различные значения, и это будет две разных строки в таблице с разными ID.

Если учитывать это поле, то да, надо один хеш выдавать.

... << RSDN@Home 1.3.17 alpha 5 rev. 62>>

Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>if (string.IsNullOrWhiteSpace(requestUrl)) return String.Empty;

尿Ǥ푙>так лучше не делать, ибо это узаконивает недокументированное поведение, которе рано или поздно пораждает легенды — типа так то оно так, но вот если...

Почему же?

> null |> hash;;
val it: int = 0

можно для пустой строки создать специальный ресурс. пусть ходят кому интересно))

	От:	RushDevion
	Дата:	19.03.23 19:07
	Оценка:	13 (2) +1

От:	_FRED_	@ViIvanov
Дата:	20.03.23 07:17
Оценка:

	От:	denis.st
	Дата:	20.03.23 07:42
	Оценка:

	От:	Ночной Смотрящий
	Дата:	20.03.23 08:41
	Оценка:	+1

	От:	Sharov
	Дата:	20.03.23 10:06
	Оценка:

От:	Sinclair	https://github.com/evilguest/
Дата:	20.03.23 11:37
Оценка:	+1

	От:	尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒
	Дата:	21.03.23 09:33
	Оценка:	+1

	От:	vaa
	Дата:	22.03.23 02:02
	Оценка: