В последнее время мне пришлось прочитать довольно много кода претендентов на позицию Senior Ruby Developer, и теперь, вытащив из глаз затычки, предотвращающие вытекание, я решил поделиться теми типовыми ошибками, которые соискатель делать не имеет права в принципе. Все они напрямую скопированы из разных решений тестового задания. Задание простое, требует немного денежной арифметики, немного простой логики, немного умения спроектировать и организовать малюсенький проектик.
Итак, встречайте: пять самых частых ляпов, зажигающих перед соискателем красный свет.
Оговорюсь, что я никогда не зверствую в кодревью и требую исправить только ошибки. Советую подправить — совсем уж корявый код, и только после апрува. Захочет коллега — исправит перед мерджем, нет — нет.
№5 Class variables
Пример:
class Foo
@@env = :prod
def env; @@env end
endВ последнее время этот тип ошибки уходит в прошлое, вместе с использованием attr_accessor и прочих глобальных прелестей. Тем не менее, иногда хочется определить константу на уровне класса, и, вроде бы, переменная класса для того и была придумана. Чтобы увидеть, в чем тут проблема, надо четко различать классы и инстансы, что немного усложнено тем, что класс — сам по себе тоже инстанс (своего айгенкласса, который синглтон, хм).
Так вот, код выше определяет переменную класса на уровне айгенкласса Foo. Всего у нас есть четыре способа объявить переменную с префиксом @: переменные класса или инстанса в разных местах:
на уровне класса (в теле определения модуля или класса)
на уровне инстанса (внутри метода)
Довольно бойлерплатный кусок кода ниже показывает различия:
class Foo
@@env_class = :foo_env_class
@env_class_instance = :foo_env_class_instance
def initialize
@@env_instance_class = :foo_env_instance_class
@env_instance_instance = :foo_env_instance_instance
end
def get
{
env_class: @@env_class,
env_class_instance: @env_class_instance,
env_instance_class: @@env_instance_class,
env_instance_instance: @env_instance_instance
}
end
end
class FooChild < Foo
@@env_class = :foo_child_env_class
@env_class_instance = :foo_child_env_class_instance
def initialize
@@env_instance_class = :foo_child_env_instance_class
@env_instance_instance = :foo_child_env_instance_instance
end
endЕсли теперь сходить в консоль и глянуть, что там получилось, то результат будет вот таким:
[1] ▸ Foo.new.get
#=> {
# :env_class=>:foo_child_env_class,
# :env_class_instance=>nil,
# :env_instance_class=>:foo_env_instance_class,
# :env_instance_instance=>:foo_env_instance_instance}
[2] ▸ FooChild.new.get
#=> {
# :env_class=>:foo_child_env_class,
# :env_class_instance=>nil,
# :env_instance_class=>:foo_child_env_instance_class,
# :env_instance_instance=>:foo_child_env_instance_instance}Из примечательного тут вот что: определенные на уровне модуля переменные с одной собакой (module instance variables) — из кода методов недоступны, их увидят методы класса, определенные внутри self << class или через def self.get. Но главное, переменная класса оказалась в родителе перетерта из наследника. Это ожидаемое поведение, переменные класса так работают. Поэтому не нужно их использовать примерно никогда. Для этих целей как раз и существует переменная инстанса класса, которая пуста в коде выше; ей просто нужен правильный геттер.
№4 Класс вместо Struct (часто вместо простого Hash)
Я знаю, что руби — это настоящий ООП, там даже каждое булево значение — инстанс настоящего класса (это не совсем так, на самом деле, но разговор об этом выходит за рамки данной заметки). И тем не менее, не нужно городить классы там, где вам нужны просто структурированные данные. Ходят слухи, что из введения краткой записи рекордов вместо многословного традиционного объявления классов с геттерами и сеттерами — однажды вырос целый язык. В руби вы не сможете защитить ваши данные, если злой коллега захочет их перезаписать, не нужно себя тешить модификаторами private и прочими attr_reader.
Поэтому если нужна структура данных — объявите ее как структуру данных. А еще лучше — вообще просто как Hash. Даже, если вам завтра потребуется какая-то аггрегация, — тут все еще руби.
[1] ▸ employee = {name: 'John', lastname: 'Smith'}.extend(
Module.new { def full; [self[:name], self[:lastname]].join(?,) end}
)
[2] ▸ employee.full
#=> "John,Smith"Ладно, шучу, так делать не нужно :) Но и злоупотреблять классами там, где достаточно простого хэша, или структуры — тоже.
№3 Hash.default_proc
Это любимая ахиллесова пята всех без исключения рельсовиков (стр. 7), ||:
class User
def initialize(**input)
@raw = input
end
def name
@raw[:name] || "N/A"
end
endНе делайте так. Определите Hash#default_proc на внутреннем хэше и спокойно отдавайте его наружу (замороженным, если боитесь измен).
№2 Вычисления с плавающей точкой
Текст уже получается длинноват, поэтому по этому пункту выскажусь кратко: никогда, ни при каких обстоятельствах, не используйте флоаты там, где нельзя вернуть «примерно то же самое, что было передано». Деньги, интервалы в днях, да вообще почти все. Используйте вместо них Rational. Или любую реализацию даблов, если прям принципиальна скорость.
№1 Итерация вместо маппинга или редьюса
И, наконец, венчает наш хит-парад — самый противный код, который вообще только можно представить.
sum = 0
[1, 2, 3, 4].each do |i|
sum += i
end
sumЗдесь ужасно просто всё. Самое удивительное, что я встречаю подобное насилие над простым проходом по массиву чуть ли не в каждом втором тестовом.
объявление переменной вне скоупа вычислений
изменение переменной внешнего скоупа изнутри итератора
совершенно убийственная многословность ради ничего
введение в заблуждение читателя: тут нет итерации как таковой, тут
reduce
Вот как этот код должен выглядеть:
[1, 2, 3, 4].reduce(0, &:+)
# или, если вы совсем новичок, то
[1, 2, 3, 4].reduce(0) do |acc, i|
acc + i
endНа сегодня, пожалуй, все. А какие ваши любимые ошибки начинающего разработчика в руби?
Комментарии (97)
qvan
14.06.2023 14:43+1А что неправильного в объявлении переменной вне скоупа и изменении её? Вы внутрь reduce заглядывали, там разве не так?
cupraer Автор
14.06.2023 14:43Да, внутрь
Enumerable#reduceя заглядывал (и даже где-то рядом патчи слал). Там вот так:static VALUE enum_inject(int argc, VALUE *argv, VALUE obj) { struct MEMO *memo; VALUE init, op; rb_block_call_func *iter = inject_i; ID id; switch (rb_scan_args(argc, argv, "02", &init, &op)) { case 0: init = Qundef; break; case 1: if (rb_block_given_p()) { break; } id = rb_check_id(&init); op = id ? ID2SYM(id) : init; init = Qundef; iter = inject_op_i; break; case 2: if (rb_block_given_p()) { rb_warning("given block not used"); } id = rb_check_id(&op); if (id) op = ID2SYM(id); iter = inject_op_i; break; } if (iter == inject_op_i && SYMBOL_P(op) && RB_TYPE_P(obj, T_ARRAY) && rb_method_basic_definition_p(CLASS_OF(obj), id_each)) { return ary_inject_op(obj, init, op); } memo = MEMO_NEW(init, Qnil, op); rb_block_call(obj, id_each, 0, 0, iter, (VALUE)memo); if (memo->v1 == Qundef) return Qnil; return memo->v1; }qvan
14.06.2023 14:43+1Ну так и memo вне скоупа объявлено… Что неправильного то в этом, был вопрос.
cupraer Автор
14.06.2023 14:43Во-первых,
memoнигде вообще не объявлено. Во-вторых, неправильно в этом то, что такой код становится контекстно-зависим, и при рефакторинге его очень легко поломать.def fun array, sum = 0 # код sum = 0 # вот эту строчку можно удалить, и сразу ничего не сломается array.each { |e| sum += e } sum endС правильным вариантом на эти грабли не наступить.
ValentinAndreev
14.06.2023 14:43Struct тормознутее обычных классов, насколько я помню, хотя не знаю насколько это важно (вот OpenStruct уже не рекомендуют из-за динамического добавления полей тормозов еще больше). А зачем 0 в reduce указывать он же там по умолчанию?
cupraer Автор
14.06.2023 14:43Struct, очевидно, быстрее классов, потому что ему ни динамический диспатчинг, ниmethod_missingне нужно проверять. Это просто увидеть тривиальным бенчмарком.Ну, ноль там для большей «обобщенности» примера :)
UserAd
14.06.2023 14:43Возможно я неправильно написал бенчмарк и он показывает неправильные результаты
require 'rubygems' require 'benchmark' class Test def initialize(a, b) @a = a @b = b end def a @a end def b @b end end Test1 = Struct.new(:a, :b) cycles = 100_000_000 Benchmark.bm do |x| x.report("Class") do cycles.times do |t| cl = Test.new(t, t+1) result = cl.a + cl.b end end x.report("Struct") do cycles.times do |t| st = Test1.new(t, t+1) result = st.a + st.b end end endУ меня вышло так:
user system total real Class 26.309904 0.098226 26.408130 ( 26.427825) Struct 25.518613 0.055952 25.574565 ( 25.576957)Разница составила 3% по времени на таком размере операций, т.е. где-то 3.0e-10 секунды на операцию.
Вообще, согласно документации, Struct создает тоже класс
The Struct class generates new subclasses that hold a set of members and their values. For each member a reader and writer method is created similar to Module#attr_accessor.cupraer Автор
14.06.2023 14:43Я никогда не утверждал, что разница будет заметной. Я опровергал тезис «
Structтормознутее обычных классов» и ваш бенчмарк подтверждает мои слова.Structсоздает тоже классРазумеется, мы же в руби. Даже обычный
Hash— это класс. И что?UserAd
14.06.2023 14:43Struct, очевидно, быстрее классов, потому что ему ни динамический диспатчинг, ни method_missing не нужно проверять.
Ложное утверждение
Между ними нет разницы. Struct == Class просто с обвязкой для генерации метода initialize и ридеров и врайтеров.
Test = Struct.new(:a) do def method_missing(method) puts "Missed method #{method}" 42 end end x = Test.new(10) x.some_method_what_does_not_existscupraer Автор
14.06.2023 14:43Вы меня троллите, что ли? Разумеется, вы вправе его перегрузить, это же руби, ну. Но до перегрузки это не обычный класс, это именно специально обернутый hash.
Код же доступен: https://ruby-doc.org/core-3.1.1/Struct.html#method-c-new
… rest = rb_ident_hash_new(); RBASIC_CLEAR_CLASS(rest); OBJ_WB_UNPROTECT(rest); tbl = RHASH_TBL_RAW(rest); for (i=0; i<argc; i++) { VALUE mem = rb_to_symbol(argv[i]); if (rb_is_attrset_sym(mem)) { rb_raise(rb_eArgError, "invalid struct member: %"PRIsVALUE, mem); } if (st_insert(tbl, mem, Qtrue)) { rb_raise(rb_eArgError, "duplicate member: %"PRIsVALUE, mem); } } rest = rb_hash_keys(rest); st_clear(tbl); RBASIC_CLEAR_CLASS(rest); OBJ_FREEZE_RAW(rest);
dsh2dsh
14.06.2023 14:43+1[1, 2, 3, 4].reduce(0, &:+)Это явно хуже читается, чем предыдущий, "плохой", вариант. А код, в первую очередь, должен хорошо читаться и пониматься, при прочих равных. Компьютеру все равно, как выглядит код, а вот человеку - нет.
Вот за это мне и не нравится современный руби. Начали тащить синтаксис отовсюду и добавлять закорючки в язык. И главное, зачем, лень несколько лишних кнопок нажать что-ли. Так все равно же авто дополнение есть. А читабельность падает.
cupraer Автор
14.06.2023 14:43+2Это синтаксис притащен из перла в версии
0.1(1993 год). И он отлично читается, если хотя бы на уровне джуна уметь программировать. А тут речь про целого синьёра.dsh2dsh
14.06.2023 14:43+3Вот эта закорючка "&:+", к примеру, мной отлично не читается и требует некоторого напряжения и времени, чтобы я её осознал. А если просматривать большой код, то 100% эта закорючка пройдёт мимо внимания. А если в этом коде таких закорючка будет много, то глаза в кучку гарантированы.
t3hk0d3
14.06.2023 14:43+2Справедливости ради
&:methodэто стандартная и довольно часто используемая конструкция рубей.Вы небось на
this::methodв Джаве тоже ругаетесь?
UserAd
14.06.2023 14:43+1№3 Hash.default_proc
Это любимая ахиллесова пята всех без исключения рельсовиков (стр. 7),
||:Крайне вредный совет, на мой взгляд, в результате мы не видим что вернется в случае nil на месте, а должны бегать по проекту и искать где этот хеш был определен и смотреть где определена его default_proc, а еще она может быть переопределена или состоять из километровой портянки switch(key)
cupraer Автор
14.06.2023 14:43О, да! А 10 разных рассинхронизированных дефолтов, разбросанных по коду, — гораздо лучше использования стандартных продуманных средств языка.
UserAd
14.06.2023 14:43Вот простой код, представим что классы разбросаны по проекту. Что мы получим в результате?
На первый взгляд 10 и "a"some_hash = {a: 1, b: 2} class A def initialize(hash) @hash = hash @hash.default_proc = proc { |hash, key| 10 } end def hash @hash end end class B < A def initialize(hash) super @hash.default_proc = proc { |hash, key| "a" } end end a = A.new(some_hash) b = B.new(some_hash) puts a.hash[:empty] puts b.hash[:empty]А если поменяем местами
a = A.new(some_hash) b = B.new(some_hash)Категоричность суждений, в данном случае, ведет к радостям отладки.
Wesha
14.06.2023 14:43+1О да!!
Почему-то люди постоянно забывают, что "передача объекта" — это под капотом на самом деле передача указателя на объект — а если в вызываемом методе начать менять содержимое объекта, то там можно таких сюрпризов для вызывающего натворить...
cupraer Автор
14.06.2023 14:43В этом конкретном коде такое количество WTF, что я бы даже джуна, который такое понаписал, уволил одним днем.
Если говорить только про
default_proc, здесь сразу три проблемы:её никто никогда не устанавливает из методов на инстансе
её никто никогда не устанавливает перезатирая существующее значение (стратегии мерджа надо обсуждать по месту)
её никто никогда не устанавливает на объектах, не принадлежащих callee
UserAd
14.06.2023 14:43+1Из моего опыта собеседований довольно много кто зазубрил всякие вещи и пишет очень метапрограммный код, но допускает в простейших вещах с AR такие перлы:
def update @counter = Counter.find params[:id] @counter.clicks += 1 @counter.save endА когда спрашиваешь где тут может быть проблема пишут код типа такого:
def update Counter.transaction do @counter = Counter.find params[:id] @counter.clicks += 1 @counter.save end endНа мой взгляд увлечение метапрограммированием зачастую приводит к очень красивому коду который невозможно понять не потратив на это космическое количество времени.
Везде надо соблюдать баланс. Код некрасивый с хешами и прочим, но обложеный тестами гораздо лучше метапроизведения искусства в которое надо верить и которое обложить тестами практически невозможно.
cupraer Автор
14.06.2023 14:43Скорее всего, дело в разнице профилей: я не пишу прикладной код, я пишу библиотеки, при помощи которых команда пишет прикладной код. (Оговорюсь сразу: я отвечал метапрограммированием на вопрос «почему я выбрал руби», и это не имеет никакого отношения к оригинальной теме собеседований: я не жду от соискателей никакого метапрограммирования).
Так вот, имею сообщить: AR — это примерно на 102% метапрограммирование (среднего качества, да, вот вы выше прекрасный пример привели, почему именно). И то, что вы задаете такие вопросы на собеседованиях — прямое следствие сильнейших возможностей языка в этом плане, без них просто никаких рельсов бы не было.
Кроме того, это ложная дихотомия. Можно увидеть гонку в коде выше и все еще хорошо владеть метарпограммированием.
Wesha
14.06.2023 14:43+1допускает в простейших вещах с AR такие перлы
А что конкретно Вам тут не нравится? Я не к тому, что мне здесь всё нравится, а к тому, что есть немалое количество мелочей, и в завимости от того, чего Вы хотели, могут потребоваться те или иные решения.
Проблема №1
Судя по тому, что идёт обращение к
params, этот код должен находиться в контроллере, потому как именноApplicationController (или ActionController::Base) определяетparams. Внутри модели этот код без дополнительных телодвижений работать не будет.Проблема №2
Нет никакой валидации входных параметров. Что если нам передали хэш, в котором нет ключа
:id?Проблема №3
Нет обработки аварийных ситуаций. Если
Counterс таким id не существует, код упадёт. Это некузяво.Проблема №4
Использовать переменную экземпляра (
@counter), если в этом нет необходимости, некузяво. Вместо этого лучше было использовать локальную переменнуюcounter.Проблема №5
Имеет место race condition. Мы уже установили, что код вызывается из контроллера — следовательно, мы имеем дело с сервером, который обслуживает многих юзеров. Если более одного юзера привели к вызову метода
updateс одним и тем жеidв одно и то же время, и при этом таблица counters была создана без колонкиlock_version, то возможна ситуация, когда clicks увеличится только на 1 (вместо ожидаемого 2), а если c колонкойlock_version, то код упадёт по Stale ObjectПроблема №6
Обёртка в транзакцию проблемы race condition не решает. Транзакция гарантирует, что группа операций записи либо целиком выпонится, либо целиком не выполнится. Однако здесь операция записи в базу всего одна — так что обёрнуто ли оно в транзакцию, не обёрнуто ли — ничего не изменится.
Чтобы оно работало нормально,
операция инкремента должна быть атомарной на уровне базы.
def update Counter.where(id: params[:id]).update_all("clicks = ISNULL(clicks, 0) + 1") endcupraer Автор
14.06.2023 14:43Все по делу, но
Counter.where(id: params[:id])— это для читателя такого кода прям гигантский WTF :)А также: использовать
AR, не гнушаясь писать бд-зависимый код, — дурной тон.IFработает в любом RDBMS из мне известных, и не намного прям длиннее.Wesha
14.06.2023 14:43Во-первых, смысл AR именно в том, чтобы максимально скрыть зависимости от конкретной базы.
Во-вторых, движение в сторону использования AR-обёрток
.where(...)вместо чистого SQL (clicks = (IF clicks IS NULL THEN 0 ELSE clicks END) + 1) идёт уже не менее чем пяток лет. Следим за тенденциями-с!В-третьих, положа руку на сердце: за вот уже 10 лет существования проекта мы поменяли движок базы ровно 0 (ноль) раз. Зачем закладываться на бд-независимый код, если скрипач по факту не нужен?
cupraer Автор
14.06.2023 14:43смысл AR именно в том, чтобы максимально скрыть зависимости от конкретной базы
Спасибо, буду знать.
движение в сторону использования AR-обёрток
Я про
ISNULLкоторого за пределами MSSQL не существует.скрипач по факту не нужен
Ну так я и сказал: «дурной тон», а не «ловите джуна» ведь :)) Не нужен, не нужен, тут согласен.
Wesha
14.06.2023 14:43Спасибо, буду знать.
Сарказм детектед. Я знаю, что Вы знаете. Но Вы забываете первое правило споров в интернете: мы с Вами тут не одни, на нас люди смотрят. Соответственно, я текст пишу не столько для лично Вас, сколько для зевак — которые как раз могут и не знать.
cupraer Автор
14.06.2023 14:43Да ладно, уж прям не саркастнуть к месту. Я же по-доброму.
Wesha
14.06.2023 14:43По-доброму флаг /s используют, чтобы компенсировать известную ошибку восприятия.
cupraer Автор
14.06.2023 14:43А когда шутят в курилке, громко объявляют: «Внимание, шутка!»?
Я не стремлюсь всем понравиться, а уж тем, кто на беззлобные подколки обижается — и подавно. Расставлять дебильные смайлики и закрывать предложения скобочками, чтобы собеседник не подумал лишнего — это не ко мне, я способен писать внятно без суржика.
ValentinAndreev
14.06.2023 14:43Мне тут отсутствие отработки ошибок (ну и save! вместо save) в голову пришло, ну или update поля, переменная вместо переменной класса - лучше, но никак не проблема.
cupraer Автор
14.06.2023 14:43переменная вместо переменной класса — лучше, но никак не проблема
Эм. Во-первых, это переменная инстанса (aka instance variable), а не класса.
Во-вторых, это потоконебезопасно.
Wesha
14.06.2023 14:43Если бы тут была не переменная инстанса, а переменная класса — то за такое сразу ссанымми тряпками за сто первый километр.
cupraer Автор
14.06.2023 14:43Я хотел было рассказать байку про переменную класса в первом примере, но это было бы в тексте лишнее, так что расскажу тут.
В общем, у меня было три гештальта в руби по части залитовать в продакшн.
Первый уже не помню какой, что-то в метапрограммировании (кажется, написать логику на отличии
__callee__от__method__, примерно, какgitсо своим бинарником делает), я его довольно быстро закрыл. Потом я полгода искал удобный случай протащить в прод flip-flop — получилось и это, в парсере чужого мохнатого формата из 90-х. А вот использовать переменную класса с пользой — года три надо мной дамокловым мечом висело.И вот, была у меня некая система плагинов, в которой было позднее связывание. Voilà — роутинг уехал в
@@routing, а я с тех пор на руби почти не пишу :)Wesha
14.06.2023 14:43А я вот уже лет эдак двадцать как осознал, что я работаю не один, а в команде (где далеко не все — гении), и времена, когда я мог писать идеально работающий код, который никто, кроме меня, понять не может, прошли. В результате родилось two second rule: "если код не может быть понят читающим за две секунды, он должен быть переписан".
К сожалению, все Ваши гештальты это правило не проходят. Так что в личном проекте они имеют право на существование, а в рабочем
нафиг с пляжа— извините, you are not a team player, Вы нам не подходите.cupraer Автор
14.06.2023 14:43Да я и работу-то в принципе не ищу, и с тезисом абсолютно согласен.
Просто у нас процентов 10 продакшн кода — мои личные проекты. OSS. Как `sidekiq`, только фича реквесты быстрее в работу принимаются.
В общую кодовую базу я, разумеется, такое не потащу, даже странно объяснять.
ValentinAndreev
14.06.2023 14:43Я что-то привык, что говоря "переменная класса" это ясно что переменная экземпляра класса. Именно переменную класса я видел только один раз в реализации синглтона (хз зачем). А по поводу потокобезопасности, что-то не встречал, чтобы именно в таких случах как-то заботились об этом, это видимо очень редко всплывающая проблема.
Wesha
14.06.2023 14:43Мне тут отсутствие отработки ошибок (ну и save! вместо save) в голову пришло
Отличие
save!в том, что оно вызывает валидаторы и падает, если валидаторы не прошли. Но в данном случае объект достаётся из базы (то есть когда его туда записывали, валидаторы уже прошли — иначе его бы в базе не было), сейчас изменился только счётчик — на что его валидировать-то? В данном конкретном случаеsaveиsave!по факту будут работать абсолютно одинаково в 99,99999999% случаев.Про Stale Object Error я уже сказал ("Проблема №5"), про потенциальную возможность отсутствия записи с таким ID тоже ("Проблема №3"), а других потенциальных ошибок при работе с базой (ну, кроме как "база ВНЕЗАПНО стала недоступна" и т.п., но в таких случаях сервер уже может заворачиваться в белую простыню и далее по тексту) здесь я не вижу — в коде, выполняющем единственное действие, много ошибок совершить и правда сложно.
vk_26
14.06.2023 14:43странное решение. Для такого тривиального случая очень неудачное решение, читается плохо. Почему бы не использовать обычный лок?
def update counter = Counter.find(params[:id]) counter.with_lock do counter.increment!(:clicks) end endcupraer Автор
14.06.2023 14:43Например потому, что решение выше работает всегда, а ваше — лишь иногда, по крайней мере, в том виде, как вы его представили.
Подсказка: этот метод может быть вызван в конкурентной среде в двух разных потоках одновременно.
vk_26
14.06.2023 14:43почему мое решение не будет работать в конкурентной среде? Используется лок. На уровне БД будет блокировка `SELECT FOR UPDATE`.
Если два одновременно потока/запроса будут выполняться, то один из запросов повесит лок на строку, а второй увидев блокировку, будет ждать снятия блокировки. Как только первый запрос выполнится, он снимет блокировку. И после этого выполнится второй запрос. И это обеспечивается СУБД. Почему это не будет работать в конкурентной среде?cupraer Автор
14.06.2023 14:43Я знаю, как работает
with_lock. В отличие от кода выше, он стартует транзакцию, а ресурсы в любом приложении конечны, к сожалению. И — рано или поздно — вы увидите вот такую неприятную штуку:Mysql2::Error: Lock wait timeout exceeded; try restarting transactionvk_26
14.06.2023 14:43Т.е. все-таки получается мы приходим к тому, что локи могут помочь с race condition. Или нет? ))
Т.е. по вашей логике раз мы когда-то гипотетически можем упереть в таймаут по локам, то мы будем сразу отказывать от этого решения, и будем за место этого применять неидеоматичные костыли? отказываться от решения, которое как раз задумано для такого типа задач )
Конкретно в нашем примере еще нужно очень постараться добраться, чтобы начать ловить ошибки по таймату. У нас в примере короткая быстрая транзакция, в которой только один апдейт одной записи. Из своего опыта, словит ошибки таймаута по локу происходит в кейсах, когда в одной транзакции несколько тяжелых операций и там висит лок. А у нас апдейт только одной записи в транзакции. Если посыпались такие ошибки на коротких транзакциях, то это говорит либо, что настройки у БД неоптимальны, либо серьезно возросла нагрузка. Настолько, что уже и без локов постгресу/mysql поплохеет. Но в этом случае нужно уже по-другому решать вопрос.cupraer Автор
14.06.2023 14:43Таймаут на лок происходит при попытке получить лок, иными словами от длины и тяжести транзакции не зависит вообще никак.
неидеоматичные костыли
ActiveRecord::Relation#update_allв данной ситуации — это идиоматичное правильное решение (а лок — костыль). Первое не позовет колбэки, не станет валидировать все поля (а транзакция — внезапно — станет), и является атомарным апдейтом на уровне базы (о чем сказал предыдущий оратор).То, что добрая половина рельсов идиоматически предлагает делать абсолютно идиотские вещи — вовсе не означает, что не нужно все делать правильно, хотя бы, когда возможно. Напомню, что идиоматический для рельсов выбор из таблицы один-ко-многим — до четвертой, кажется, версии, — порождал печально известную проблему N+1. Зато идиоматически.
vk_26
14.06.2023 14:43на счет валидации - тут да, она не нужна в данном примере, и это плюс.
Атомарный апдейт - как он поможет с гонками?
Wesha
14.06.2023 14:43мы когда-то гипотетически можем упереть в таймаут
"Если какая-то неприятность может случиться — она случается" (c)
Wesha
14.06.2023 14:43Почему бы не использовать обычный лок?
Потому что рельсам — рельсово, а базе — базово. Почему за локами, транзакционностью и проч. должен следить rails со своими костылями и велосипедами, если база делает это нативно?
Martin_mad
14.06.2023 14:43Специально зарегистрировался что бы оставить комментарий)
Почему нельзя чз transaction(isolation: serializable) просто сделать? На постгресе неплохо работает
cupraer Автор
14.06.2023 14:43Ну я же ответил буквально тремя комментариями выше, почему плодить больше транзакций богу транзакций там, где можно обойтись атомарным апдейтом — очень плохая практика.
База не резиновая, транзакция — тяжела (в сравнении с инструкцией), валидировать всю запись а потом вызывать все колбэки — лишний оверхед.
vk_26
14.06.2023 14:43так БД сама по себе не обеспечивает гарантию предотвращения например lost update, как в нашем примере - при конкурентных запросах могут потеряться клики. Вернее БД может это делать, но нужно переключить уровень изоляции на serializable, но тогда БД будет сильно тормозить при сколько-нибудь существенной нагрузке.
Тоже самое с транзакциями - ты сам должен оборачивать в транзакции последовательность операций, которые нужно выполнить атомарно.
UserAd
14.06.2023 14:43Еще есть метод icrement
cupraer Автор
14.06.2023 14:43Чуть выше предлагали инкремент с локом, и я довольно подробно рассказал, почему так не нужно делать тоже.
UserAd
14.06.2023 14:43Так inrement как раз сгенерирует SQL запрос который предлагали выше. Лока в данном случае не будет
cupraer Автор
14.06.2023 14:43А, вы имеете в виду вне лока? Да, можно. Только не increment, который вообще в базу не ходит, и даже не increment!, который просто писал какой-то пьяный медведь, судя по коду, и в котором все еще есть гонка, а кошерный update_counters, который, действительно, выполнит атомарный
UPDATE.UserAd
14.06.2023 14:43increment! использует update_counters и там нет гонки, просто он учитывает все increment которые были до сохранения.
А применять уничижительные сравнения к незнакомым людям некультурно.
cupraer Автор
14.06.2023 14:43Вам еще не надоело спорить на ровном месте? Вот гонка во всей красе.
change = public_send(attribute) - (public_send(:"#{attribute}_in_database") || 0) self.class.update_counters(id, attribute => change, touch: touch)применять уничижительные сравнения к незнакомым людям некультурно
С чего это вы взяли, что к незнакомым? Сообщество довольно тесное, тут же не джава.
UserAd
14.06.2023 14:43Хорошо, вот код:
object = Counter.where(id: 10).first!Мы делаем запрос к базе, получаем object.clicks = 0, object.clicks_in_database = 0
Делаем increment, получаем clicks = 1, clicks_in_database = 0
Делаем increment!, получаем clicks = 2, clicks_in_databse = 0, вызываем update_counters с параметрами 10, {:clicks => (2 - 0), touch: true}, который делает запрос с UPDATE ... SET clicks = clicks + 2 ...
Единственная гонка которая может тут быть это из-за touch на тему последнего обновления.cupraer Автор
14.06.2023 14:43Контроллер 1 достал значение, потом контроллер 2 достал значение, потом контроллер 2 обновил значение, потом контроллер один обновил ... Упс!
Или у вас нагрузка 1 реквест в час?
UserAd
14.06.2023 14:43Контроллер 1 послал значение в increment_counters +1 через increment!
Контроллер 2 послал значение в increment_counters +1 через increment!Вот только что написал в тестовом проекте такой контроллер и сгенерировал нагрузку через siege.
Вообще мы очень сильно отвлеклись от темы статьи, на мой взгляд, тут стоит придерживаться простых принципов "как не надо писать на ":
Заумно и непонятно - новый человек должен понимать что происходит быстро
Размазывая логику - не стоит заставлять бегать по 1000 файлам для понимания того что вернется
Напирая до абсолюта на какие-либо принципы (DRY в худшем варианте, например)
Используя конструкции и подходы не являющиеся частью языка. (см. пункт 1)
vk_26
14.06.2023 14:43а почему вы взяли, что гонки не будет? Это про решение выше с where и про это? атомарность - это вообще не про это. Атомарность - либо выполнится все, либо ничего. А для гонок - как раз механизм локов, либо оптимистическая блокировка, где разруливать на уровне приложения надо. Ну либо включать serializable в БД, но тогда БД будет еле волочиться.
cupraer Автор
14.06.2023 14:43Потому что я умею читать руби код по ссылке.
Еще раз: один вызов `UPDATE` в базе: либо выполнится, либо нет.
vk_26
14.06.2023 14:43при чем тут руби код? Без локов клики могут потеряться при конкурентных вызовах запросов, даже если написать запросы на чистом sql
cupraer Автор
14.06.2023 14:43Нет, не могут. Пожалуйста, подумайте, это не сложно.
vk_26
14.06.2023 14:43ну вы аргументируйте, а не отвечайте типа сам дурак.
Почитайте про аномалию lost update в БД при конкурентных апдетах. И как ее решать.
cupraer Автор
14.06.2023 14:43Lost update тут вообще ни при чем, он не может произойти на атомарном апдейте.
Вот сиквел, который генерируют рельсы для
update_counters:
Все это я раз тридцать повторил выше. Я не понимаю, как можно еще более лучше аргументировать, извините. А самое главное, я не очень понимаю, зачем бы мне это было нужно: мне совершенно все равно, поймете вы, или нет.
vk_26
14.06.2023 14:43я спрашивал, почему вы считаете, что атомарный апдейт дает гарантию предотвращения lost update?
cupraer Автор
14.06.2023 14:43Во-первых, вы этого никогда не спрашивали.
Во-вторых, вы мне там рекомендовали что-то почитать — ну вот и почитайте. Вам дали отправную точку — копайте, если интересно. Если нет — ну считайте, что вы правы, мне-то что?
vk_26
14.06.2023 14:43до этого вы утверждали, что решение на локах не будет корректно работать в конкурентной среде:
Например потому, что решение выше работает всегда, а ваше — лишь иногда, по крайней мере, в том виде, как вы его представили.
Подсказка: этот метод может быть вызван в конкурентной среде в двух разных потоках одновременно.
так что я думаю, если отбросить эмоции и детское "сам дурак, а я умный", то и для вас эта дискуссия была полезная и вы узнали что-то новое )
Wesha
14.06.2023 14:43ИЧСХ, на заданный мной вопрос "А что конкретно Вам тут не нравится?" так до сих пор никто и не ответил.
cupraer Автор
14.06.2023 14:43И да, решение на локах работает не всегда, даже перечитывание треда поможет это при желании понять.
Транзакции — вообще жуткий костыль, который сначала приводит к lost updates, а потом мы с ним героически сражаемся локами, которые — еще больший костыль.
При этом, я уже лет 20 в глаза не видел сиквел, и лет пять не пишу на руби. Перерос.
Wesha
14.06.2023 14:43И да, решение на локах работает не всегда
А я так сразу и написал — "в завимости от того, чего Вы хотели, могут потребоваться те или иные решения". Есть где развернуться.
UserAd
14.06.2023 14:43и лет пять не пишу на руби. Перерос.
А статья тогда это фантомная боль?
Я вот лет 10 не пишу на РНР, но что-то меня не тянет смотреть РНР код и говорить "вы все делаете неправильно"
cupraer Автор
14.06.2023 14:43Я не говорю «вы все делаете неправильно», я дал несколько советов людям, способным учиться.
Я не виноват в том, что в индустрии так много некомпетентных людей, мнящих себя синьёрами.
А еще я продолжаю поддерживать свои OSS библиотеки и (как написано в первом предложении) проверяю тестовые задания в нашей компании.
read_from_left_to_right
14.06.2023 14:43Начал читать с мыслью, что узнаю как не опозориться на собесе и узнал, всего то надо не вести себя как автор статьи. А именно грубить, чувствовать себе экспертом который перерос руби и не пишет на нем 5 лет, но считает себя лучше синьоров которые пишут и читают тысячи строк рубишного кода в неделю. Использование
**kwargsвместоharg = {}, и это молчу что и то и другое является антипатерном и лучше явно указывать что принимает метод,экспоуз всей внутренней структуры только потому что лень написатьh.fetch(:key, "default_value")и это только по одному примеру.При этом, я уже лет 20 в глаза не видел сиквел
лучше бы и дальше не видели, чем вот такое писать
CASE COUNT(DISTINCT(?->>'currency')) WHEN 0 THEN JSON_BUILD_OBJECT('currency', NULL, 'amount', 0) WHEN 1 THEN JSON_BUILD_OBJECT('currency', MAX(?->>'currency'), 'amount', SUM((?->>'amount')::int)) ELSE NULL ENDcupraer Автор
14.06.2023 14:43Почему не подходит fetch, я тут раза три в комментариях рассказал. (**kwargs) — устоявшийся в коммьюнити способ показать в примере, что принимает функция, без дополнительных комментариев (антипаттерн, лолшта).
Можно читать миллион строк кода в день, и все равно не понимать очевидных вещей.
Ну и да, про сиквел вы просто задачу не осилили осознать, так бывает.
read_from_left_to_right
14.06.2023 14:43Почему не подходит fetch, я тут раза три в комментариях рассказал.
Не вижу где, по слову fetch на странице только 3 комментария, включая этот.
устоявшийся в коммьюнити способ
Комьюнити elixir? Вот им и расказывается что и как не далать, а отойти от разработки на руби 5 лет, а потом начать всех учить как не стоит писать на руби - смешно.
Ну и да, про сиквел вы просто задачу не осилили осознать, так бывает.
Просто ткнул вас носом в ваше вранье про 20 лет без сиквела, так тоже бывает и, по все видимости, довольно часто.
cupraer Автор
14.06.2023 14:43по слову
fetchHash#fetch без блока делает (исходники же доступны, почему так сложно туда заглянуть?) ровно то же самое, что
||if (hash_stlike_lookup(hash, key, &val)) { return (VALUE)val; } else { if (block_given) { return rb_yield(key); } else if (argc == 1) { VALUE desc = rb_protect(rb_inspect, key, 0); if (NIL_P(desc)) { desc = rb_any_to_s(key); } desc = rb_str_ellipsize(desc, 65); rb_key_err_raise(rb_sprintf("key not found: %"PRIsVALUE, desc), hash, key); } else { return argv[1]; // СМОТРЕТЬ ВОТ СЮДА } }Впрочем, мне не сложно повторить:
fetchопределяет дефолтное значение в месте вызова, что не лезет ни в какие рамки: тут5, тут —:ok, а вон там — вообще забыли.Комьюнити elixir?
Нет, руби.
Вот им и расказывается что и как не далать
Что, и кому мне рассказывать, я разберусь без советов говнокодеров, спасибо.
а отойти от разработки на руби 5 лет, а потом начать всех учить как не стоит писать на руби - смешно.
Да нет, как показывают буквально все комментарии под этий заметкой, — кроме Wesha тут нет ни одного человека, который мог бы хотя бы на лычку «мидл» претендовать.
ткнул вас носом в ваше вранье про 20 лет без сиквела
Чего? Вы там берега не попутали? Если вы что-то куда-то и ткнули — так это только себя носом в дерьмо.
read_from_left_to_right
14.06.2023 14:43Hash#fetch без блока делает (исходники же доступны, почему так сложно туда заглянуть?) ровно то же самое, что
||совсем не то же самое, какой смысл скидывать исходники если вы их сами не читаете или не умеете читать, fetch отработает только в том случае если в хэше не найден ключ, в то время как || отработает когда lookup вернул falsy value, если для вас этот одно и то же, то почитайте учебники по логике, параграф про тождество.
Впрочем, мне не сложно повторить:
fetchопределяет дефолтное значение в месте вызова, что не лезет ни в какие рамки: тут5, тут —:ok, а вон там — вообще забыли.Так в том и дело, мы явно указываем, что получить и когда, так как значения могут быть разных типов, строки, числа, массивы и в этом случае нужно городить в default_proc кучу условий, проверяя какой ключ вызван и подставлять дефолтное значение, я уже молчу о том что fetch позволяет строго подходить к lookup-ам и рейзить ошибку, что бывает очень полезно в процессе разработки, исключая возможность опечаток.
который мог бы хотя бы на лычку «мидл» претендовать
Вы слишком о много о себе возомнили, ваши знания нерелевантны, ваша оценка тоже никому не нужна, подтверждением тому служит что все достойные синьоры уже не ходят к вам на собесы, остались только те кто использует class variable.
outlingo
Крайне сомнительный совет. Если у вас есть некоторая сущность с вполне себе определенными полями, её практически однозначно нужно выводить в класс, хотя бы ради того, чтобы потом не пытаться понять "а какие именно ключи содержатся в этом хэше в этом месте".
Задача хэша - именно хранить хэш-таблицу "ключ-значение" а не упорядоченную структуру фиксированного типа, поэтому если где-то для хранения экземпляра доменной модели используется хэш, это сразу сигнал того, что вероятно тут где-то откровенный мисдизайн
Ну и логичное следствие, что хранить мапинги типа логин -> User в хэше - нормально и правильно. А за хранить поля юзера в хэш-таблице как в примере - сразу бить по рукам.
P.S.: структура как "чуть более организованый хэш" тоже не слишком хорошая идея.
cupraer Автор
Ну тут ваше слово против моего. По крайней мере, пока руби не научится деструктурировать инстансы классов в аргументах функции.
Классы не нужны в принципе, а тут — особенно.
teemour
рукалицо против вашего слова
Wesha
Коллега, я вижу, ещё юн — он не был в тех кругах ада, которыми проходил я...
UserAd
Если вам не нравятся классы, то зачем вы используете язык который основан на концепции ООП?
cupraer Автор
Разработка не укладывается в бинарную концепцию ООП/ФП (более того, уже лет 20 даже само это разделение — искусственно).
Руби силен метапрограммированием (до появления Эликсира — самый крутой в этом плане язык), гибкостью синтаксиса, изящной адаптацией функционального подхода, лямбдами, наконец. А классы — ну вынужденное зло, Матц на хайпе в 90-е погорячился, с кем не бывает.
Wesha
Здесь есть ещё другая опасность: опечатки. Например, человек написпал
h[:iteratpr] = some_value, а потом удивляется, почему в соседнем методеh[:iterator]возвращаетnil, хотя "вот же оно, я ж его только что присвоил!!!" В случае класса оно сразу заорёт, что "нет такого метода".cupraer Автор
Человек не может захотеть вызвать
Hash#[]=по очевидным причинам (это метод, а я вроде ведь почти русским по белому написал: «структура данных»).Да, если нужны вызовы методов на инстансах, придется терпеть класс.
Wesha
В руби "класссы" — это чуть менее, чем всё. Поэтому давайте определение, что Вы понимаете под словом "структура данных".
cupraer Автор
Ну ок, я имел в виду, как, мне кажется, довольно понятно из контекста, структурированные данные. «Структура данных» — формулировка неаккуратная, согласен, но первым в тексте встречается именно определение «структурированные данные».
A-TA-TA
Думаю, что здесь автор имеет в виду какой-то конкретный пример, в котором данный совет очевидно полезен.
Проблема в том, что экстраполяция этого совета на общие случаи выглядит притянутой за уши без конкретных примеров и формализованных правил, когда хэш должен мутировать в класс.
cupraer Автор
Да правило проще некуда: если нет действий, которые умеет выполнять только инстанс, а есть просто набор данных — класс не нужен.