Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I would like a review for this connection to RabbitMQ. I just developed it and seems to be working well, but I would like another set of eyes on it before putting this on the sever.

package models

import com.rabbitmq.client.Connection
import com.rabbitmq.client.ConnectionFactory
import com.rabbitmq.client.ConnectionFactory
import com.rabbitmq.client.MessageProperties

import anorm.SQL
import anorm.sqlToSimple
import anorm.toParameterValue
import play.api.Play.current

object RabbitMQConnection {
  private var connection: Connection = null
  def getConnection(ss:Connection): Connection = {
    println(ss+"  connection <<<<<<<<<<<<<<<<<<<<<<<<")
    connection=ss
    connection match {
      case null => {
        val factory = new ConnectionFactory()
        println("waiting for new connection")
        factory.setHost("172.22.22.222")
        println("host setted")
        connection = factory.newConnection()
        println("connection created")
        connection
      }
      case _ =>{
        println("connection is not null")
        connection
      }
    }
  }
}

object RMQ {
  var connection = RabbitMQConnection.getConnection(null)

  def setQ(qName: String, message: String) = {
    println("ping received")
    try {
      println(connection)
      if (connection != null) {
        if (connection.isOpen()) {
          println("connection is open")
        } else {
          connection = RabbitMQConnection.getConnection(null)
          println("connection is new "+connection)
        }
        println("connetion is ready to use")
        val channel = connection.createChannel()
        channel.queueDeclare(qName, true, false, false, null) //suggestion 
        channel.basicPublish("", qName, MessageProperties.PERSISTENT_TEXT_PLAIN, message.getBytes())
        println("status" + channel.close())
        println("setQ complete executed for " + qName)
        Map("result" -> "success")

      } else {
        println("connection can't established to rabbit mq for =>" + qName)
        LogFile.QLogs(qName, message)
        Map("result" -> "error")
      }
    } catch {
      case e: Exception =>
        println(e.printStackTrace())
        println("Rabbit Mq Server is Down for =>" + qName)
        LogFile.QLogs(qName, message)
        Map("result" ->"error")
    }
  }

}
share|improve this question
add comment

2 Answers 2

Well, the first thing I would recommend is replacing all of those println statements with some kind of logging. In any kind of production environment, logging is something that needs more thought and care so that it is useful, without consuming resources unnecessarily. Or, if they're just for the original developer during development, maybe delete them.

I find the structure of this code to be rather odd. For all the trappings of OO programming, what you essentially have is two global public functions and one global public variable.

The function name setQ doesn't seem to say what you're actually doing - which appears to be creating a channel and publishing a message to it. publishMessage would make more sense, I think.

All of the code that checks the usability of the connection should be extracted into a single function, something along the lines of haveUsableConnection - the two if/else blocks are making it hard to see the code that is actually doing the work.

I question the hard coded IP address. Magic numbers and magic strings are frequently a bad idea.

share|improve this answer
add comment
  • Don't use null, but instead Option[Connection]. null is frowned upon in Scala.

  • It looks quite odd that getConnection(ss:Connection) takes in a Connection. That method either sets the connection (opposite of get!), or initialize and returns the connection if the argument is null. Create two different methods. It does not make much sense as it is... it looks like you just wanted to use pattern matching somewhere.

  • You can get rid of the whole object RabbitMQConnection and replace your lazy initialization of the connection with the Scala lazy keyword. (EDIT: actually, it is not clear what exactly you should do with the connection, since you also have to always check if it is still open. I'll let you figure this out, but you should be aware of Scala's lazy.)

  • Since setQ only needs the connection to create a Channel, I would just have a method to fetch the Channel. That method would check if the Connection is open and restart it if needed. (Separation of concerns.)

  • I hope all the println are just for debugging and will be removed. If you keep those, use some logging service instead, even if it just prints to console in the end. More generally, you should strive for separation of concerns and find a way to separate the code that actually does something and the logging code.

  • I would declare the return type of setQ.

  • About that return type: instead of Map("result" -> "success") and Map("result" ->"error") use Scala's Try. Try is actually a monad so you'll be able to call map and flatMap on the result of setQ.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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