10 ошибок в Ruby / Ruby on Rails, которые легко исправить

Original author: Adam Niedzielski
  • Translation
Программисты делают ошибки в коде. Некоторые просто раздражают (ошибки, не программисты – прим. переводчика), другие могут быть действительно опасными для приложения.
Представляю вам свою подборку из 10 распространенных ошибок разработчиков Ruby/RoR и советов о том, как их избегать. Надеюсь, они помогут сократить вам время отладки кода.

1. Двойное отрицание и сложные условия.


if !user.nil?
  # ...
end

unless user.blank?
  # ...
end

unless user.active? || address.confirmed?
  # ...
end

Двойные отрицания сложны для чтения. Каждый раз я трачу пару секунд для того, чтобы распарсить условие. Используйте Rails API и пишите user.present? вместо !user.blank?.

Я также редко считаю применение unless оправданным, особенно с множественными условиями. Как быстро вы поймете, в каком случае данное условие будет выполнено?

unless user.active? || address.confirmed?
...
end


2. Использование save вместо save! без проверки возвращаемого значения.


user = User.new
user.name = "John"
user.save

В чем проблема данного кода? Вы не узнаете об ошибке сохранения, если она произойдет. Ни единого следа не останется в ваших логах, и вы потратите уйму времени, пытаясь разобраться: «Почему же в базе данных нет пользоветелей?».
Если вы уверены, что данные всегда корректны и запись должна сохраняться – используйте bang методы (save!, create! и т.д.). Используйте save, только если вы проверяете возвращаемое значение:

if user.save
...
else
...
end


3. Использование self без необходимости.


class User
  attr_accessor :first_name
  attr_accessor :last_name

  def display_name
    "#{self.first_name} #{self.last_name}"
  end
end

В случае self.first_name достаточно написать first_name, и результат не изменится. Но это скорей вопрос стиля, и никаких негативных последствий за собой не несет. Однако помните, что self необходимо использовать при присваивании значений.

self.first_name = "John"


4. Проблема N+1 запроса.


posts = Post.limit(10)
posts.each do |post|
   do_smth post.user
end

Если вы посмотрите в логи при выполнении данного кода, то увидите, что выполняется 11 запросов к базе данных: один на выборку постов и по одному на каждого пользователя. Рельсы не в состоянии предугадать ваши желания и сделать запрос на выгрузку сразу 10 пользователей (возможно, даже вместе с постами). Для того, чтобы направить Рельсы в нужную сторону, можно использовать метод includes (подробней здесь).

5. Булево поле с тремя значениями.


Предполагается, что булево поле может иметь только два значения – true или false, не так ли? А как насчет nil? Если вы забыли указать значение по умолчанию и добавить опцию null: false в вашей миграции, вам придется иметь дело с тремя булевыми значениями – true, false and nil. В результате имеем подобный код:

# пост новый, не опубликован и не неопубликован
if post.published.nil?
  # ...
end

# пост опубликован
if post.published
  # ...
end

# пост неопубликован
unless post.published
  # ...
end

Если вам нужно обрабатывать три различных значения – используйте текстовое поле.

6. Потерянные записи после удаления.


Рассмотрим простой пример:

class User < ActiveRecord::Base
  has_many :posts
end

class Post < ActiveRecord::Base
  belongs_to :user
  validates_presence_of :user
end

Если мы удалим пользователя, то связанные с ним посты перестанут быть корректными (что, скорей всего, приведет к ошибкам в приложении). В этом случае необходимо обрабатывать удаление связанных объектов:

class User < ActiveRecord::Base
  has_many :posts, dependent: :delete
end


7. Использование кода приложения в миграциях.


Предположим, что у вас есть следующая модель:

class User < ActiveRecord::Base
  ACTIVE = "after_registration"
end

и вы хотите добавить в нее поле points (очки). Для этого вы создаете миграцию. Вы также хотите установить значение поля points для существующих пользователей равным 10. Для этого вы пишите в миграции:

User.where(status: User::ACTIVE).update_all(points: 10)

Запускаете миграцию, и — Ура! — все сработало: существующие пользователи имеют по 10 очков. Время идет, код меняется, и вот вы решаете удалить константу User::ACTIVE. С этого момента ваши миграции не работают, вы не можете запустить миграции с нуля (вы получите ошибку uninitialized constant User::ACTIVE).

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

Примечание переводчика:
На мой взгляд, гораздо удобнее использовать временные миграции для изменения значений. Особенно при выкатке изменений на production: вероятность забыть запустить rake отнюдь не равна нулю, а миграции за вас запустит Capistrano.
А чистить временные миграции можно после релиза.

8. Вездесущий puts.


Оставшийся после отладки puts засоряет логи и вывод во время прогона тестов. Используйте Rails.logger.debug, который позволит фильтровать логи в зависимости от выбранного уровня.

Примечение переводчика:

А лучше научитесь пользоваться дебаггером. При этом не забывайте удалять binding.pry перед коммитом.

9. Забываем про map.


Я часто вижу подобный код:

users = []
posts.each do |post|
  users << post.user
end

Это как раз тот случай, когда нужно использовать map: лаконично и идиоматично.

users = posts.map do |post|
  post.user
end

# а я бы написал так (прим. переводчика)
users = posts.map(&:user)


10. Не используем Hash#fetch.


name = params[:user][:name]

Что здесь не так? Данный код вызовет исключение (NoMethodError: undefined method `[]' for nil:NilClass), если в хэше не будет ключа user. Если вы рассчитываете на то, что ключ должен быть хэше, используйте Hash#fetch:

name = params.fetch(:user)[:name]

Тогда вы получите более осмысленное исключение:

KeyError: key not found: :user


Вместо заключения.


А какие у вас любимые ошибки?
AdBlock has stolen the banner, but banners are not teeth — they will be back

More
Ads

Comments 38

    0
    Ошибками это назвать тяжело. Скорее тонкости использования языка и фреймворка, а их, в зависимости от масштаба проекта, ну очень много может возникнуть…
      0
      Даже с точки зрения новичка в рельсах, пункты 2, 4, 6, 8 мне однозначно кажутся ошибками.
        0
        Я, в общем, далек от Ruby, но предложенный автором вариант в пункте 2, кажется тоже не слишком хорошим. Разве в Ruby нет механизма исключений?
          +2
          Вообще, такой вариант считается хорошей практикой, даже scaffold вроде так генерит. Это же не исключение, что пользователь, например, ввёл некорректный email. Надо просто отрендерить опять форму, но уже с показанными ошибками.
            0
            Почему не исключение? Если мы уже пишем, то некорректные данные должны вызывать исключение. Мы должны были их проверить до записи
              0
              save обычно используется вместе с if, и по else идет рендер страницы с ошибками:

              if user.save
                redirect_to user_path(user)
              else
                render 'edit'
              end
              
              # или так:
              user.save
              if user.errors.any?
              # ...
              


              save! в этом случае покажет пользователю страницу ошибки.

              Лучше этот совет изменить, я думаю: если вы не обрабатываете ошибку (вы уверены на момент написания кода, что ошибок не может быть), то напишите save!, а если обрабатываете, то save.
            0
            Метод save! кинет исключение, а save не кинет, в итоге ошибка будет проигнорирована, если человек забудет проверить возвращаемое значение.
            Поэтому если не писать эту проверку сразу же, то надо обязательно использовать save!
        +5
        Есть корректный способ в миграциях менять данные:
        #псевдокод
        class Migration < BaseMigration
          class User < AR::Base
          end
        
          def up
            User.update_all smth: 123
          end
        
          def down
          end
        end
        

        Просто объявлем минимальные модели для миграции. Если нужны сложные проеобразования, то лучше написать 1 sql запрос и не использовать activerecord.

        Даже если в коде проекта есть модель пользователя, в миграции будет использоваться модель из неймспейса миграции.
          +3
          По первому пункту могу посоветовать ruby style guide (https://github.com/bbatsov/ruby-style-guide). Там описаны конкретно эти примеры (двойное отрицание, множественные условия в unless и т.д.).
          И вообще, на самом деле этот гайд учит писать код не так, чтобы он работал, а чтобы его понимали другие люди, которые потом будут его читать.
          А также позволяет уменьшить количество использование лишних блоков кода.
            0
            Если вам нужно обрабатывать три различных значения – используйте текстовое поле.
            Сомнительный совет, когда есть enum (Rails 4.1+).
              0
              «Поддержка» enum в Рельсах тоже сомнительна.
                0
                Приведённые там доводы сомнительны.

                Автор утверждает, что enum надо использовать для значений, у которых хорошо выражена численная природа. Но позвольте, что тогда не так с обычными числами?

                Автор утверждает, что если он надумает дать дамп данных аналитику, то аналитик не поймёт, что там за чиселки встречаются, если нет явного словаря, который только усложнит работу. Это верно. Зато числа компактнее в памяти, быстрее обрабатываются. Выбирайте, что для вас важнее и насколько.

                Автор утверждает, что сравнение состояний между собой приводит к странным результатам. Ну, не надо так делать, если это плохо работает. Используйте числа. То, что enum это числа — деталь реализации (см. конец первого абзаца), обязательной чертой перечисляемого типа является только отношение эквивалентности, но не порядка. Просто компьютеры привыкли к числам и могут их обрабатывать быстрее всего.
              0
              Я использую self для пояснения:
              self.foo — атрибут
              foo — метод
                0
                Атрибут — это тоже метод. Attribute reader. Getter.
                  0
                  Это для рельсовых моделей. Когда атрибут (self.something) — это поле в базе.
                    0
                    Атрибут в руби по сути это переменная экземпляра класса (instance variable), имя которой всегда начинается с символа @. Получить доступ к атрибуту за пределами класса напрямую нельзя. Для чтения атрибута используется reader-метод, а для установки значения атрибута используется writer-метод. Либо в этих же целях можно при необходимости использовать методы instance_variable_get и instance_variable_set.

                    Для удобства определены методы модуля attr_reader и attr_writer, которые создают методы для чтения и записи указанных атрибутов. Также существует метод attr_accessor, который создает и reader-, и writer-метод.

                    По соглашению имя геттера и сеттера (ридера и врайтера) атрибута "@attribute" будут называться attribute и attribute= соответственно.

                    Исходя из этого имеем:

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

                    В статье утверждается, что self.foo необходимо использовать при присвоении. Это так, потому что foo = 5 это создание и присвоение локальной переменной. А self.foo = 5 это вызов writer-метода foo=(5).
                +1
                А я люблю unless. Это мой самый любимый момент в Ruby. Я был в восторге от такой возможности, когда только изучал язык.
                  +1
                  Это же очень простая дилемма: выступая в роли писателя все любят `unless` и вырвиглазные регулярки. Как только мы превращаемся в читателей, копаясь в чужом коде, сразу хочется уканделябрить человека, использующего кашу из логических операций, со скобками, да еще и с неявным семантическим логическим отрицанием.
                    +2
                    Регулярки можно писать и не вырвиглазными, с комментами и выравниванием, как например тут: github.com/arnau/ISO8601/blob/master/lib/iso8601/duration.rb#L193-L208, но это надо хотеть писать читаемый код :-)
                    А unless хорош, когда после него только одно выражение (без && или ||)
                      0
                      А и я не говорил, что регулярки — это плохо :)

                      Насчет `unless` я придерживаюсь мнения, что короткая форма — добро, а длинная — зло, которое всегда говорит о плохой структуре / именовании (переменная userIsEmpty вместо hasUser, например).
                  0
                  Private не изменяет видимость методов класса. Многие считают, что метод bar в этом примере будет приватным.

                  class Foo
                    private
                  
                    def self.bar
                      puts 'bar'
                    end
                  end
                  
                  Foo.bar # => bar
                  


                  Если вам нужен приватный метод класса, используйте privat_class_method или singleton class (class << self).
                    +1
                    Часто возникает ситуация, когда в методе нужно создать объект, как-то его изменить и возвратить. Очевидной имплементация будет

                    def agent
                      agent = Mechanize.new
                      agent.user_agent = 'Mozilla'
                      agent
                    end
                    


                    С этим кодом все хорошо, но он не очень элегантный. Используя Object#tap можно превратить его в

                    def agent
                      Mechanize.new.tap { |agent| agent.user_agent = 'Mozilla' }
                    end
                    

                      +2
                      tap хорош, но лучше в таких случаях использовать:
                      yeild self if block_given?
                      внутри метода initialize инстанцируемого объекта.

                      тогда код будет чуточку лаконичней:
                      
                      def agent
                        Mechanize.new { |agent| agent.user_agent = 'Mozilla' }
                      end
                      


                      Кстати, автор библиотеки используемой в примере такую возможность дал:
                      github.com/sparklemotion/mechanize/blob/master/lib/mechanize.rb#L217
                      +1
                      Недавно видел куча кода типа

                      Model.all.present?

                      не надо так

                      Model.exists?
                        +2
                        Почему-то ничего не было написано про классический User.find, кидающий исключение в случае, если не найдено ни одного юзера и замену ему User.find_by_id
                          0
                          Такая же ситуация, как с методом save! Если не обрабатывается find_by_id, то лучше find использовать.
                            0
                            Да, я как бы в курсе если что… Просто это наиболее часто встречающаяся ошибка среди начинающих рельсовиков — полагать, что find вернеть nil, что update_attributes просто обновляет аттрибуты без вызова сохранения. И что значит, «если не обрабатывается find_by_id»? Каждому оператору свое предназанчение, мне например иногда важнее вызвать find, который в случае чего вызовет у меня exception и кинет мне в багтрекер ошибку. При этом это помогает мне сделать архитектура приложения, которая сама по себе не падает ну или дальнейшее выполнение которой мне уж точно не нужно. Например background таски какие-нибудь. В рельсах же в контроллерах такое лучше не делать по причине того, что нам все-таки по возможности нужно показать либо мессадж с ошибкой пользователю, либо редиректнуть его, либо еще чего похлеще сделать.
                            0
                            В рельсах начиная с 4.0 рекомендуют использовать не find_by_id(value), а find_by(id: value), лучше читается, когда полей для поиска несколько (сравните find_by_name_and_surname(v1, v2) и find_by(name: v1, surname: v2)).
                              0
                              github.com/rails/activerecord-deprecated_finders
                              > Note that find(primary_key), find_by..., and find_by...! are not deprecated.
                                0
                                Но работает это медленнее, потому что find_by_xxx это все через method_missing.
                                  +1
                                  Первый раз только, потом уже метод будет доопределен. github.com/rails/rails/blob/master/activerecord/lib/active_record/dynamic_matchers.rb#L18
                                    0
                                    Да, method_missing примерно в три раза медленне (попробуйте Gist).

                                    Но речь идет о доле микросекунды, в то время как запрос к базе занимает десятки-сотни милисекунд.

                                    Так что профита в использовании find_by(id: ...) вместо find_by_id(...) никакого нет, остается лишь дело вкуса / командных стайл-гайдов.
                              +2
                              Ну я бы не назвал это прямо ошибками. Скорее некоторый относительно субъективный стайл-гайд.

                              А вот что касается bang-методов — так вообще уж извините, но это ересь. Вообще плохой пример. Особенно если в проекте используется модульная система и большинство однотипных методов подобного рода вынесены в единый логический модуль — это совет-убийца. Учитывая особенности фреймворка, я бы лучше порекомендовал делать явные проверки, либо (что еще правильнее де-факто) — любой функционал стараться покрывать тестами. Тогда даже если у нас поменяются данные и тест не пройдет — мы узнаем об этом заранее, до того как на реальном проекте какие-то реальные данные вообще хоть куда-то пойдут.

                              По поводу unless тоже не совсем согласен — приведенный пример не самый удачный. Поправьте меня если неправ, но двойные отрицания лучше всего заменяются на условия, преобразуясь в цепочку условий. В зависимости от ситуации, их можно и обратить, что повысит удобочитаемость. Тот же RubyMine подобные вещи инспектирует и предлагает в некоторых моментах замены как раз подобного рода (не всегда удачно, но у нас по крайней мере есть выбор что удобнее).

                              А вообще в свое время один умный и опытный товарищ поделился со мной двумя замечательными ссылками:
                              github.com/arbox/ruby-style-guide
                              github.com/arbox/rails-style-guide

                              Советую как минимум, ознакомиться.
                                0
                                > Если мы удалим пользователя, то связанные с ним посты перестанут быть корректными

                                foreign keys же.
                                  +1
                                  github.com/matthuhiggins/foreigner до 4.2 и c 4.2 из коробки api.rubyonrails.org/v4.2.0/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_foreign_key:
                                  Creating a cascading foreign key
                                  add_foreign_key :articles, :authors, on_delete: :cascade
                                  
                                  generates:
                                  ALTER TABLE "articles" ADD CONSTRAINT articles_author_id_fk FOREIGN KEY ("author_id") REFERENCES "authors" ("id") ON DELETE CASCADE
                                  

                                  Поддерживаются postgresql и mysql.
                                    0
                                    До этого в моделях ещё можно было использовать has_* :name, dependent: :delete_all, что давало аналогичный результат, но требовало дополнительных запросов к базе.
                                      0
                                      давно пора уже из кед вылезать.
                                    0
                                    Жадный лоадинг иногда уместно использовать вместе с bullet.

                                    Only users with full accounts can post comments. Log in, please.