Concatenating sets contained within a set of objects, functionally in scala

I'm just beginning in scala and I'm converting some java code into scala and trying to make it nice and functionally elegant.

I have the following code containing a method (getRequiredUploadPathKeys) which gives a union of all of the path keys required 开发者_运维知识库by all of the available path templates in MyClass.

trait PathTemplate {
    def getRequiredPathKeys:Set[PathKey]
}

class MyClass(accessPaths:Set[PathTemplate], targetPath:PathTemplate){

    def getAllRequiredPathKeys: Set[PathKey] = {
        val pathKeys = HashSet[PathKey]()
        pathKeys.addAll(targetPath.getRequiredPathKeys)

        for (accessTemp <- accessPaths) {
            pathKeys.addAll(accessTemp.getRequiredPathKeys)
        }
        return pathKeys
    }
}

This method just seems like it could be much more concise in scala. Can anyone point me towards how to get it there?

Thanks, Paul

---

def getAllRequiredPathKeys = 
  (accessPaths + targetPath).flatMap(_.getRequiredPathKeys)

Or with for

def getAllRequiredPathKeys = 
  for(path <- accessPaths + targetPath; 
      key <- path.getRequiredPathKeys) yield key

---

The curious thing here is that you are using Java's HashSet -- there's no addAll in Scala -- and no need for that.

So I'm going to comment on various design decisions and how they differ from Java to Scala. First,

    val pathKeys = HashSet[PathKey]()

Idiomatic Scala doesn't usually refer to the class implementing the interface, since the interface has a factory. So you'd usually just use Set[PathKey](). Also, this is obviously a Scala factory, since there's no new keyword -- which means the rest of the code wouldn't work, as there's no addAll in Scala's Set.

    pathKeys.addAll(targetPath.getRequiredPathKeys)

Here you don't use targetPath.getRequiredPathKeys directly because Java's Set is mutable. The default Scala Set is immutable, which makes this useless -- you can just use targetPath.getRequirePathKeys as the base set, without having to add its elements to another collection.

Performance-wise, Scala -- like functional languages -- use persistent datastructures in its immutable collections implementations. That means it reuses parts of one data structure to create a derived data structure, instead of copying everything every time you create a new data structure. This is only possible because of the immutability guarantees.

At any rate, you could just do this:

val pathKeys = targetPath.getRequiredPathKeys

Next,

    for (accessTemp <- accessPaths) {
        pathKeys.addAll(accessTemp.getRequiredPathKeys)
    }

First, the idiomatic use of for comprehensions is to return a value, ie, for-yield. The above use should be restricted just to the side-effectful parts of your code. To be more clear, the above could be done in two steps:

    // Get all required Path Keys
    val requiredPathKeys = for {
        accessPath <- accessPaths
        requiredPathKey <- accessPath.getRequiredPathKeys
    } yield requiredPathKey

    // Then add them
    pathKeys.addAll(requiredPathKeys)

However, given that you don't addAll stuff to Scala's Set, you'd either need to make pathKeys a var, or use a mutable data structure, or simply change the order:

    val requiredPathKeys = for {
        accessPath <- accessPaths
        requiredPathKey <- accessPath.getRequiredPathKeys
    } yield requiredPathKey

    val pathKeys = targetPath.getRequiredPathKeys ++ requiredPathKeys

Notice, however, the repetition of getRequiredPathKeys. Functional programmers abhor repetition. Sometimes overly so, in my opinion, but, in this case, it can be easily removed:

    val pathKeys = for {
        accessPath <- accessPaths + targetPath
        requiredPathKey <- accessPath.getRequiredPathKeys
    } yield requiredPathKey

The above code suggests another improvement. If you see this question about Scala's for comprehensions, you'll see that the above is equivalent to

    val pathKeys = (accessPaths + targetPath).flatMap( accessPath =>
        accessPath.getRequiredPathKeys.map( requiredPathKey => requiredPathKey ))

The map at the end is redundant, since it doesn't do anything. In fact, whenever a yield returns just a simple identifier, there's a redundant map in the expression. Removing it gives us:

    val pathKeys = (accessPaths + targetPath).flatMap(accessPath =>
        accessPath.getRequiredPathKeys)

Or, using Scala's anonymous function parameter syntax,

    val pathKeys = (accessPaths + targetPath).flatMap(_.getRequiredPathKeys)

Finally,

    return pathKeys

The keyword return in Scala is used to designate an exception in the work flow -- a point at which the method's execution is aborted instead of going through to the end. Not that this isn't without use, but, like exceptions themselves, it shouldn't be used without cause. Here, you should have used

    pathKeys

But, at this point, your code became just these two lines:

    val pathKeys = (accessPaths + targetPath).flatMap(_.getRequiredPathKeys)
    pathKeys

Which makes the val assignment completely redudant. You can thus reduce it to:

    (accessPaths + targetPath).flatMap(_.getRequiredPathKeys)

So the method becomes

def getAllRequiredPathKeys: Set[PathKey] = {
    (accessPaths + targetPath).flatMap(_.getRequiredPathKeys)
}

Now, I know I said "finally" back there, but there's one more tidbit of Scala convention to apply. Whenever a method contains a single expression instead of multiple statements, the convention is to ommit the curly braces. In other words, do this:

def getAllRequiredPathKeys: Set[PathKey] =
    (accessPaths + targetPath).flatMap(_.getRequiredPathKeys)

Or, if space allowed it, put everything on a single line. In fact, if you removed getAllRequiredPathKeys type, it would fit nicely. On the other hand, explicit return types on public method are encouraged.

So this will give you a Scala's immutable Set. Let's say your input and output must be Java's Set. In that case, I don't see anything that could be done to simplify your code, unless you were willing to convert between Java and Scala sets. You could do that like this:

trait PathTemplate {
    def getRequiredPathKeys:java.util.Set[PathKey]
}

import scala.collection.JavaConverters._

class MyClass(accessPaths:java.util.Set[PathTemplate], targetPath:PathTemplate){
    def getAllRequiredPathKeys: java.util.Set[PathKey] =
        (accessPaths.asScala + targetPath).flatMap(_.getRequiredPathKeys.asScala).asJava
}

This, however, uses Scala's mutable Set, which means each operation creating a new Set will copy all the contents of the old Set.

---

Do you mean something like:

def getAllRequiredPathKeys = (accessPaths map { _.getRequiredPathKeys }).flatten ++ targetPath.getRequiredPathKeys