lunduniversity / introprog

Teaching material for "Introduction to Programming using Scala" at Lund University, LTH. http://cs.lth.se/pgk/
142 stars 173 forks source link

In preperation assignments for blockbattle laboration EventType is used as a trait for things that are events #462

Closed benkibejs closed 2 years ago

benkibejs commented 4 years ago

In 5.2.1 Assignment 7 d the given code includes a trait that has the name EventType, one of the implemented classes with that trait is not in fact just a type of event but rather an event with content. This is a bit philosophical but i would consider a EventType only beeing an enumeration or similar of the different types of events as in PixelWindow.lastEventType.

My solution would be to rename the object containing the events simpy "Events" and the trait Event, since the objects and classes implement are used as events and not just a reference/name of different events.

object Events {
    trait Event
    case class KeyPressed(key: String) extends Event
    case object WindowClosed extends Event
    case object Undefined extends Event
}
bjornregnell commented 4 years ago

Well, this could be an improvement but it is not obvious as the plural form maybe does not read so nice when e.g. if (e == Event.WindowsClosed) ??? etc. I also think this can trigger an enlightning discussion with students of when/if to use the more or less redundant word "Type" in type names. But your solution to the confusion of avoiding Event.Event is good.

benkibejs commented 4 years ago

I definitely think the question of using type in a type name is a good one, and if that question is added in that assignment i probably think that is a great solution! Especially when comparing to the PixelWindow.lastEventType, which is a simple enumeration of different things that might have happened, but is actually under PixelWindow.Event.

bjornregnell commented 4 years ago

Also, I'm a bit worried that I break something in relation to other code examples if I fix this without a thurough inspection of all related stuff, and there is a bit tight on time so I need to prioritize before print deadline. So I'm marking this won't fix for now, but leaving it for next major overhaul in relation to Scala 3 migration etc.