Codereview - подскажите что в методе можно улучшить?
От: Glestwid  
Дата: 16.03.23 18:08
Оценка: -1
Вот, на интервью скинули такой метод на ревью. Что в нем можно улучшить и(или) ускорить, помимо добавления логирования ошибок ?



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

            return urlHash;
        }
        catch (Exception ex)
        {

            throw;
        }
    }
c# codereview
Re: Codereview - подскажите что в методе можно улучшить?
От: IncremenTop  
Дата: 16.03.23 19:04
Оценка:
Здравствуйте, Glestwid, Вы писали:

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


1. Разнести все по слоям — хоть и спорно, но как минимум 2 слоя напрашивается. Соответственно как минимум отдельные методы по созданию сущности и сохранению
2. Работа с ошибками — зачем пользователю АПИ видеть, что там именно БД накрылась медным тазом? Должны быть скорее всего иные ошибки.
3. Трейсинг.
4. Очень странная ошибка на пустой урл. Я бы вообще валидатор прикрутил бы с нормальной проверкой на то, что это именно урл, а не три буквы.
5. Непонятно, что будет если урл уже в базе? В чем бизнес-цель метода?
Отредактировано 17.03.2023 12:03 VladD2 . Предыдущая версия .
Re: Codereview - подскажите что в методе можно улучшить?
От: GarryIV  
Дата: 16.03.23 19:28
Оценка: +1
Здравствуйте, Glestwid, Вы писали:

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



G>

G>    public async Task<string> ToHash(CreateHashCommand request, CancellationToken cancellationToken) // вы там определитесь command у вас или request
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); // AddAsync должен таки возвращать что-то
G>            await _context.SaveChangesAsync(cancellationToken); // хочется запустить оба и ждать только потом. хотя тут вообще хрень какая-то с этим Add&Save
G>            var urlHash = _hashgen.EncodeLong(urlInDb.Id); // жесть, мутабельный урл. 

G>            return urlHash; // можно и без urlHash обойтись, но это придирки 
G>        }
G>        catch (Exception ex)
G>        {
G>            throw; // нахрена ловили? тем более Exception
G>        }
G>    }

G>
WBR, Igor Evgrafov
Re: Codereview - подскажите что в методе можно улучшить?
От: Ночной Смотрящий Россия  
Дата: 16.03.23 19:32
Оценка: +2
Здравствуйте, Glestwid, Вы писали:

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


В дополнение к уже сказанному метод нужно назвать ToHashAsync.
... << RSDN@Home 1.3.17 alpha 5 rev. 62>>
Re: Codereview - подскажите что в методе можно улучшить?
От: bnk СССР http://unmanagedvisio.com/
Дата: 16.03.23 19:32
Оценка: 12 (3) +1
Здравствуйте, Glestwid, Вы писали:

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



Скорми ChatGPT:

В целом, ваш код выглядит неплохо, однако есть несколько замечаний и предложений по улучшению:

Имя метода ToHash не является самым информативным. Рекомендуется использовать более понятные имена, такие как GenerateUrlHashAsync или CreateUrlHashAsync.

Вместо InvalidEnumArgumentException лучше использовать ArgumentException или создать собственный класс исключения, например InvalidUrlException, так как InvalidEnumArgumentException обычно используется для проверки аргументов типа перечисления (enum).

В качестве CreatedBy используется пустая строка. Если это поле не является обязательным, возможно, стоит использовать null или добавить код для получения имени пользователя или системного аккаунта.

Добавьте XML-комментарии к методу, чтобы объяснить его назначение и параметры.

В блоке catch вы выбрасываете исключение снова, без всякой обработки. В зависимости от ситуации, вы можете добавить логирование или обработку ошибок.

Вот пример кода с учетом предложенных изменений:


/// <summary>
/// Generates a hash for the given URL, saves it to the database, and returns the hash.
/// </summary>
/// <param name="request">A CreateHashCommand containing the URL to hash.</param>
/// <param name="cancellationToken">A CancellationToken to observe while waiting for the task to complete.</param>
/// <returns>A Task that represents the asynchronous operation, containing the generated URL hash as a string.</returns>
public async Task<string> GenerateUrlHashAsync(CreateHashCommand request, CancellationToken cancellationToken)
{
    try
    {
        if (string.IsNullOrWhiteSpace(request.Url))
            throw new ArgumentException("Url cannot be null or empty.", nameof(request.Url));

        var urlInDb = new Url()
        {
            Created = DateTime.UtcNow,
            CreatedBy = null,
            OriginalUrl = request.Url
        };

        await _context.Urls.AddAsync(urlInDb, cancellationToken);
        await _context.SaveChangesAsync(cancellationToken);

        var urlHash = _hashgen.EncodeLong(urlInDb.Id);

        return urlHash;
    }
    catch (Exception ex)
    {
        // Add logging or error handling here.
        throw;
    }
}


Я в принципе согласен

Добавка насчет AddAsync — в данном контексте не имеет особого смысла (можно использовать Add), возможно дадут bonus point:
https://thesharperdev.com/ef-cores-addasync-v-add-method/

О еще нашел, вроде лежит на поверхности.
Какой смысл передавать весь request если из него используется только Url. Лишняя зависимость.
Не разумнее будет только Url и передать как параметр?
Отредактировано 17.03.2023 12:03 bnk . Предыдущая версия . Еще …
Отредактировано 16.03.2023 19:53 bnk . Предыдущая версия .
Отредактировано 16.03.2023 19:33 bnk . Предыдущая версия .
Re: Codereview - подскажите что в методе можно улучшить?
От: yenik  
Дата: 16.03.23 19:41
Оценка: +3
Здравствуйте, Glestwid, Вы писали:

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


Метод удалить как страшный сон, запросить требования, ознакомиться с проектом, реализовать требуемую функциональность по-людски.
Re: Codereview - подскажите что в методе можно улучшить?
От: 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒  
Дата: 16.03.23 22:14
Оценка: 15 (2) +1
- Плохой нейминг, последнее, чего ожидаешь от ToHash() — это то что он полезет в базу да еще и с апдейтом.
— если не используются nullable reference types, то request?.Url в обоих случаях а лучше var requestUrl = request?.Url и дальше юзать переменную.
— если _context EF, то последнее действие "сохранить все", соответственно предпоследнее достаточно просто обновить trackable пустем добавления в in-memory коллекцию, при правильной настройке Id будет заретрофичен после сохранения. В любом случае операции с _context нужно выносить в repository.
— выше писали про мутабельный Url — тут этого, конечно, нет, от силы плохие названия методов, хаш создается чтобы не светить Id наружу или что то в этом роде.
— если копать глубже, можно смотреть на single responsibility и сюда же idempotency метода, странновато добавлять Url без проверки на существование, нуждается в перепроверке, опять же вынос в репозиторий облегчает это.
Отредактировано 16.03.2023 22:23 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒ . Предыдущая версия .
Re: Codereview - подскажите что в методе можно улучшить?
От: Sharov Россия  
Дата: 16.03.23 23:17
Оценка:
Здравствуйте, 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>    }

G>



Помимо бредового исключения, должно быть ArgNullExp, надо бы сам request на null проверить для начала.
+ Все сказанное выше.
Кодом людям нужно помогать!
Re: Codereview - подскажите что в методе можно улучшить?
От: Sharov Россия  
Дата: 16.03.23 23:24
Оценка:
Здравствуйте, 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>


Опять же, выше хорошее замечания как про SRP. так и про идемпотентность. По уму, метод должен получть
urlInDb на вход как параметр и вызывать hashgen.EncodeLong. И все, не более.
Кодом людям нужно помогать!
Re[2]: Codereview - подскажите что в методе можно улучшить?
От: m2user  
Дата: 17.03.23 00:03
Оценка:
尿Ǥ푙>- если копать глубже, можно смотреть на single responsibility и сюда же idempotency метода, странновато добавлять Url без проверки на существование, нуждается в перепроверке, опять же вынос в репозиторий облегчает это.

Судя по типу Url в БД не url, а запросы, каждый со своим timestamp. Так что проверка на существование url вряд ли тут нужна.
Re: ненавижу такие угадайки
От: Quebecois Канада https://www.canada.ca/
Дата: 17.03.23 03:48
Оценка: +2
Здравствуйте, Glestwid, Вы писали:

Здесь можно прикопаться к:

Интервьюер, скорее всего, имеет в виду некоторое подмножество из этих пунктов, и других впридачу. Если вы назовете меньше — посчитает лентяем, больше — занудой.
Если очень хочется там работать, то можно позадавать кучу вопросов, зачем делается то, зачем это, и что хотим поймать через code review — это даст какие-никакие подсказки к содержимому чёрного ящика в голове спрашивающего, но это может не сработать.
Re[2]: Codereview - подскажите что в методе можно улучшить?
От: Glestwid  
Дата: 17.03.23 07:33
Оценка:
尿Ǥ푙>- если _context EF, то последнее действие "сохранить все", соответственно предпоследнее достаточно просто обновить trackable пустем добавления в in-memory коллекцию, при правильной настройке Id будет заретрофичен после сохранения. В любом случае операции с _context нужно выносить в repository.


Кстати,Url.Id назначается еще до SaveChanges, сразу при вызове Add(). А с двумя await что-то можно улучшить?
Re[3]: Codereview - подскажите что в методе можно улучшить?
От: 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒  
Дата: 17.03.23 10:20
Оценка:
Мы обсуждаем вещи, сути которых не знаем. Обычно "hash" — это то что решает, нужно ли что то делать или нет на основании того если что то еще уже или нет. Если этот hash не идемпотентен, то возможно одна из ветвей логики, посвященная существующему хашу в приложении, никогда не будет выполняться, мне это кажется странным вот моя логика.
Re[3]: Codereview - подскажите что в методе можно улучшить?
От: 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒  
Дата: 17.03.23 10:23
Оценка:
Как я сказал ранее, скорее всего первый await не нужен, достаточно простого добавлению в коллекцию trackable entities. Два раза базу дергать не надо.
Re[3]: Codereview - подскажите что в методе можно улучшить?
От: 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒  
Дата: 17.03.23 10:30
Оценка:
это такая игра/процесс, лучше пройти ее на берегу для обеих сторон. Предложение дефолтить значния в конструкторе немного "странные", а с датами так и прсто некорректные.
Re[4]: Codereview - подскажите что в методе можно улучшить?
От: Sharov Россия  
Дата: 17.03.23 10:36
Оценка:
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

尿Ǥ푙>Как я сказал ранее, скорее всего первый await не нужен, достаточно простого добавлению в коллекцию trackable entities. Два раза базу дергать не надо.


Он же базу дернет один раз, при save changes. Как без обращения к базе получить Id, разве просто add будет достаточно?
Кодом людям нужно помогать!
Re[2]: ненавижу такие угадайки
От: 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒  
Дата: 17.03.23 10:37
Оценка:
это такая игра/процесс, лучше пройти ее на берегу для обеих сторон. Предложение дефолтить значния в конструкторе немного "странные", а с датами так и прсто некорректные.
Re[5]: Codereview - подскажите что в методе можно улучшить?
От: 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒  
Дата: 17.03.23 10:41
Оценка:
может и так но зачем await при первом вызове _context.? они меняют постоянно все, я не уверен на 100%,

похоже, что на уровне интента, человек, который писал это, думал, что едиственным способ получить обратно identity — дернуть базу, что не так. я так это понял
Re[3]: Codereview - подскажите что в методе можно улучшить?
От: 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒  
Дата: 17.03.23 10:43
Оценка:
Как я сказал ранее, скорее всего первый await не нужен, достаточно простого добавлению в коллекцию trackable entities. Два раза базу дергать не надо.
Re[6]: Codereview - подскажите что в методе можно улучшить?
От: Sharov Россия  
Дата: 17.03.23 10:57
Оценка:
Здравствуйте, 尿컙拋㕪⬎⤇Ǥ꧃푙刾ꄔ൒, Вы писали:

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


Факт добавления в trackable entities Id не даст, Id даст бд. Без await в Id будет какой-нибудь 0.
Кодом людям нужно помогать!
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.