Simple Calculator RESTfull API on Java

Question

I have written a small calculator REST API and I need someone to look the structure and design of what I did. I want to know how I can make it better. Here is an example with multiplication. Other operations are similar.

Task was to write RESTfull API that will take request like this /multiply/a/b/c with double type parameters a, b ,c and return a*b*c in JSON.

Structure:

me.leqada.rest
├── Application.java
├── logic
│   ├── Cache.java
│   └── Logic.java
├── model
│   ├── ActInfo.java
│   ├── Error.java
│   └── Output.java
└── service
    └── Calculator.java

Application class (Just to not use web.xml):

import me.leqada.rest.service.Calculator;
import org.apache.log4j.Logger;
import javax.ws.rs.ApplicationPath;
import java.util.HashSet;
import java.util.Set;

@ApplicationPath("/")
public class Application extends javax.ws.rs.core.Application {
    private final static Logger logger = Logger.getLogger(Application.class);

    public Set<Class<?>> getClasses() {
        final Set<Class<?>> resources = new HashSet<>();

        try {
            resources.add(Calculator.class);
        }
        catch(Exception e) {
            logger.error(e.getStackTrace());
        }
        return resources;
    }
}

Calculator class (request handlers):

import me.leqada.rest.logic.Cache;
import me.leqada.rest.logic.Logic;
import me.leqada.rest.model.ActInfo;
import me.leqada.rest.model.Output;
import org.apache.log4j.Logger;
import me.leqada.rest.model.Error;
import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.PathParam;
import javax.ws.rs.Produces;
import javax.ws.rs.core.Response;

@Path("/")
@Produces("application/json")
public class Calculator {
    private final static Logger logger = Logger.getLogger(Calculator.class);
    private Output output;
    private ActInfo resultInfo = new ActInfo();
    private final Error error = new Error();
    private final Logic logic = new Logic();

    @GET
    @Path("multiply/{a}/{b}/{c}")
    public Response multiplication(
            @PathParam("a") String textA,
            @PathParam("b") String textB,
            @PathParam("c") String textC) {
        logger.info("Requested multiplication: " + textA + " " + textB + " " + textC);
        String[] paramsText = {"multiplication", textA, textB, textC};
        // Trying to get result from cache
        // If there is no result for this parameters
        // then we need to calculate it
        resultInfo = Cache.get(paramsText, logic);
        // If error code = 0 then there is no problem with params
        if (resultInfo.getErrorCode() == 0) {
            logger.info("Returning: " + resultInfo.getResult());
            output = new Output("multiplication", resultInfo.getResult());
            return Response.status(200).entity(output).build();
        }
        else {
            logger.info("Returning Error: " + resultInfo.getErrorCode() + "|" + resultInfo.getErrorText());
            error.setError(resultInfo.getErrorCode(), resultInfo.getErrorText());
            return Response.status(400).entity(error).build();
        }
    }
}

Cache class (here we check if there any result in cache with for given parameters. If not - then we will call Logic class to calculate result):

import me.leqada.rest.model.ActInfo;
import org.apache.jcs.JCS;
import org.apache.jcs.access.exception.CacheException;
import org.apache.log4j.Logger;
import java.util.Arrays;

public class Cache {
    private final static Logger logger = Logger.getLogger(Cache.class);

    public static ActInfo get (String[] paramsText, Logic logic) {
        ActInfo resultInfo = new ActInfo();
        String action = paramsText[0];
        double[] params;
        int key =  Arrays.deepHashCode(paramsText);

        logger.info("Trying to get information from cache ...");
        logger.info(action + "|" + paramsText[1] + "|" + paramsText[2] + "|" + paramsText[3]);
        logger.info("Generated key is: " + key);

        try {
            JCS cache = JCS.getInstance("default");
            ActInfo cachedData = (ActInfo)cache.get(key);

            if (cachedData != null) {
                logger.info("Value found in cache. Returning: " + cachedData.getResult());
                // The cachedData is valid and can be used
                return cachedData;
            }
            else{
                logger.info("Value not found in cache. Trying to parse doubles from input ...");
                params = logic.parseToNum(paramsText);

                if (params == null) {
                    logger.warn("Parameter problem. Returning 400 code.");
                    resultInfo.setError(400, "Invalid parameter");

                } else {
                    for (double param : params) {
                        // If passed parameter is not fit to double
                        if (Double.isInfinite(param)){
                            logger.info("Result number is too big. Returning 402 error");
                            resultInfo.setError(402, "Too big number");
                            resultInfo.setResult(0);
                            return resultInfo;
                        }
                    }
                    logger.info("Calling act method to calculate final result ...");
                    resultInfo = logic.act(action, params);
                }
                logger.info("Final result calculated. Adding to cache and returning: " +
                        resultInfo.getResult());

                cache.put(key, resultInfo);
                return resultInfo;

            }
        } catch (CacheException e) {
            logger.error(e.getMessage());
            resultInfo.setError(500, "Server error");
            return resultInfo;
        }
    }
}

Logic class (Parsing double values from String parameters and calling arithmetic operations on them):

import me.leqada.rest.model.ActInfo;
import org.apache.log4j.Logger;

public class Logic {
    private final static Logger logger = Logger.getLogger(Logic.class);

    /**
     * Get double values from Strings. First element of paramsText is always arithmetic operation type
     * */
    public double[] parseToNum(String[] paramsText) {
        logger.info("Trying to parse parameters ...");
        double[] params = new double[3];

        for (int i = 0; i < 3; i++) {
            try {
                params[i] = Double.parseDouble(paramsText[i + 1]);
                logger.info("params[" + i + "] = " + params[i]);
            } catch (NumberFormatException ignored) {
                logger.warn("Number format exception. Returning null");
                return null;
            }
        }
        logger.info("Parameters are parsed successfully");

        return params;
    }

    /**
     * Calling arithmetic operation method depending on the action
     * */
    public ActInfo act(String action, double[] params) {
        ActInfo resultInfo = new ActInfo();
        logger.info(action + " of params: " + params[0] + ", " + params[1] + ", " + params[2]);
        switch (action) {
            // I removed other operation cases for compactness
            case "multiplication": {
                resultInfo.setResult(mult(params));
                if (Double.isInfinite(resultInfo.getResult())) {
                    logger.warn("Result number is too big. Returning 402 error");
                    resultInfo.setError(402, "Too big number");
                    resultInfo.setResult(0);
                }
                break;
            }
        }

        return resultInfo;
    }

    private double mult(double[] arr) {
        return arr[0] * arr[1] * arr[2];
    }
}

ActInfo class (Using this in Logic class):

// It must be serializable to work with JCS caching
public class ActInfo implements Serializable {
    private String errorText;
    private int errorCode;
    private double result;

    public ActInfo () {
        // Default error code is 0. It means that there is no error
        this.errorCode = 0;
    }

    public String getErrorText() {
        return errorText;
    }
    public int getErrorCode() {
        return errorCode;
    }

    public void setError(int errorCode, String errorText) {
        this.errorCode = errorCode;
        this.errorText = errorText;
    }

    public double getResult() {
        return result;
    }

    public void setResult(double result) {
        this.result = result;
    }
}

Error class (Error object returns by Calculator service in case of errors):

public class Error {
    private String errorText;
    private int errorCode;

    public String getErrorText() {
        return errorText;
    }
    public int getErrorCode() {
        return errorCode;
    }

    public void setError(int errorCode, String errorText) {
        this.errorCode = errorCode;
        this.errorText = errorText;
    }
}

Output class (If there is no error then object of this class will be converted to JSON and returned by Calculator class):

public class Output {
    private final String actionName;
    private final double result;

    public Output(String actionName, double result) {
        this.actionName = actionName;
        this.result = result;
    }

    public String getActionName() {
        return actionName;
    }
    public double getResult() {
        return result;
    }
}

Answer 1

In my opinion your classes names are justified by what they are doing, but I want to review the code for some classes: My review is based on the above code only,

Error class: This class should be immutable, If once you will get any error, then that error will have its specific code and error-text and which will not be changed, if you get some error again then it will be a new error with some diff. error code and message. You can make it immutable as the Output class.

public class Error {
    private final String errorText;
    private final int errorCode;

    public Error(int errorCode, String errorText) {
        this.errorText = errorText;
        this.errorCode = errorCode;
    }

    public String getErrorText() {
        return errorText;
    }
    public int getErrorCode() {
        return errorCode;
    }
}

ActInfo class: You should have Error class reference instead of errorCode and errorText fields, And that is the purpose of having Error class. You should provide getters and setters accordingly for error object. I don't think you need constructor also, because the default values are already NULL for Error and 0 for result:

public class ActInfo implements Serializable {
    private Error error;
    private double result;

    public Error getError() {
        return error;
    }

    public void setError(Error error) {
        this.error = error;
    }

    public double getResult() {
        return result;
    }

    public void setResult(double result) {
        this.result = result;
    }
}

Logic class: Due to above changes in Error class and ActInfo class, some code you have to refactor as below:

public class Logic{
...................
................
    case "multiplication": {
            resultInfo.setResult(mult(params));
            if (Double.isInfinite(resultInfo.getResult())) {
                logger.warn("Result number is too big. Returning 402 error");
                resultInfo.setError(new Error(402, "Too Big number"));
                resultInfo.setResult(0);
            }
            break;
        }
................
...............
    // Try to make the code generic whenever possible
    private double mult(double[] arr) {
        double result = 1;
        for(double e : arr) {
            result *= e;
        }
        return result;
    }

}

Cache class: In this class get() method is really very lengthy, its little bit hard to understand, Your method should be small enough to understand, It should do only one thing, We can make core more clean if we create the methods small by making them responsible for doing one thing and methods should be justified by their names and what they are actual doing, Here we can refactor it and extract the functionality of calculating the multiplication result, if not exists in cache, in different method, so that we can use that method somewhere else also if needed, because multiplication calculation logic is separate now. May be we can refactor like below:

public static ActInfo get (String[] paramsText, Logic logic) {
        String action = paramsText[0];
        int key =  Arrays.deepHashCode(paramsText);

        logger.info("Trying to get information from cache ...");
        logger.info(action + "|" + paramsText[1] + "|" + paramsText[2] + "|" + paramsText[3]);
        logger.info("Generated key is: " + key);

        try {
            JCS cache = JCS.getInstance("default");
            ActInfo cachedData = (ActInfo)cache.get(key);

            if (cachedData != null) {
                logger.info("Value found in cache. Returning: " + cachedData.getResult());
                // The cachedData is valid and can be used
                return cachedData;
            }
            else{
                ActInfo resultInfo = calculateMultiplicationResult(paramsText, logic);
                logger.info("Final result calculated. Adding to cache and returning: " +
                        resultInfo.getResult());
                cache.put(key, resultInfo);
                return resultInfo;

            }
        } catch (CacheException e) {
            logger.error(e.getMessage());
            ActInfo resultInfo = new ActInfo();
            resultInfo.setError(new Error(500, "Server error"));
            return resultInfo;
        }
    }

    private static ActInfo calculateMultiplicationResult(String[] paramsText, Logic logic) {
        double[] params;
        ActInfo resultInfo = new ActInfo();
        logger.info("Value not found in cache. Trying to parse doubles from input ...");
        params = logic.parseToNum(paramsText);

        if (params == null) {
            logger.warn("Parameter problem. Returning 400 code.");
            resultInfo.setError(new Error(400, "Invalid parameter"));

        } else {
            for (double param : params) {
                // If passed parameter is not fit to double
                if (Double.isInfinite(param)){
                    logger.info("Result number is too big. Returning 402 error");
                    resultInfo.setError(new Error(402, "Too big number"));
                    resultInfo.setResult(0);
                    return resultInfo;
                }
            }
            logger.info("Calling act method to calculate final result ...");
            resultInfo = logic.act(action, params);
        }
        return resultInfo;
    }

Note: As I am seeing there are lots of log statements in the code , so my advice is- Try to log minimum meaningful and required information only.