Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify SendGridMessage internal code #406

Closed
thinkingserious opened this issue Feb 23, 2017 · 4 comments · Fixed by #993
Closed

Simplify SendGridMessage internal code #406

thinkingserious opened this issue Feb 23, 2017 · 4 comments · Fixed by #993
Labels
difficulty: medium fix is medium in difficulty status: work in progress Twilio or the community is in the process of implementing type: twilio enhancement feature request on Twilio's roadmap

Comments

@thinkingserious
Copy link
Contributor

Issue Summary

Refactor repeated code out into utility functions. e.g. https://github.com/sendgrid/sendgrid-csharp/blob/master/src/SendGrid/Helpers/Mail/SendGridMessage.cs#L142

@thinkingserious thinkingserious added status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap labels Feb 23, 2017
@pbolduc
Copy link

pbolduc commented Apr 1, 2017

I have have a signed contributor agreement ready to send.
I have been working on a change for this issue. However, I am unclear on the expected behavior of these methods when you pass a non-zero personalizationIndex or a non-null personalization object. I have plans on create unit tests to validated the behavior before and after to ensure no regressions. For example, if I do:

    var emailAddress = new EmailAddress();
    var personalization = new Personalization();
    _sut.AddTo(emailAddress, 0, personalization);

This throws NullReferenceException because the Tos collection is not initialized.

    var emailAddress1 = new EmailAddress();
    var personalization = new Personalization();
    personalization.Tos = new List<EmailAddress>();
    _sut.AddTo(emailAddress1, personalization: personalization);

    var emailAddress2 = new EmailAddress();
    _sut.AddTo(emailAddress2, 2);

This throws ArgumentOutOfRangeException cause this code checks of the index is greater than the count and insert at the point. I think the expression should be personalizationIndex < this.Personalizations.Count (note, avoid using Linq methods when collections have a property) or it should just be an Add() call instead of Insert or it should check the parameters better.

Thoughts?

@SendGridDX
Copy link

Hello @pbolduc,

For the AddTo method, if you are passing in a personalization, I am assuming that personalization is already initialized. The idea is that you may have already defined several personalizations and now you want to go back and add an email to a particular one.

Regarding the second code example, when you execute _sut.AddTo(emailAddress2, 2); the code should create a new personalization that will be at position 2. I believe you are suggesting that simply append it to the next available location in the list. Perhaps the out of range issue is happening because we only have 1 personalization at this point, which should be at position 0.

@pbolduc
Copy link

pbolduc commented Apr 3, 2017

On the AddTo, it would be possible to have a personalization already initialized but the associated collection Tos / Bccs / Ccs not initialized yet. It could simplify a bunch of things if each of the models that contain collections, that the collections are auto initialized with empty lists. The down side is extra memory is allocated and will need to be garbage collected. Also, during serialization, extra checks would be needed to ensure empty arrays are not serialized.

On the second example, you are correct in that I do no have that many items in the collection. Either the API could throw an error (recommended) or append the new personalization at the end of the collection. To me it would make sense to throw an error because caller would expect to be able to get the newly created personalization at message.Personalization[2]. this would throw index out of bounds exception.

Tightening up the API will make it easier and less error prone for the users. However, it must be consistent with the other language SDKs.

@SendGridDX
Copy link

@pbolduc,

How about adding a constructor here that initializes all the minimally required fields.

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest labels Sep 30, 2017
@thinkingserious thinkingserious added status: work in progress Twilio or the community is in the process of implementing and removed status: help wanted requesting help from the community labels Mar 1, 2018
@childish-sambino childish-sambino added type: twilio enhancement feature request on Twilio's roadmap and removed type: community enhancement feature request not on Twilio's roadmap labels Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: work in progress Twilio or the community is in the process of implementing type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants