betagouv / betaGouvBot

Automated assistant for beta.gouv.fr administrative tasks on slack.
https://beta.gouv.fr
MIT License
4 stars 10 forks source link

Hide FormatMail implementation #84

Closed bonjourmauko closed 7 years ago

bonjourmauko commented 7 years ago

Superseded by #89

Morendil commented 7 years ago

Why not, but it seems to me (now that we've picked up a few more sticks off the top) that MailAction wants to own this implementation. Observe the pattern - we're always instantiating MailAction with the result of calling FormatMail (via various format_mail methods which do not take consistent parameters).

From the perspective of client code we might as well cut the middleman and pass to MailAction's constructor whatever we're passing to format_mail - a mail template filename, a recipient list, and a templating context. This will make the calls more consistent. Killing two birds with one stone, that will also get rid of the smell in MailAction where it has to reach into the innards of a hash structure. Instead, MailAction can instantiate FormatMailand ask it for subject and recipients.

Let me know if you'd like to tackle this or if you'd rather I did.

bonjourmauko commented 7 years ago

Sounds good, go ahead 🙂.

bonjourmauko commented 7 years ago

Sorry for my laconic previous message. Long version now:

I'm really happy about the work we've done the last few weeks. We've together let emerge, in a pragmatic way, some patterns by refactoring rigorously. I'm convinced that's a very good way to build software (I can't say the best, as I can't either say we've applied always "best practices", for there's always better: there's always space for improvement, and what's better can only come from within and not imported from the outside).

That being said, I've confidence in the fact we're arriving to a state where we can feel responsibilities are better distributed, architecture becomes more modular and patterns recognisable. The payoff of our efforts is an application that's easier to extend, to better fit its purpose.

Kudos!