From d2d4aa96e3ab1729b800ae6835a2d15a5ce02dcb Mon Sep 17 00:00:00 2001 From: Patrik Nordwall Date: Wed, 10 Jul 2019 14:45:37 +0200 Subject: [PATCH] Style Guide: Passing around too many parameters, #22805 --- .../akka/typed/StyleGuideDocExamples.java | 272 ++++++++++++++++++ .../akka/typed/StyleGuideDocExamples.scala | 173 +++++++++++ .../src/main/paradox/typed/style-guide.md | 66 +++++ 3 files changed, 511 insertions(+) diff --git a/akka-actor-typed-tests/src/test/java/jdocs/akka/typed/StyleGuideDocExamples.java b/akka-actor-typed-tests/src/test/java/jdocs/akka/typed/StyleGuideDocExamples.java index db9cfaec9e..0149a1c374 100644 --- a/akka-actor-typed-tests/src/test/java/jdocs/akka/typed/StyleGuideDocExamples.java +++ b/akka-actor-typed-tests/src/test/java/jdocs/akka/typed/StyleGuideDocExamples.java @@ -13,6 +13,9 @@ import akka.actor.typed.javadsl.Behaviors; // #fun-style import akka.actor.typed.javadsl.AbstractBehavior; import akka.actor.typed.javadsl.Receive; +import akka.actor.typed.javadsl.TimerScheduler; + +import java.time.Duration; // #oo-style interface StyleGuideDocExamples { @@ -131,4 +134,273 @@ interface StyleGuideDocExamples { // #oo-style } + + interface FunctionalStyleSetupParams1 { + // #fun-style-setup-params1 + + // this is an anti-example, better solutions exists + public class Counter { + public interface Command {} + + public static class IncrementRepeatedly implements Command { + public final Duration interval; + + public IncrementRepeatedly(Duration interval) { + this.interval = interval; + } + } + + public enum Increment implements Command { + INSTANCE + } + + public static class GetValue implements Command { + public final ActorRef replyTo; + + public GetValue(ActorRef replyTo) { + this.replyTo = replyTo; + } + } + + public static class Value { + public final int value; + + public Value(int value) { + this.value = value; + } + } + + public static Behavior create(String name) { + return Behaviors.setup( + context -> Behaviors.withTimers(timers -> counter(name, context, timers, 0))); + } + + private static Behavior counter( + final String name, + final ActorContext context, + final TimerScheduler timers, + final int n) { + + return Behaviors.receive(Command.class) + .onMessage( + IncrementRepeatedly.class, + command -> onIncrementRepeatedly(name, context, timers, n, command)) + .onMessage(Increment.class, notUsed -> onIncrement(name, context, timers, n)) + .onMessage(GetValue.class, command -> onGetValue(n, command)) + .build(); + } + + private static Behavior onIncrementRepeatedly( + String name, + ActorContext context, + TimerScheduler timers, + int n, + IncrementRepeatedly command) { + context + .getLog() + .debug( + "[{}] Starting repeated increments with interval [{}], current count is [{}]", + name, + command.interval, + n); + timers.startTimerWithFixedDelay("repeat", Increment.INSTANCE, command.interval); + return Behaviors.same(); + } + + private static Behavior onIncrement( + String name, ActorContext context, TimerScheduler timers, int n) { + int newValue = n + 1; + context.getLog().debug("[{}] Incremented counter to [{}]", name, newValue); + return counter(name, context, timers, newValue); + } + + private static Behavior onGetValue(int n, GetValue command) { + command.replyTo.tell(new Value(n)); + return Behaviors.same(); + } + } + // #fun-style-setup-params1 + } + + interface FunctionalStyleSetupParams2 { + // #fun-style-setup-params2 + + // this is better than previous example, but even better solution exists + public class Counter { + // messages omitted for brevity, same messages as above example + // #fun-style-setup-params2 + public interface Command {} + + public static class IncrementRepeatedly implements Command { + public final Duration interval; + + public IncrementRepeatedly(Duration interval) { + this.interval = interval; + } + } + + public enum Increment implements Command { + INSTANCE + } + + public static class GetValue implements Command { + public final ActorRef replyTo; + + public GetValue(ActorRef replyTo) { + this.replyTo = replyTo; + } + } + + public static class Value { + public final int value; + + public Value(int value) { + this.value = value; + } + } + // #fun-style-setup-params2 + + private static class Setup { + final String name; + final ActorContext context; + final TimerScheduler timers; + + private Setup(String name, ActorContext context, TimerScheduler timers) { + this.name = name; + this.context = context; + this.timers = timers; + } + } + + public static Behavior create(String name) { + return Behaviors.setup( + context -> + Behaviors.withTimers(timers -> counter(new Setup(name, context, timers), 0))); + } + + private static Behavior counter(final Setup setup, final int n) { + + return Behaviors.receive(Command.class) + .onMessage( + IncrementRepeatedly.class, command -> onIncrementRepeatedly(setup, n, command)) + .onMessage(Increment.class, notUsed -> onIncrement(setup, n)) + .onMessage(GetValue.class, command -> onGetValue(n, command)) + .build(); + } + + private static Behavior onIncrementRepeatedly( + Setup setup, int n, IncrementRepeatedly command) { + setup + .context + .getLog() + .debug( + "[{}] Starting repeated increments with interval [{}], current count is [{}]", + setup.name, + command.interval, + n); + setup.timers.startTimerWithFixedDelay("repeat", Increment.INSTANCE, command.interval); + return Behaviors.same(); + } + + private static Behavior onIncrement(Setup setup, int n) { + int newValue = n + 1; + setup.context.getLog().debug("[{}] Incremented counter to [{}]", setup.name, newValue); + return counter(setup, newValue); + } + + private static Behavior onGetValue(int n, GetValue command) { + command.replyTo.tell(new Value(n)); + return Behaviors.same(); + } + } + // #fun-style-setup-params2 + } + + interface FunctionalStyleSetupParams3 { + // #fun-style-setup-params3 + + // this is better than previous examples + public class Counter { + // messages omitted for brevity, same messages as above example + // #fun-style-setup-params3 + public interface Command {} + + public static class IncrementRepeatedly implements Command { + public final Duration interval; + + public IncrementRepeatedly(Duration interval) { + this.interval = interval; + } + } + + public enum Increment implements Command { + INSTANCE + } + + public static class GetValue implements Command { + public final ActorRef replyTo; + + public GetValue(ActorRef replyTo) { + this.replyTo = replyTo; + } + } + + public static class Value { + public final int value; + + public Value(int value) { + this.value = value; + } + } + // #fun-style-setup-params3 + + public static Behavior create(String name) { + return Behaviors.setup( + context -> + Behaviors.withTimers(timers -> new Counter(name, context, timers).counter(0))); + } + + private final String name; + private final ActorContext context; + private final TimerScheduler timers; + + private Counter(String name, ActorContext context, TimerScheduler timers) { + this.name = name; + this.context = context; + this.timers = timers; + } + + private Behavior counter(final int n) { + return Behaviors.receive(Command.class) + .onMessage(IncrementRepeatedly.class, command -> onIncrementRepeatedly(n, command)) + .onMessage(Increment.class, notUsed -> onIncrement(n)) + .onMessage(GetValue.class, command -> onGetValue(n, command)) + .build(); + } + + private Behavior onIncrementRepeatedly(int n, IncrementRepeatedly command) { + context + .getLog() + .debug( + "[{}] Starting repeated increments with interval [{}], current count is [{}]", + name, + command.interval, + n); + timers.startTimerWithFixedDelay("repeat", Increment.INSTANCE, command.interval); + return Behaviors.same(); + } + + private Behavior onIncrement(int n) { + int newValue = n + 1; + context.getLog().debug("[{}] Incremented counter to [{}]", name, newValue); + return counter(newValue); + } + + private Behavior onGetValue(int n, GetValue command) { + command.replyTo.tell(new Value(n)); + return Behaviors.same(); + } + } + // #fun-style-setup-params3 + } } diff --git a/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala b/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala index bb36a47f75..45d2df39cc 100644 --- a/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala +++ b/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala @@ -6,10 +6,13 @@ package docs.akka.typed //#oo-style //#fun-style +import scala.concurrent.duration.FiniteDuration + import akka.actor.typed.ActorRef import akka.actor.typed.Behavior import akka.actor.typed.scaladsl.ActorContext import akka.actor.typed.scaladsl.Behaviors +import akka.actor.typed.scaladsl.TimerScheduler //#fun-style import akka.actor.typed.scaladsl.AbstractBehavior //#oo-style @@ -81,4 +84,174 @@ object StyleGuideDocExamples { } + object FunctionalStyleSetupParams1 { + // #fun-style-setup-params1 + + // this is an anti-example, better solutions exists + object Counter { + sealed trait Command + case object Increment extends Command + final case class IncrementRepeatedly(interval: FiniteDuration) extends Command + final case class GetValue(replyTo: ActorRef[Value]) extends Command + final case class Value(n: Int) + + def apply(name: String): Behavior[Command] = + Behaviors.withTimers { timers => + counter(name, timers, 0) + } + + private def counter(name: String, timers: TimerScheduler[Command], n: Int): Behavior[Command] = + Behaviors.receive { (context, message) => + message match { + case IncrementRepeatedly(interval) => + context.log.debug( + "[{}] Starting repeated increments with interval [{}], current count is [{}]", + name, + interval, + n) + timers.startTimerWithFixedDelay("repeat", Increment, interval) + Behaviors.same + case Increment => + val newValue = n + 1 + context.log.debug("[{}] Incremented counter to [{}]", name, newValue) + counter(name, timers, newValue) + case GetValue(replyTo) => + replyTo ! Value(n) + Behaviors.same + } + } + } + // #fun-style-setup-params1 + } + + object FunctionalStyleSetupParams2 { + // #fun-style-setup-params2 + + // this is better than previous example, but even better solution exists + object Counter { + sealed trait Command + case object Increment extends Command + final case class IncrementRepeatedly(interval: FiniteDuration) extends Command + final case class GetValue(replyTo: ActorRef[Value]) extends Command + final case class Value(n: Int) + + private case class Setup(name: String, context: ActorContext[Command], timers: TimerScheduler[Command]) + + def apply(name: String): Behavior[Command] = + Behaviors.setup { context => + Behaviors.withTimers { timers => + counter(Setup(name, context, timers), 0) + } + } + + private def counter(setup: Setup, n: Int): Behavior[Command] = + Behaviors.receiveMessage { + case IncrementRepeatedly(interval) => + setup.context.log.debug( + "[{}] Starting repeated increments with interval [{}], current count is [{}]", + setup.name, + interval, + n) + setup.timers.startTimerWithFixedDelay("repeat", Increment, interval) + Behaviors.same + case Increment => + val newValue = n + 1 + setup.context.log.debug("[{}] Incremented counter to [{}]", setup.name, newValue) + counter(setup, newValue) + case GetValue(replyTo) => + replyTo ! Value(n) + Behaviors.same + } + } + // #fun-style-setup-params2 + } + + object FunctionalStyleSetupParams3 { + // #fun-style-setup-params3 + + // this is better than previous examples + object Counter { + sealed trait Command + case object Increment extends Command + final case class IncrementRepeatedly(interval: FiniteDuration) extends Command + final case class GetValue(replyTo: ActorRef[Value]) extends Command + final case class Value(n: Int) + + def apply(name: String): Behavior[Command] = + Behaviors.setup { context => + Behaviors.withTimers { timers => + new Counter(name, context, timers).counter(0) + } + } + } + + class Counter private ( + name: String, + context: ActorContext[Counter.Command], + timers: TimerScheduler[Counter.Command]) { + import Counter._ + + private def counter(n: Int): Behavior[Command] = + Behaviors.receiveMessage { + case IncrementRepeatedly(interval) => + context.log.debug( + "[{}] Starting repeated increments with interval [{}], current count is [{}]", + name, + interval, + n) + timers.startTimerWithFixedDelay("repeat", Increment, interval) + Behaviors.same + case Increment => + val newValue = n + 1 + context.log.debug("[{}] Incremented counter to [{}]", name, newValue) + counter(newValue) + case GetValue(replyTo) => + replyTo ! Value(n) + Behaviors.same + } + } + // #fun-style-setup-params3 + } + + object FunctionalStyleSetupParams4 { + // #fun-style-setup-params4 + + // this works, but previous example is better for structuring more complex behaviors + object Counter { + sealed trait Command + case object Increment extends Command + final case class IncrementRepeatedly(interval: FiniteDuration) extends Command + final case class GetValue(replyTo: ActorRef[Value]) extends Command + final case class Value(n: Int) + + def apply(name: String): Behavior[Command] = + Behaviors.setup { context => + Behaviors.withTimers { timers => + def counter(n: Int): Behavior[Command] = + Behaviors.receiveMessage { + case IncrementRepeatedly(interval) => + context.log.debug( + "[{}] Starting repeated increments with interval [{}], current count is [{}]", + name, + interval, + n) + timers.startTimerWithFixedDelay("repeat", Increment, interval) + Behaviors.same + case Increment => + val newValue = n + 1 + context.log.debug("[{}] Incremented counter to [{}]", name, newValue) + counter(newValue) + case GetValue(replyTo) => + replyTo ! Value(n) + Behaviors.same + } + + counter(0) + } + } + } + + // #fun-style-setup-params4 + } + } diff --git a/akka-docs/src/main/paradox/typed/style-guide.md b/akka-docs/src/main/paradox/typed/style-guide.md index d3aa1f4bdb..7286526f58 100644 --- a/akka-docs/src/main/paradox/typed/style-guide.md +++ b/akka-docs/src/main/paradox/typed/style-guide.md @@ -124,3 +124,69 @@ Some reasons why you may want to use the functional style: @@@ +## Passing around too many parameters + +One thing you will quickly run into when using the functional style is that you need to pass around many parameters. + +Let's add `name` parameter and timers to the previous `Counter` example. A first approach would be to just add those +as separate parameters: + +Scala +: @@snip [StyleGuideDocExamples.scala](/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala) { #fun-style-setup-params1 } + +Java +: @@snip [StyleGuideDocExamples.java](/akka-actor-typed-tests/src/test/java/jdocs/akka/typed/StyleGuideDocExamples.java) { #fun-style-setup-params1 } + +Ouch, that doesn't look good. More things may be needed, such as stashing or application specific "constructor" +parameters. As you can imagine, that will be too much boilerplate. + +As a first step we can place all these parameters in a class so that we at least only have to pass around one thing. +Still good to have the "changing" state, the @scala[`n: Int`]@java[`final int n`] here, as a separate parameter. + +Scala +: @@snip [StyleGuideDocExamples.scala](/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala) { #fun-style-setup-params2 } + +Java +: @@snip [StyleGuideDocExamples.java](/akka-actor-typed-tests/src/test/java/jdocs/akka/typed/StyleGuideDocExamples.java) { #fun-style-setup-params2 } + +That's better. Only one thing to carry around and easy to add more things to it without rewriting everything. +@scala[Note that we also placed the `ActorContext` in the `Setup` class, and therefore switched from +`Behaviors.receive` to `Behaviors.receiveMessage` since we already have access to the `context`.] + +It's still rather annoying to have to pass the same thing around everywhere. + +We can do better by introducing an enclosing class, even though it's still using the functional style. +The "constructor" parameters can be @scala[immutable]@java[`final`] instance fields and can be accessed from +member methods. + +Scala +: @@snip [StyleGuideDocExamples.scala](/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala) { #fun-style-setup-params3 } + +Java +: @@snip [StyleGuideDocExamples.java](/akka-actor-typed-tests/src/test/java/jdocs/akka/typed/StyleGuideDocExamples.java) { #fun-style-setup-params3 } + +That's nice. One thing to be cautious with here is that it's important that you create a new instance for +each spawned actor, since those parameters must not be shared between different actor instances. That comes natural +when creating the instance from `Behaviors.setup` as in the above example. Having a +@scala[`apply` factory method in the companion object and making the constructor private is recommended.] +@java[static `create` factory method and making the constructor private is highly recommended.] + +This can also be useful when testing the behavior by creating a test subclass that overrides certain methods in the +class. The test would create the instance without the @scala[`apply` factory method]@java[static `create` factory method]. +Then you need to relax the visibility constraints of the constructor and methods. + +It's not recommended to place mutable state and @scala[`var` members]@java[non-final members] in the enclosing class. +It would be correct from an actor thread-safety perspective as long as the same instance of the enclosing class +is not shared between different actor instances, but if that is what you need you should rather use the +object-oriented style with the `AbstractBehavior` class. + +@@@ div {.group-scala} + +Similar can be achieved without an enclosing class by placing the `def counter` inside the `Behaviors.setup` +block. That works fine, but for more complex behaviors it can be better to structure the methods in a class. +For completeness, here is how it would look like: + +Scala +: @@snip [StyleGuideDocExamples.scala](/akka-actor-typed-tests/src/test/scala/docs/akka/typed/StyleGuideDocExamples.scala) { #fun-style-setup-params4 } + +@@@