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

Check smsc type of batch and submits it to the proper queue

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

Problem

Here, the for loop loops over a arraylist and checks smsc type of batch and submits it to the proper queue.

Is there a way to rewrite this piece of code in a better way? Is there any way to eliminate the if/else?

for (Batch batch : input.getBatches()){
ArrayList arrSentsms = batch.getlist();
String smsc =batch.getSmsc();
    if (smsc.equals("v")) {
            SubmitToQueueV.sendJMSMessage(arrSentsms, priority);
        } else if (smsc.equals("s")) {

            SubmitToQueueS.sendJMSMessage(arrSentsms, priority);
        } else if (smsc.equals("fake1")) {

            SubmitToQueueFake.sendJMSMessage(arrSentsms, priority);
        } else if (smsc.equals("con")) {

            SubmitToQueueCon.sendJMSMessage(arrSentsms, priority);

        } else if (smsc.equals("ce")) {

            SubmitToQueueCEcns.sendJMSMessage(arrSentsms, priority);

        } else if (smsc.equals("cli")) {

            SubmitToxQueueCli.sendJMSMessage(arrSentsms, priority);

        } else if (smsc.equals("pel")) {
            SubmitTQueuePel.sendJMSMessage(arrSentsms, priority);

        } else {
            LOG.debug("smsc" + " smsc not found");
        }
    }
}

Solution

I assume you have (or can easily extract) an interface JMSQueue for this answer.

That's not really an unreasonable assumption I'd say, if you need help with that just drop me a note.

You're using the wrong tool here. What you have is a "mapping" between a string and a certain JMS-Queue.

Let's put that into code:

private Map queues = new HashMap<>();

// [...]
for (Batch batch : input.getBatches()) {
    JMSQueue responsibleQueue = queues.get(batch.getSmsc());
    if (responsibleQueue == null) {
        // queue not found. Log or whatever and then do the next batch
        continue; 
    }
    responsibleQueue.sendJMSMessage(batch.getList(), priority);
}


Note that I've done some small changes on your code:

  • Respect indentation and whitespace around operators. Your IDE can help you with that.



  • Avoid unnecessary variables. They only make it harder to understand the code. Don't have everything inlined, but exercise care when creating variables.



  • Respect the naming conventions (talking about getlist)

Code Snippets

private Map<String, JMSQueue> queues = new HashMap<>();

// [...]
for (Batch batch : input.getBatches()) {
    JMSQueue responsibleQueue = queues.get(batch.getSmsc());
    if (responsibleQueue == null) {
        // queue not found. Log or whatever and then do the next batch
        continue; 
    }
    responsibleQueue.sendJMSMessage(batch.getList(), priority);
}

Context

StackExchange Code Review Q#139714, answer score: 6

Revisions (0)

No revisions yet.