bripkens / health-check-adapter

Connect health check endpoints to Slack
Apache License 2.0
3 stars 3 forks source link

Type safe props #3

Closed britter closed 8 years ago

britter commented 8 years ago

Apply akka best practices: Introduce props methods in the companion objects of Actors for creating Props in a type safe manner.

britter commented 8 years ago

This is currently WIP. I'm trying to implement this for the creation of reports in the AppActor as well, but I need some time refactor the map function which maps from reporter configs to ActorRefs.

britter commented 8 years ago

I'm working on type safe creation of Reporter actors. Here's what I've got so far in the AppActor:

  val reporters = config.reporters.map(config2Actor)

  def config2Actor(entry: (String, AbstractReporterConfig)): (String, ActorRef) = {
    val consoleReporterClass = classOf[ConsoleReporter]
    val slackReporterClass = classOf[SlackReporter]

    val props = entry._2.implementation match {
      case consoleReporterClass => Props(new ConsoleReporter(mapper, config.asInstanceOf[ConsoleReporterConfig]))
      case slackReporterClass => Props(new SlackReporter(mapper, config.asInstanceOf[SlackReporterConfig]))
    }

    val actor = context.actorOf(props, s"reporter-${entry._1}")
    (entry._1, actor)
  }

This looks pretty ugly. A few things that could be improved:

This would change the above implementation to:

  val reporters = config.reporters.map(config2Actor)

  def config2Actor(entry: (String, AbstractReporterConfig)): (String, ActorRef) = {
       val props = entry._2.implementation match {
      case ConsoleReporterConfiguration.implementation => ConsoleReporter.props(mapper, config.asInstanceOf[ConsoleReporterConfig])
      case SlackReporterConfiguration.implementation => SlackReporter.props(mapper, config.asInstanceOf[SlackReporterConfig])
    }

    val actor = context.actorOf(props, s"reporter-${entry._1}")
    (entry._1, actor)
  }

Pros:

Cons:

I'd like to hear comments before I go further down this road.

bripkens commented 8 years ago

The second solution looks very good to me. Unnecessary boilerplate seems okay to me, especially since the addition of reporters isn't something that is going to happen very often :+1:

hseeberger commented 8 years ago

I don't have the time to dive into the details, but the asInstanceOf absolutely must disappear: there must be a way to use pattern matching more elegantly. Can't you define a fixed set of possible reporters (in config: foo, bar, baz) and create a corresponding ADT (sealed base trait and case objects/classes)?

britter commented 8 years ago

@bripkens voilà! This is ready for reintegration. Please review.

hseeberger commented 8 years ago

Great stuff!

bripkens commented 8 years ago

Cool stuff, thanks @britter! :)

bripkens commented 8 years ago

Also thank you @hseeberger for your comments :)