CODEWITHSUNDEEP
scala
Adam Davies
Adam Davies

Reputation: 2812

Scala proper use of break to return

I have the following code:

private def hasRole(role: String): Boolean = {
  var hasRole = false;
  if(getUserDetails.isDefined){

    // getAuthorities returns java.util.Collection<GrantedAuthority>
    val authorities: util.Collection[_ <:GrantedAuthority] = getUserDetails.get.getAuthorities
    // Wrap the collection is a Scala class 
    val authoritiesWrapper = JCollectionWrapper.apply(authorities);
    for(authority <- authoritiesWrapper.iterator){
      if(authority.getAuthority == role){
        hasRole = true;
        scala.util.control.Breaks.break  
      }
    }
  }
  hasRole
}

The question is, is scala.util.control.Breaks.break the correct way toreturn when I've found the role? Doesn't look right to me.

Upvotes: 1

Views: 2952

Answers (5)

Adam Davies
Adam Davies

Reputation: 2812

Thanks Everyone, based on @Rex Kerr answer I now have this:

private def hasRole(role: String): Boolean = {
  var hasRole: Boolean = false;
  if(getUserDetails.isDefined){
    val authorities: util.Collection[_ <:GrantedAuthority] = getUserDetails.get.getAuthorities
    val authoritiesWrapper = JCollectionWrapper.apply(authorities);
    hasRole = authoritiesWrapper.iterator.exists(_.getAuthority == role)
  }
  hasRole
}

which seems to look and feel right. I'm using exists to see of the role exists in the collection and then returning that result. By default false is returned if the user is not defined (not logged in).

Please comment if this is still not perfect.

Upvotes: 0

Randall Schulz
Randall Schulz

Reputation: 26486

I can only offer you generalities, but I hope it's at least a little helpful...

Unfortunately, your sample code contains free identifiers, so it's not possible for me to understand what it does without guessing and assuming.

You're thinking about the problem wrong. What is it you're doing here? You're finding a element of a collection. Use the library! It has all manner of such things off-the-shelf.

When it comes to dealing with Option[Something] the preferred approach is to map over it. If it's a None, you get a None out. If it's Some(thing) then the function you pass to map will be applied to thing and the result will be Some(what-your-function-returned-for-thing).

Alternatively, and what newcomers to Scala often find more palatable, you can use pattern matching on an Option to effectively distinguish the None case from the Some(thing) case.

When it comes to dealing with Java libraries, it's best to push the conversions from and to the Java collections to the very periphery of your code and keep the bulk of your code idiomatic Scala using native Scala collections.

The same goes for nulls coming from Java. Turn them into Option at the earliest possible time. As a convenience, the Option(thing) factory will turn a thing that is null into a None and wrap a non-null thing in a Some

Addendum

The upshot is that you really should not be using these control flow mechanisms in this code. They're all based on exceptions (other than return) and are rather at odds with Scala's emphasis on using functional programming. The library supports a clean, succinct, efficient implementation of the essetial logic you're trying for. Don't go against the grain like this.

Upvotes: 1

Rex Kerr
Rex Kerr

Reputation: 167911

If you want to use breakable, you need to do it like so:

import scala.util.control.Breaks._
breakable {
  for (i <- 0 to 10000) { if (i>3) break }
}

But if you find yourself doing that often, you're probably not using the collections library to its full extent. Try instead

authoritiesWrapper.iterator.exists(_.getAuthority == role)

Also, in the example you gave, you could also

if (authority.getAuthority == role) return true

When to choose which? In general, you should use the control-flow options in the collections library if you can. They generally are the fastest and clearest. Wait, fastest--why is that? Both break and return (from within a for or other context that requires a closure--basically anything but if and while and match) actually throw a stackless exception which is caught; in the break case it's caught by breakable and in the return case it's caught by the method. Creating a stack trace is really slow, but even stackless exceptions are kind of slow.

So using the correct collections method--exists in this case--is the best solution.

Upvotes: 9

Brian
Brian

Reputation: 20295

The question is, is scala.util.control.Breaks.break the correct way toreturn when I've found the role? Doesn't look right to me.

Since your just looking for the first instance of authority.getAuthority == role you can use find which does exactly that and is the idiomatic way to do this. 

authoritiesWrapper.iterator.find(authority => authority.getAuthority == role)

or more concisely

authoritiesWrapper.iterator.find(_.getAuthority == role)

These return an Option type which you can get the value of authority from if it exists.

Upvotes: 1

djechlin
djechlin

Reputation: 60848

Why not just return true and return false replacing hasRole?

Upvotes: 0

Related Questions