0

我有三个模型（Message、User和Recipient）。我有一个方法Message，目前看起来像这样：

def add_recipient(emails)
  emails.split(' ').map do |email|
    user = User.where(email: email).first_or_create
    user.persisted? ? user : nil
  end.compact.map do |user|
    recipient = Recipient.new(message: self, user: user)
    recipients << recipient
    recipient
  end
end

基本上，它可以接收一封或多封电子邮件（由空格分隔），尝试使用它查找或创建一个有效用户（删除任何无效电子邮件），然后将它们作为收件人添加到邮件中。

这行得通，但我觉得它非常难看。我能做些什么来重构这个方法？

4

1 回答 1

0

重构建议？好的，我们开始吧！

  1. 显示意图：如果添加多个收件人，请将其重命名为add_recipients（复数）。

  2. find_or_create单一职责：在此操作中对用户来说真的是个好主意吗？您将如何报告电子邮件无效？

    代码的读者也不清楚为什么要检查persisted?至少需要一些注释。因此，我将在其他地方处理此登录，并将用户列表传递给该方法。

  3. 使用关联方法：您可以调用build或create在关联代理上，例如self.recipients.build.

写完所有这些，我的代码看起来会完全不同：

users = User.where(email: emails.split(' '))
users.each { |user| message.recipients.build(user: user) }
于 2013-10-22T15:22:32.860 回答