HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Connecting to RabbitMQ

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
connectingrabbitmqstackoverflow

Problem

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 {
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" -> "er

Solution

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.

Context

StackExchange Code Review Q#55312, answer score: 6

Revisions (0)

No revisions yet.