alexandru / scala-best-practices

A collection of Scala best practices
4.39k stars 623 forks source link

2.17. SHOULD NOT define case classes nested in other classes #16

Open muuki88 opened 10 years ago

muuki88 commented 10 years ago

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?

ericacm commented 10 years ago

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.

alexandru commented 10 years ago

@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 commented 10 years ago

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).

SiliconMeeple commented 10 years ago

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

ejstrobel commented 9 years ago

:+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.

pelepelin commented 7 years ago

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.