scala-best-practices icon indicating copy to clipboard operation
scala-best-practices copied to clipboard

2.17. SHOULD NOT define case classes nested in other classes

Open muuki88 opened this issue 11 years ago • 6 comments

I saw a lot of people using this for akka-actor messages, like


class MyActor extends Actor {
...
}

object MyActor {
   case class MessageA
   case object MessageB
}

Would this be okay (as the case classes are static) or implies 2.17 that MyActorMessageA would be a better choice?

muuki88 avatar Nov 02 '14 11:11 muuki88

Declaring messages (case classes) in an Actor's companion object is idiomatic Akka.

Which makes sense because objects were designed for modularization. See http://www.artima.com/pins1ed/modular-programming-using-objects.html.

I understand the point about closing over the parent "this", but I'm wondering if there should be exceptions to the rule for use cases like Actor companion objects.

ericacm avatar Nov 03 '14 14:11 ericacm

@ericacm, sorry about the late response, I've been super busy and I missed this issue.

I declare messages inside the actor's companion object all the time. Inside the same process it shouldn't be a problem anyway.

On rule 2.17, to tell you the truth, while closing over this does happen and should be avoided, I have no idea what happens in this particular instance (i.e. plain object in companion object) and I'll have to double-check, because intuitively it shouldn't be much of a problem, but the serialization that Java is doing is pretty dumb so you never know :-)

I'll get back to you on this one, in the meantime I'm thinking that it's OK to leave those in the companion object, simply because we need a namespace of where to put them, so if Java's serialization is that dumb as to break things here, then maybe we shouldn't be doing Java serialization and should be the subject of another rule :-)

alexandru avatar Nov 13 '14 16:11 alexandru

BTW, watch out for the Cake pattern - if you're using the Cake pattern, never declare messages that you want to send to actors and/or serialize inside of a trait that gets mixed in your Cake (I got burned by this, being the reason for why that rule is in).

alexandru avatar Nov 13 '14 16:11 alexandru

Maybe that suggests the rule should be "SHOULD NOT define case classes inside traits (unless deliberately using Path Dependent Types)"?

SiliconMeeple avatar Nov 17 '14 07:11 SiliconMeeple

:+1: @HolyHaddock

But I'd also advise against using Path Dependent Types unless there is a really really good reason, and a simpler solution is not viable.

ejstrobel avatar Dec 03 '14 18:12 ejstrobel

I've done an experiment with Scala 2.11.11 and 2.12.2 and it seems that reasoning does not apply to classes defined in objects.

package test

case class X(xField: String)

object X {
  case class Y(yField: String)
}

object Reflect extends App {
  val y = X.Y("inner-of-object")

  println(y.getClass.getDeclaredFields.mkString("\n"))
}

Prints only private final java.lang.String test.X$Y.yField. Also, if I add some data to companion object, it is not visible in serialized form of y. Also, if I change @SerialVersionUID on companion object, it does not prevent deserialization of y.

// The result is different in IDEA scratch file, because it creates outer object or class.

pelepelin avatar Jun 28 '17 15:06 pelepelin