Calculator implemented in a MessageDrivenBean

Is it right? any improvements? different approach...?

```
import java.util.Properties;

import javax.ejb.EJBException;
import javax.ejb.MessageDrivenBean;
import javax.ejb.MessageDrivenContext;
import javax.jms.JMSException;
import javax.jms.Message;
import javax.jms.MessageListener;
import javax.jms.Queue;
import javax.jms.QueueConnection;
import javax.jms.QueueConnectionFactory;
import javax.jms.QueueSender;
import javax.jms.QueueSession;
import javax.jms.Session;
import javax.jms.TextMessage;
import javax.naming.Context;
import javax.naming.InitialContext;
import javax.naming.NamingException;

public class CalculatorImpl implements MessageDrivenBean,MessageListener {

private static Properties properties;
private static Context jndiContext;
private static final String
//JNDI_PROVIDER_CONTEXT_FACT = "weblogic.jndi.WLInitialContextFactory" //weblogic specific value
JNDI_PROVIDER_CONTEXT_FACT = "org.jnp.interfaces.NamingContextFactory";
;
private static final String
//JNDI_PROVIDER_URL = "t3://localhost:7003"//weblogic specific value
JNDI_PROVIDER_URL = "jnp://localhost:7003"//Mention valid provider url
;
private static final String
JNDI_SECURITY_PRINCIPAL = "weblogic"//Mention valid principal value
;
private static final String
JNDI_SECURITY_CREDENTIALS = "weblogic12"//Mention valid credential value
;

private String splitMsg,expr1,expr2 = null;
private double trigVal = 0.0;
private boolean negPostNum = false;
private int subIdx,addIdx,postIdx,preIdx = 0;

//----------------------------------------------------------------------------------------
public CalculatorImpl(){

properties = new Properties();
properties.put (Context.PROVIDER_URL, JNDI_PROVIDER_URL);
properties.put(Context.INITIAL_CONTEXT_FACTORY,JNDI_PROVIDER_CONTEXT_FACT);
properties.put(Context.SECURITY_PRINCIPAL,

Some notes:

-
The get* methods do not look too easy to understand. I bet that there is some library which could do the calculations for you (here a question about it on Stack Overflow), so using one would improve both readability and maintainability of your code (since you need less code) and save you a few days too.

See also: Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

-
From Code Complete, 2nd Edition, p761:


Use only one data declaration per line


[...]
It’s easier to modify declarations because each declaration is self-contained.


[...]


It’s easier to find specific variables because you can scan a single
column rather than reading each line. It’s easier to find and fix
syntax errors because the line number the compiler gives you has
only one declaration on it.

-

} catch (final EJBException e) {
     e.printStackTrace();
     throw new EJBException();
 } ...

Logging and rethrowing an exception usally leads to logging it twice (application servers logs system output too) which means harder maintenance (two stacktrace for every error). Furthermore, if you rethrow an exception set its cause:

} catch (final EJBException e) {
    throw new EJBException(e);
} ...

It helps debugging a lot.

Other references:

• Avoid printStackTrace(); use a logger call instead

• Why is exception.printStackTrace() considered bad practice?

-
l_message is unnecessary, you could use p_message instead of it.

-
The declaration of l_result could be inside the try block. Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method, declare them where they are first used. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

final String result = calculate(requestStr);
writeResponse(result, correlationID);

StackExchange Code Review Q#40453, answer score: 3