Use a final class for Queue instances instead of anonymous trait instances
#9763
Conversation
|
|
||
| private def removeTaker(taker: Promise[Nothing, A])(implicit trace: Trace): UIO[Unit] = | ||
| ZIO.succeed(takers.remove(taker)) | ||
|
|
||
| val capacity: Int = queue.capacity | ||
| override def capacity: Int = queue.capacity |
There was a problem hiding this comment.
queue.capacity is already a val. We don't need a val here too
Queue instances instead of anonymous trait instancesQueue instances instead of anonymous trait instances
| ): Queue[A] = new Queue[A] { | ||
| ): Queue[A] = new QueueImpl[A](queue, takers, shutdownHook, shutdownFlag, strategy) | ||
|
|
||
| final class QueueImpl[A] private[Queue] ( |
There was a problem hiding this comment.
Can we make the class private?
There was a problem hiding this comment.
Done. I didn't make it private because I was worried it would make the class leak its private scope. That's why I made the constructor private and not the class. But the compiler isn't complaining so I guess it's fine to make it private
|
|
||
| def offer(a: A)(implicit trace: Trace): UIO[Boolean] = | ||
| override def offer(a: A)(implicit trace: Trace): UIO[Boolean] = |
There was a problem hiding this comment.
I don't mind it either way, but is there a reason other than readability for adding the override keyword?
There was a problem hiding this comment.
I generally prefer leaving it off - if someone adds a default implementation, they should get a compiler error.
There was a problem hiding this comment.
I prefer adding them. Makes it more explicit. If you differ from the super class or the super has a default value, you get a/the right error message.
There was a problem hiding this comment.
To me, explicitness > implicitness. So yeah it's about readability. So I don't have to guess or to remember that it's overriding something
| shutdownHook: Promise[Nothing, Unit], | ||
| shutdownFlag: AtomicBoolean, | ||
| strategy: Strategy[A] | ||
| ) extends Queue[A] { |
There was a problem hiding this comment.
Can you explain why we need to make this change?
There was a problem hiding this comment.
Better stack traces. That's a kind of change we already made in some other places with Kyri
| queue: MutableConcurrentQueue[A], | ||
| takers: ConcurrentDeque[Promise[Nothing, A]], | ||
| shutdownHook: Promise[Nothing, Unit], | ||
| shutdownFlag: AtomicBoolean, |
There was a problem hiding this comment.
You can merge the Atomic with the Queue to avoid another field.
There was a problem hiding this comment.
I'm sorry, I'm not sure to understand your comment 🤔
No description provided.