Рецензировал я значит сегодня PR и увидел этот код: contentType :=

"image/jpeg"
if strings.HasSuffix(strings.ToLower(fileName), ".png") {
contentType = "image/png"
}

if !strings.HasSuffix(strings.ToLower(fileName), ".jpg") &&
!strings.HasSuffix(strings.ToLower(fileName), ".png") {
return echo.ErrNotFound
}


предложил исправить и даже не поленился объяснить что к чему, написать тесты и предоставить доказательства моих опасений что этот код можно в худшем случае ускорить в 150 раз и в лучшем как минимум ~6 раз:
https://go.dev/play/p/FS5HczEV5x0

но моё предложение отвергли с объяснением: "это конечно хорошо, но мы оставим нынешний более читабельный код"

Вот сижу я и недоразумеваю. Действительно ли это настолько менее читабельно что неоправдывает эффективность? 🤔

Как вы думаете?

P.S. этот код выполняется в HTTP handler'е, т.е. это не нечто что пару раз выполнится, оно будет крутиться в каждом запросе на этот endpoint

Определять тип по расширению фу такими быть)) Я написал img.jpeg и сломал ваш код😁

у нас ещё до этого фильтр стоит, .jpeg инвариант в данном случае

Ты можешь еще раз поговорить со своими коллегами. Мол, не понял, давайте заново. Если два раза на хендлер с нагрузкой в сто разпросов в сутки, то они правы

может это возврат существуещего файла в на гет запрос, тогда логично, что проверка была при загрузке этого файла(это кстати тоже может быть необязательно), а не при чтении, нужно контекст знать, что понимать сломается что то или нет

мне кажется все 3 варианта одинакого плохо читабельны 😄 и я бы использовал http.DetectContentType вместо этих костылей. Но аргументы про аллокацию я бы тут тоже не принял. При загрузке файлов 2 аллокации даже на погрешность тайминга обработчика не повлияют

если уж упарываться, то зачем equal.Fold если вы проверяете на только ASCII строки, можно и switch переделать. Но, главное, понимать зачем. То есть получили вы вместо 250нс на эту операцию условные 25мс. Изменился ли тайминг загрузки и обработки файла?

какая в жопу читабельность? этот код невалидный на 100%

Все варианты фигня, man path.Ext

да все идет к тому что лучше юзать внешние библиотеки и тогда нельзя будет доебаться)

npm!

func EvenBetter2(fileName string) (contentType string, err error) { extension := path.Ext(fileName) switch extension { case ".jpg": return "image/jpeg", nil case ".png": return "image/png", nil default: return "", ErrNotFound } }

у этого кода не то поведение, что было в оригинальном коде

там есть тест. Этот код проходит тест

это что то значит кроме того, что покрытие неполное?

ну это значит, что нам нужно было какое-то поведение, мы тестом его задали. Я не спорю, может реально не 1 в 1 с исходным кодом, просто в тесте не отражено это

ну то есть strings.ToLower() и strings.FoldEqual по вашему просто для прикола использовали?

ну ты докопался, что я ToLower забыл? Ну хорошо, судя по исходному коду нужен он там, да

"и читабельнее всех вариантов и быстрее" если добавить ToLower то перестанет быть быстрей чем вариант с foldEqual

это вопрос культуры. Экономически вопрос не имеет значения.

кажется вы не читали ни требования ни сам код)

какие тут могут быть требования? я читал код, я бы такой код на ревью завернул вне зависимости от требований

в принципе да, можно было использовать его. Я просто знал в момент написания что ничего кроме .jpg и .png там априори быть не может

помоему path.Ext не учитывая case-insensitive

да увы, я забыл добавить upper case тест в суматохе

func EvenBetter2(fileName string) (contentType string, err error) { extension := path.Ext(fileName) extension = strings.ToLower(extension) switch extension { case ".jpg": return "image/jpeg", nil case ".png": return "image/png", nil default: return "", ErrNotFound } }

я бы с вами даже не работал если бы вы всегда так агрессивно выражались 🙂

BenchmarkCurrent-20 8306320 148.8 ns/op 72 B/op 3 allocs/op BenchmarkBetter-20 21330168 56.54 ns/op 24 B/op 1 allocs/op BenchmarkEvenBetter-20 300046702 3.993 ns/op 0 B/op 0 allocs/op BenchmarkEvenBetter2-20 124245432 9.700 ns/op 0 B/op 0 allocs/op

какой ты нежный, мальчик Томми.JPG

да пофиг на эти 4 нс, стало гораздо читабельнее, как по мне

а выражаюсь я так, потому что предъявленное вами решение очевидно очень плохое, а вы вместо того, чтобы понять, почему оно для вас неочевидно, пытаетесь его защитить

в условиях поставленных требований оно не плохое. Я просто хотел вам дать feedback того что ваша коммуникация крайне агрессивна и может оттолкнуть

работал я с этими "в условиях поставленных требований", это приводит лишь к тому, чтобы вместо того, чтобы сразу сделать нормально, мы наслаиваем костыли над костылями. Вот мой фидбек вам 😁

ваше опасение по поводу того что входные данные могут измениться и имя файла может несоответствовать содержанию я понял и знаю. Но оно в нашем случае не релевантно.

рвется всегда в том месте, где тонко