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

What is the "proper" way to write this in Java?

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

Problem

I have a class that has 5 similar methods, they just relay input parameters and return a result - best practice would be to skip the class, but that's not what I'm asking about :) This is a method from the class:

public String deleteItem(Integer itemId,String username, String kpNumber) {
        try {
            return getItemServiceFacade().deleteItem(itemId, username, kpNumber);
        } catch (RemoteException re) {
            String message = "5:unexpected error:" + re.toString();
            logger.error(message);
            return message;
        } catch (CoreException ce) {
            String message = "5:unexpected error:" + ce.toString();
            logger.error(message);
            return message;
        }
    }


Now, since all the methods are similar, each has the same catch block, each does the same things... so I'd like to rewrite this but I'm unsure how far should I go in "atomizing" the methods. One way is what I consider the "pure" way, so the new code looks like this:

public String deleteItem(Integer itemId,String username, String kpNumber) {
        try {
            return getItemServiceFacade().deleteItem(itemId, username, kpNumber);
        } catch (RemoteException re) {
            logException(re);
            return getMessage(re);
        } catch (CoreException ce) {
            logException(ce);
            return getMessage(re);
        }
    }

private String getMessage(Exception exception){
    return "5:unexpected error:" + exception.toString();
}

private void logException(Exception exception){
    logger.error(getMessage(exception));
}


but I'm also left wondering if I should combine these two methods into one method, like this

public String handleException (Exception e){
    String message = "5:unexpected error:" + exception.toString();
    logger.error(message);
    return message;
}


so what I guess I'm trying to ask is how far should you go in separation of concerns when you have clear and simple

Solution

One alternative, although I don't know if you think it's too complex, would be to apply a visitor pattern:

private interface Command {
    String apply(ItemServiceFacade facade) throws RemoteException, CoreException;
}

public String deleteItem(final Integer itemId, final String username, final String kpNumber) {
    return execute(new Command() {
        @Override
        public String apply(ItemServiceFacade facade) throws RemoteException, CoreException {
            return facade.deleteItem(itemId, username, kpNumber);
        }
    });
}

    // other methods...

private String execute(Command command) {
    try {
        return command.apply(getItemServiceFacade());
    } catch (RemoteException re) {
        String message = "5:unexpected error:" + re.toString();
        logger.error(message);
        return message;
    } catch (CoreException ce) {
        String message = "5:unexpected error:" + ce.toString();
        logger.error(message);
        return message;
    }
}


I used this pattern when faced with a similar though slightly more complicated situation (implementing optimistic locking for a few different operations).

Code Snippets

private interface Command {
    String apply(ItemServiceFacade facade) throws RemoteException, CoreException;
}

public String deleteItem(final Integer itemId, final String username, final String kpNumber) {
    return execute(new Command() {
        @Override
        public String apply(ItemServiceFacade facade) throws RemoteException, CoreException {
            return facade.deleteItem(itemId, username, kpNumber);
        }
    });
}

    // other methods...

private String execute(Command command) {
    try {
        return command.apply(getItemServiceFacade());
    } catch (RemoteException re) {
        String message = "5:unexpected error:" + re.toString();
        logger.error(message);
        return message;
    } catch (CoreException ce) {
        String message = "5:unexpected error:" + ce.toString();
        logger.error(message);
        return message;
    }
}

Context

StackExchange Code Review Q#12728, answer score: 3

Revisions (0)

No revisions yet.