Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Code Review Gems

Code Review Gems

From Scala eXchange, London, 2014

This session explores difficulties teams are experiencing with Scala, and how to overcome them. If you're starting or growing a team, the code examples and alternatives we present may help you progress faster.

The examples are from commercial code reviews across a wide range of developers and projects. We've seen what developers are struggling with, and how teams are shipping code while getting to grips with Scala.

We will look at examples including:

* managing error handling and configuration
* failure to use the type system
* battling with libraries; and
* making code easier to read and maintain.

Behind each piece of code there's a reason why the code ended up that way. Richard will provide suggestions from the Scala ecosystem that would give a better result.

Richard Dallaway

December 09, 2014
Tweet

More Decks by Richard Dallaway

Other Decks in Technology

Transcript

  1. Focus on Two Questions Are we producing good Scala?
 …making

    the most of what Scala offers? Is this code maintainable?
  2. class Settings() { val dbUser: Option[String] = read("db.user") } def

    doTheWork(conf: Settings): Unit = { val user = try { conf.dbUser.get } catch { case nse: NoSuchElementException => throw new IllegalArgumentException(“Missing setting") } } doTheWork(new Settings())
  3. class Settings() { val dbUser: Option[String] = read("db.user") } def

    doTheWork(conf: Settings): Unit = { val user = try { conf.dbUser.get } catch { case nse: NoSuchElementException => throw new IllegalArgumentException(“Missing setting") } } doTheWork(new Settings())
  4. class Settings() { val dbUser: Option[String] = read("db.user") } def

    doTheWork(conf: Settings): Unit = { val user = try { conf.dbUser.get } catch { case nse: NoSuchElementException => throw new IllegalArgumentException(“Missing setting") } } doTheWork(new Settings()) 㱺 Unit
  5. import com.typesafe.config.{Config,ConfigFactory} class Settings(conf: Config) { val dbUser: String =

    conf.getString("db.user") } val settings = new Settings(ConfigFactory.load()) def doTheWork(conf: Settings): Unit = { println(s"Using ${conf.dbUser}") } doWork(settings)
  6. import com.typesafe.config.{Config,ConfigFactory} class Settings(conf: Config) { val dbUser: String =

    conf.getString("db.user") } val settings = new Settings(ConfigFactory.load()) def doTheWork(conf: Settings): Unit = { println(s"Using ${conf.dbUser}") } doWork(settings)
  7. Observations Watch out for ugly code Make time to discover

    what’s already out there
 …and adapt if it doesn’t quite fit.
  8. Observations Watch out for ugly code Make time to discover

    what’s already out there
 …and adapt if it doesn’t quite fit. Unit is the end of the line
  9. Good Scala? Something the team arrives at Based on the

    broader community Changes over time
  10. object PaymentStatus { val NOT_STARTED = "n/a" val PENDING =

    "pending" val COMPLETE = "complete" val REJECTED = "rejected" } def isFinished(status: String): Boolean = status == COMPLETE || status == REJECTED // Run-time match error: "uh-oh" match { case NOT_STARTED | PENDING => false case COMPLETE | REJECTED => true }
  11. object PaymentStatus { val NOT_STARTED = "n/a" val PENDING =

    "pending" val COMPLETE = "complete" val REJECTED = "rejected" } def isFinished(status: String): Boolean = status == COMPLETE || status == REJECTED // Run-time match error: "uh-oh" match { case NOT_STARTED | PENDING => false case COMPLETE | REJECTED => true }
  12. – Martin “Enums fill a different niche: essentially as efficient

    as integer constants but safer and more convenient to define and to use.”
  13. object StatusEnum extends Enumeration { type Status = Value val

    NotStarted = Value("n/a") val Pending = Value("pending") val Complete = Value("complete") val Rejected = Value("rejected") } def isFinished(status: Status): Boolean = status == Complete || status == Rejected def isInProgress(status: Status): Boolean = status match { case Pending => true case Complete | Rejected => false }
  14. object StatusEnum extends Enumeration { type Status = Value val

    NotStarted = Value("n/a") val Pending = Value("pending") val Complete = Value("complete") val Rejected = Value("rejected") } def isFinished(status: Status): Boolean = status == Complete || status == Rejected def isInProgress(status: Status): Boolean = status match { case Pending => true case Complete | Rejected => false }
  15. Enumerations Automatic integer identifier Automatic string name Values are ordered

    Automatic iterator Light (not one class per value) Same type after erasure No non-exhaustive match warning
  16. object Status { sealed abstract class Value(val name: String) case

    object NotStarted extends Value("n/a") case object Pending extends Value("pending") case object Complete extends Value("complete") case object Rejected extends Value("rejected") val values = Seq(NotStarted, Pending, Complete, Rejected) } def isFinished(status: Status.Value): Boolean = status == Complete || status == Rejected // Compile error: match may not be exhaustive // It would fail on the following input: NotStarted def isInProgress(status: Status.Value): Boolean = status match { case Pending => true case Complete | Rejected => false }
  17. object Status { sealed abstract class Value(val name: String) case

    object NotStarted extends Value("n/a") case object Pending extends Value("pending") case object Complete extends Value("complete") case object Rejected extends Value("rejected") val values = Seq(NotStarted, Pending, Complete, Rejected) } def isFinished(status: Status.Value): Boolean = status == Complete || status == Rejected // Compile error: match may not be exhaustive // It would fail on the following input: NotStarted def isInProgress(status: Status.Value): Boolean = status match { case Pending => true case Complete | Rejected => false }
  18. implicit val statusColumnType = MappedColumnType.base[Value, String]( _.name, s => Status.values.find(_.name

    == s) getOrElse NotStarted ) case class Payment(status: Value, id: Long=0L) lazy val payments = TableQuery[Payments] def pending(implicit s: Session) = payments.filter(_.status === (Pending : Value)).list
  19. implicit val statusColumnType = MappedColumnType.base[Value, String]( _.name, s => Status.values.find(_.name

    == s) getOrElse NotStarted ) case class Payment(status: Value, id: Long=0L) lazy val payments = TableQuery[Payments] def pending(implicit s: Session) = payments.filter(_.status === (Pending : Value)).list
  20. Observations Let the type system help you Get data into

    types as soon as you can “Scala Enumerations”
 https://underscoreconsulting.com/blog/posts/
 2014/09/03/enumerations.html
  21. class ProblemSpec extends Specification { "The processor" should { "detect

    a name change" in { // If this was in the database... val ap = Applicant(number="0000000002", 
 surname="Smith", forenames=None) // ...and we received this change... val input = CsvRow( year = "2014", code = Code.Important, personId = "000000001", number = "0000000002", choice = Choice.Third, surname = Some("Smith"), forenames = Some("Alice") ) Processor.diff(ap, input) must beSome(NameChange()) } } }
  22. class ProblemSpec extends Specification { "The processor" should { "detect

    a name change" in { // If this was in the database... val ap = Applicant(number="0000000002", 
 surname="Smith", forenames=None) // ...and we received this change... val input = CsvRow( year = "2014", code = Code.Important, personId = "000000001", number = "0000000002", choice = Choice.Third, surname = Some("Smith"), forenames = Some("Alice") ) Processor.diff(ap, input) must beSome(NameChange()) } } }
  23. class SolutionSpec extends Specification { "The processor" should { "detect

    a name change" in { // If this was in the database... val app = TestApplicant() withNoForenames // ...and we received this change... val input = TestCsvRow() matching(app) withForenames "Alice" // ...the action must be a name change: Processor.diff(app.create, input.create) must beSome(NameChange()) } } }
  24. case class TestApplicant( number: String="0000000002", surname: String="Smith", forenames: Option[String]=Some("Colin")) {

    def withNoForenames: TestApplicant = copy(forenames=None) def create: Applicant = Applicant(number, surname, forenames) }
  25. import org.scalacheck._, Arbitrary._, Gen._ object Builders { lazy val applicantGenerator:

    Gen[Applicant] = for { number <- tenRandomDigits surname <- arbitrary[String] forenames <- arbitrary[Option[String]] } yield Applicant(number, surname, forenames) val tenRandomDigits = listOfN(10, Gen.numChar).map(_.mkString) implicit class ApplicantOps(app: Applicant) { def withNoForenames: Applicant = app.copy(forenames=None) } implicit val applicant: Arbitrary[Applicant] = Arbitrary(applicantGenerator) }
  26. import org.scalacheck._, Arbitrary._, Gen._ object Builders { lazy val applicantGenerator:

    Gen[Applicant] = for { number <- tenRandomDigits surname <- arbitrary[String] forenames <- arbitrary[Option[String]] } yield Applicant(number, surname, forenames) val tenRandomDigits = listOfN(10, Gen.numChar).map(_.mkString) implicit class ApplicantOps(app: Applicant) { def withNoForenames: Applicant = app.copy(forenames=None) } implicit val applicant: Arbitrary[Applicant] = Arbitrary(applicantGenerator) } Applicant(8698360245,,None)

  27. import org.scalacheck._, Arbitrary._, Gen._ object Builders { lazy val applicantGenerator:

    Gen[Applicant] = for { number <- tenRandomDigits surname <- arbitrary[String] forenames <- arbitrary[Option[String]] } yield Applicant(number, surname, forenames) val tenRandomDigits = listOfN(10, Gen.numChar).map(_.mkString) implicit class ApplicantOps(app: Applicant) { def withNoForenames: Applicant = app.copy(forenames=None) } implicit val applicant: Arbitrary[Applicant] = Arbitrary(applicantGenerator) } Applicant(8698360245,,None)
 Applicant(5652792232,ʳ襍!మۄᭀ勽洆尩シ¡儺㤩㿀蜦î⫴躪㦙馟 珺苁✋鄐!⺟椙㠖䙷⦶"↓⽉⍿¢"璒!罁ⴰⴱⴲⴳⴴⴵⴶⴷ!ѳ꜏䓐ꫠꫡꫢꫣꫤꫥꫦꫧự!侽ᘞ藜܋!铜,
 Some(ಬ枃ཎᴄ䏺顅))
  28. import org.scalacheck._, Arbitrary._, Gen._ object Builders { lazy val applicantGenerator:

    Gen[Applicant] = for { number <- tenRandomDigits surname <- arbitrary[String] forenames <- arbitrary[Option[String]] } yield Applicant(number, surname, forenames) val tenRandomDigits = listOfN(10, Gen.numChar).map(_.mkString) implicit class ApplicantOps(app: Applicant) { def withNoForenames: Applicant = app.copy(forenames=None) } implicit val applicant: Arbitrary[Applicant] = Arbitrary(applicantGenerator) }
  29. import org.scalacheck._, Arbitrary._, Gen._ object Builders { lazy val applicantGenerator:

    Gen[Applicant] = for { number <- tenRandomDigits surname <- arbitrary[String] forenames <- arbitrary[Option[String]] } yield Applicant(number, surname, forenames) val tenRandomDigits = listOfN(10, Gen.numChar).map(_.mkString) implicit class ApplicantOps(app: Applicant) { def withNoForenames: Applicant = app.copy(forenames=None) } implicit val applicant: Arbitrary[Applicant] = Arbitrary(applicantGenerator) }
  30. import org.scalacheck._, Arbitrary._, Gen._ object Builders { lazy val applicantGenerator:

    Gen[Applicant] = for { number <- tenRandomDigits surname <- arbitrary[String] forenames <- arbitrary[Option[String]] } yield Applicant(number, surname, forenames) val tenRandomDigits = listOfN(10, Gen.numChar).map(_.mkString) implicit class ApplicantOps(app: Applicant) { def withNoForenames: Applicant = app.copy(forenames=None) } implicit val applicant: Arbitrary[Applicant] = Arbitrary(applicantGenerator) }
  31. class RandSpec extends Specification with ScalaCheck { import Builders._ "The

    processor" should { "detect a name change" in prop { (applicant: Applicant, input: CsvRow) => Processor.diff( applicant withNoForenames, input matching(applicant) withForenames “Alice") must beSome(NameChange()) } } }
  32. Observations You can still use what you know Make use

    of an expressive syntax
 …and neat libraries Don’t settle for crappy tests
  33. Challenges Code Maintenance Learning the language Explaining the problem Framework

    experience Explaining the thinking Exploring the ecosystem Readable tests
  34. Summary Learn about the wider Scala ecosystem
 —get out there!

    Don’t leave it until “the review”
 —think about code as if you were external