I’m really struggling to come up with a design pattern for an Alerter I’m building. Here’s a contrived example of what I’m trying to do:
A person wants to get an alert by weather type (rain, snow, sun, etc.). A person also has a choice of alert method (email, sms, slack channel, hipchat room, etc.)
I need to: have a class which takes in a weather type. Then it retrieves all the people that care about that weather type. Then it loops through all the people and sends them their alert (based on the person’s alert type preference).
I’m trying to stay away from the smell of switch statements, but maybe it’s not as bad as it seems?
Here’s my basic outline, but it seems like it should be done “better”:
public class Alerter
{
private readonly WeatherType _weatherType;
public Alerter(WeatherType weatherType)
{
_weatherType = weatherType;
}
public void SendAlerts()
{
var people = PersonRepository.GetPeople(_weatherType);
foreach (Person person in people)
{
switch (person.AlertType)
{
case Email:
var e = new EmailAlerter();
e.SendToPerson(person, _weatherType);
return;
case SMS:
var s = new SmsAlerter();
s.SendToPerson(person, _weatherType);
return;
}
}
}
}
5
var people = PersonRepository.GetPeople(_weatherType);
The first thing to note is that you are using a service locator here, which is widely recognised as an anti-pattern. So you could improve that by injecting an instance of IPersonRepository into the Alerter constructor.
switch (person.AlertType)
{
case Email:
...
}
The switch statement has a lot of duplicated code as each case is selecting a type and invoking SendToPerson on that type. This could be replaced with a factory (injected via the constructor, of course) that returns an instance of IAlerter and the SendToPerson method called on that:
public void SendAlerts()
{
var people = _personRepository.GetPeople(_weatherType);
foreach (var person in people)
{
var alerter = _alerterFactory(person.AlertType);
alerter.SendToPerson(person, _weatherType);
}
}
(note, I have assumed the return‘s are a mistake in your code, otherwise it would only ever alert the first person.)
Since every user has its own prefered alert method (probably coming from some user settings), this information belongs to the user (= should be part of the user’s internal state, as an attribute). User objects must be responsible for deciding how to send the alerts and it must be opaque for the caller.
public void SendAlerts()
{
var people = PersonRepository.GetPeople(_weatherType);
foreach (Person person in people)
{
person.send(_weatherType); //inside it's using EmailAlerter, SmsAlerter, etc.
}
}
That way, you get rid of any switch. Note that I’m not using any design pattern (exception maybe dependency injection).
Design patterns are no silver bullet: they are good servants but bad masters. Don’t let all your design decisions be guided by them, but use them only when you face a problem that a design pattern has already resolved.