patternMinor
Connecting to RabbitMQ
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
```
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
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
All of the code that checks the usability of the connection should be extracted into a single function, something along the lines of
I question the hard coded IP address. Magic numbers and magic strings are frequently a bad idea.
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.