Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
muuki88 opened this issue Nov 2, 2014 · 6 comments
Open

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

muuki88 opened this issue Nov 2, 2014 · 6 comments

Comments

@muuki88
Copy link

muuki88 commented Nov 2, 2014

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
Copy link

ericacm commented Nov 3, 2014

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
Copy link
Owner

@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
Copy link
Owner

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
Copy link

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

@ejstrobel
Copy link

👍 @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
Copy link

pelepelin commented Jun 28, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants