Refactor Mail Helper Array Assignments

[thinkingserious]

This issue comes from Twitter: https://twitter.com/mootpointer/status/776305972339838976

Reference code: https://github.com/sendgrid/sendgrid-ruby/blob/master/lib/sendgrid/helpers/mail/mail.rb#L864

def attachments=(attachments)
  @attachments = @attachments.nil? ? [] : @attachments
  @attachments = @attachments.push(attachments.to_json)
end

should be:

def add_attachment(attachment)
  @attachments = @attachments.nil? ? [] : @attachments
  @attachments = @attachments.push(attachment.to_json)
end

This is true for all cases where we assign to an array in the Mail Helper.

Technical details:

• sendgrid-ruby Version: 4.0
• Ruby Version: 2.2

[mootpointer]

Thanks for opening this. Something like the above would be what I would suggest. This should be done in concert with adding deprecation warnings to the existing setter methods, or at least signal that they do not work as standard setter methods.

[swifthand]

While looking at another issue I noticed that the default checks in all of the array assignments.

There is a bit of idiomatic Ruby used to wrap this @value.nil? ? default_value : @value, that is to place the default in the reader method as an ||= assignment, and have the class (even internally) access the array attribute via the reader method. This removes the need for a nil-or-apply-default check at every single use.

Additionally, because Array#push in Ruby is mutating, the re-assignment back to @attachments is unnecessary. Simply @attachments.push(attachment.to_json) is sufficient.

Applying this both in the case of attachments, it looks like this:

def add_attachment(attachment)
  attachments.push(attachment.to_json)
end

def attachments
  @attachments ||= []
end

The Mail#attachments method will, upon first use, detect that @attachments is falsey and perform the assignment, thus providing it with a default value of []. Subsequent calls will simply return @attachments in whatever state it currently is.

Then, in Mail#add_attachment, we can safely reference @attachments by instead accessing it via the method, and be guaranteed to always receive an Array, which we can then push onto.

[thinkingserious]

Loving the feedback @swifthand!

Could you please fill out this form so we can send you some swag?

[swifthand]

@thinkingserious Gladly!

Also saw the link to the Mail Helper project. I haven't used Github's new project feature before but I'll give it a look. Does this issue fall under that as well or are these array assignments best addressed separately?

[thinkingserious]

Hi @swifthand,

I think this issue belongs in the Mail Helper project. I'm not quite sure how to best manage that yet, we are new to the Projects feature too, kicking the tires.

My guess is that we will swarm on the requirements, agree on a scope of work, break up the tasks into issues and add them to the project's backlog.

[thinkingserious]

Hello Everyone!

Please follow progress here: #86

Thanks!