Gathering emails, phones, URIs, and tag related to a room

Question

I've made a strange method with multiples conditionals, creating a lot of Lists and I was wondering if I could write it in a better and more legible way.

def findRooms(room:Room) = {
    ...
    if(room.id != null && !room.id.isEmpty)
      query+=("_id" -> room.id)
    if(room.members != null && room.members.length > 0){
      val emails = scala.collection.mutable.MutableList[String]()
      val phones = scala.collection.mutable.MutableList[String]()
      val forwardUris = scala.collection.mutable.MutableList[String]()
      for(member <- room.members){
        if(member.email != null && member.email != None)
          emails+=member.email.get
        if(member.forwardUrl != null && member.forwardUrl != None)
          forwardUris+=member.forwardUrl.get
        if(member.phoneNumber != null && member.phoneNumber != None)
          phones+=member.phoneNumber.get
      }

      if(emails.size > 0)
        query+=("members.user.email" -> MongoDBObject("$all" -> emails))

      if(phones.size > 0)
        query+=("members.user.phoneNumber" -> MongoDBObject("$all" -> phones))

      if(forwardUris.size > 0)
        query+=("members.user.forwardUrl" -> MongoDBObject("$all" -> forwardUris))
    }

    if(room.tags != null && room.tags.length > 0){
      val tags = scala.collection.mutable.MutableList[String]()
      for(tag <- room.tags){
        tags+=tag
      }
      if(tags.size > 0){
        query+=("tags" -> MongoDBObject("$in" -> tags))
      }
    }
    ...
}

Answer 1

There's really not much we can help you, given the low level of detail in the question.

There are somethings you have in this code snippet that are generally considered a Very Bad Idea, so I'll give what help I can in that regard.

Null Checks

There are a bunch of these types of checks in the posted code:

if(room.id != null && !room.id.isEmpty)

If you have any control at all over Room you should be using Option instead. If you have no control over Room, define these methods using an implicit. These checks should only every be made in one place, not every place that accesses the code.

Assuming that id is a String (just a guess). The signature should in Room should be:

def idOption: Option[String] = Option(this.id).filter(_.nonEmpty)

Which would drop the code in the snippet to:

room.idOption.foreach(id => query += ("_id" -> id))

Much simpler.

Premature Optimizations

If you ever have enough members in a room for the MutableList optimization to gain you anything, you'll probably be exceeding the length of the Mongodb query. This needlessly complicates your code.

Ironically, any gains are lost when you forgot to use nonEmpty rather than .size > 0, which needs to iterate through the lists you just made to avoid iterating through the lists.

It looks like the accessors for the Member attributes already return Option values, so this can be much more readably rendered like this:

val emails = for {
  member <- room.members
  email <- member.email
} yield email

val phones = for {
  member <- room.members
  phone <- member.phoneNumber
} field phone

// 'uri' is not the plural of 'url', they are not
// actually equivalent, please don't conflate them.
val forwardUrls = for {
  member <- room.members
  url <- member.forwardUrl
} yield url

if(emails.nonEmpty)
  query+=("members.user.email" -> MongoDBObject("$all" -> emails))

if(phones.nonEmpty)
  query+=("members.user.phoneNumber" -> MongoDBObject("$all" -> phones))

if(forwardUrls.nonEmpty)
  query+=("members.user.forwardUrl" -> MongoDBObject("$all" -> forwardUrls))

Redundant Code

Much of the tags block is a no-op. It condenses down to just this:

if (room.tags.nonEmpty) {
  query+=("tags" -> MongoDBObject("$in" -> room.tags))
}