atc0005 / bounce

Small utility to assist with building HTTP endpoints
MIT License
1 stars 0 forks source link

Refactor teamsNotifier, emailNotifier #37

Open atc0005 opened 4 years ago

atc0005 commented 4 years ago

Both of these functions are nearly identical with the exception of the parameter list (just one?) and the anonymous function called within to perform the actual notification.

One route I tinkered with was creating a common settings "bundle" within the notifications manager and passing that bundle into two instances of a common notifier function. Another approach is to have only a single instance, but apply checks within to determine if notification type should be attempted.

Regardless of the final outcome, the initial implementation on the bench for #21 duplicates nearly 1:1 the content of teamsNotifier and emailNotifier functions and this is a strong code smell.

atc0005 commented 4 years ago

Thought: Create an actual Go type for each notification type (e.g., email, teams, ...)?

atc0005 commented 4 years ago

The timeoutValue could be calculated outside of the worker and passed to the specific notifier instance. This would be another example of keeping config package references concentrated within the StartNotifyMgr function.

timeoutValue := config.GetTimeout(
    sendTimeout,
    nextScheduledNotification,
    retries,
    retriesDelay,
)

The timeout, delays, etc currently unique to a specific notifier could also be handled in a similar manner.

atc0005 commented 4 years ago

Hearing about how database drivers "register" with the sql package via init side-effects makes me think about using a registration system here. It could help with a number of checks that are currently wired in that look to see whether notifications are enabled.

Example:

// If we don't have *any* notifications enabled we will just
// discard the item we have pulled from the channel
if !cfg.NotifyEmail() && !cfg.NotifyTeams() {
    // ...
}
atc0005 commented 4 years ago

see also atc0005/brick#4