2
\$\begingroup\$

Please take a look at the following Scala program and give me suggestions for improvement. I'm sure there would be plenty. This is my very first Scala code, so please don't be frustrated because of its low quality.

abstract class Expression {

  def eval() : List[List[String]] = this match {
    case Identifier(token) => List(List(token))
    case Union(exprs) => exprs.flatMap(e => e.eval)
    case Sequence(exprs) => exprs.map(e => e.eval).reduceLeft(product)
    case Iteration(min, max, expr) => {
      val subResult = expr.eval;
      (min to max toList)
        .flatMap(card => List.fill(card)(subResult).foldLeft(List(List[String]()))(product))
    }
  }

  def product(first: List[List[String]], second: List[List[String]]) : List[List[String]] = {
    for { x <- first; y <- second} yield x ++ y
  }
}

case class Identifier(token: String) extends Expression

case class Union(subExprs: List[Expression]) extends Expression

case class Sequence(subExprs: List[Expression]) extends Expression

case class Iteration(minCard: Int, maxCard: Int, subExpr: Expression) extends Expression

object App {

  def main(args: Array[String]) = {
    println(
      Iteration(
        1, 2,
        Union(
          List(
            Identifier("cat"),
            Sequence(
              List(
                Identifier("dog"),
                Iteration(
                  0, 1, Identifier("pig")
                ),
                Identifier("bird")
              )
            )
          )
        )
      ).eval
    )
  }
}
\$\endgroup\$
  • \$\begingroup\$ Why have you put the concrete behaviour of the case classes into their parent class, rather than have eval be an abstract method which is made contract in each subclass? That is not typical OO technique. \$\endgroup\$ – itsbruce Dec 24 '14 at 9:47
  • \$\begingroup\$ I implemented an equivalent solution in Java, where I used the visitor design pattern. I just thought maybe in Scala it would be a good idea to exploit pattern matching. \$\endgroup\$ – Dušan Rychnovský Dec 24 '14 at 9:57
  • \$\begingroup\$ Visitor pattern should use pattern matching in Scala, yes. Much less boilerplate. That said, your code doesn't really do anything other than provide a trivial demonstration of that technique, which makes it hard to comment on it. \$\endgroup\$ – itsbruce Dec 24 '14 at 10:16
  • \$\begingroup\$ @itsbruce, Your last comment made me wonder if I shouldn't instead use a visitor-like implementation. However, according to google (e.g. stackoverflow.com/a/8618146/1103412) it seems like my original approach is more idiomatic. \$\endgroup\$ – Dušan Rychnovský Dec 24 '14 at 10:49
  • \$\begingroup\$ @itsbruce, What I asked for was suggestions for more idiomatic Scala constructs and hints about how to structure the code better and make it more readable. In particular, as a programmer with mostly OOP background, when experimenting with the functional style, I end up with single-line chains of function calls which to me seem very hard to read. I would definitelly like to get better at this. Thanks :) \$\endgroup\$ – Dušan Rychnovský Dec 24 '14 at 10:49
5
\$\begingroup\$

This is quite good. I have one suggestion, though:

Use traits instead of abstract classes, and since probably all your data of type Expression will be defined only in this file make it a sealed trait:

sealed trait Expression {
 // same body
}

Sealing a trait (or abstract class for that matter) has the advantage that whenever you'll do a pattern match over a value the compiler can tell you if you omitted a case. Also, using a trait has two advantages over abstract classes:

  • traits can be used to express everything that an abstract class can, with little syntactic overhead (when expressing the equivalent of class parameters). While the converse is not true (you cannot inherit, or mixin, multiple abstract classes).

  • traits are a slight performance optimization, since for any non-abstract member of a trait, the compiler literally copies those definitions in the bodies of the subclasses (not that this optimization is ever truly useful, the knowledge of how the compiler works is more important though).

Second, as a response to all suggestions that you should use the OO style more, that's really a choice that depends on the situation. By using the functional design you leave yourself vulnerable to adding new data, i.e. whenever you add a new case class you have to update every pattern match, but adding new functionality does not require you to update any of the previously defined case classes. While in OO the opposite would be true. So choosing between the two styles is really a question about leaving your code open to easy extension with respect to new data (OO), or new functionality (functional).

\$\endgroup\$
  • 1
    \$\begingroup\$ Very thoughtful analysis. ++ for recognizing that OOP isn't a silver bullet. Welcome to Code Review. \$\endgroup\$ – RubberDuck Mar 20 '15 at 9:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.