1
\$\begingroup\$

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))
      }
    }
    ...
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$ Jan 22, 2015 at 12:27

1 Answer 1

3
\$\begingroup\$

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))
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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