GAS-ICS-Sync icon indicating copy to clipboard operation
GAS-ICS-Sync copied to clipboard

Beginning to refactor globals into locals

Open bandtank opened this issue 4 years ago • 6 comments

In response to #196, I started refactoring the code to remove several globals. I also removed some unnecessary conditional logic and optimized a handful of temporary variables. Gating logic has been moved into the email summary function, which also removed the need for the boolean. Lastly, I tweaked a few comments to be more consistent with the rest of the comments.

I tested the changes using 12 calendars in a variety of configurations along with added, modified, and removed events.

bandtank avatar Apr 14 '21 17:04 bandtank

I added a couple comments, but otherwise I think it looks pretty good. @jonas0b1011001 to double-check functionality and see if he wants to add any comments as well

derekantrican avatar Apr 14 '21 17:04 derekantrican

Looks fine to me. The only thing we are losing is the ability to enable "new version available" mails without receiving summary mails aswell. Not sure if we should keep the extra switch.

jonas0b1011001 avatar Apr 14 '21 18:04 jonas0b1011001

Looks fine to me. The only thing we are losing is the ability to enable "new version available" mails without receiving summary mails aswell. Not sure if we should keep the extra switch.

Maybe a configuration for each type of email is warranted. Something like enableUpdateEmail and enableSummaryEmail. For example:

var enableUpdateEmail = true;          // Send an email if a new version of the script is available
var enableSummaryEmail = false;        // Send a summary email after each run if anything changes
var email = "";                        // To receive emails, an address must be provided
...
if(enableUpdateEmail) checkForUpdate();
...
if(enableSummaryEmail) sendSummary(email, addedEvents, modifiedEvents, removedEvents);
...

bandtank avatar Apr 14 '21 18:04 bandtank

That's what i was thinking. I'm happy with your example.

jonas0b1011001 avatar Apr 14 '21 19:04 jonas0b1011001

I think one thing helpful to note here is that you could just use the bool and use Session.getActiveUser().getEmail() to get the email address. Though I don't know if that would cause more perm requests (related to #123)

derekantrican avatar Apr 14 '21 19:04 derekantrican

I think it's better to make more of it as automated as possible, which would include things like Session.getActiveUser().getEmail(). However, users may want to send the notifications to another email address, so a way to choose would be good. I'm going to submit a different PR with a few tweaks to the notification flow, which I didn't think were a good fit for this PR due to the more limited scope.

bandtank avatar Apr 14 '21 19:04 bandtank