Refactoring a method which is calling others methods that throws Exception

Question

I have several (more than 20) methods (getXXX()) that may throw an exception (a NotCalculatedException) when they are called.

In another method, I need to access the results given by these methods. For the moment, I have an horrible code, which looks like:

public void myMethod() {
    StringBuffer sb = new StringBuffer();
    // Get 'foo' result...
    sb.append("foo = ");
    try {
        sb.append(getFoo());
    } catch (NotCalculatedException nce) {
        sb.append("not calculated.");
    }
    // Get 'bar' result...
    sb.append("\nbar = ");
    try {
        sb.append(getBar());
    } catch (NotCalculatedException nce) {
        sb.append("not calculated.");
    }
    ...
}

Without modifying the getXXX methods (thus they must keep their throws NotCalculatedException), how would you refactor / simplify myMethod() to make it looks better?

Please note that this project is still using Java 1.4 :(

---

EDIT

I cannot put all the getXXX() methods in the try { ... } block, as the StringBuffer will be incomplete if one method throws a NotCalculatedException.

public void myMethod() {
    StringBuffer sb = new StringBuffer();
    try {
        sb.append("foo = ");
        sb.append(getFoo());
        sb.append("\nbar = ");
        sb.append(getBar());
    } catch (NotCalculatedException nce) {
        sb.append("not calculated.");
    }
    ...
}

In others words, if getFoo() throws a NotCalculatedException, I want to have this kind of output :

foo = not calculated
bar = xxx
...

If I put everything in one single try { ... }, I will have that output, which I don't want to get:

foo = not calculated

Answer 1

I don't think you should use NotCalculatedException to control the logic.

But I have a few idea about it.

1. You need another getter method

   sb.append(this.getFoo("not calculated"));

2. Create hasValue method

   sb.append(hasFoo()?this.getFoo():"not calculated");

3. Create a generic getter method

   sb.append(this.getValueByName("foo"));

Answer 2

For each getXXX you could add a getXXXOrDefault() that wraps the exception an returns the value of getXXX or "not calculated.".

public void myMethod() {    
        StringBuffer sb = new StringBuffer();    
        // Get 'foo' result...    
        sb.append("foo = ");
        sb.append(getFooOrDefault());
        // Get 'bar' result...    
        sb.append("\nbar = ");
        sb.append(getBarOrDefault());
        // ...
}

public Object getFooOrDefault() {
        try {
                return getFoo();
        } catch() {
                return "not calculated.";
        }
}

Or ... Use Reflection

public Object getValueOrDefault(String methodName) {
        try {
                // 1 . Find methodName
                // 2 . Invoke methodName 
        } catch() {
                return "not calculated.";
        }
}

But I think I still prefer the first option.

Answer 3

My suggestion is more code, but improved readibility for the myMethod:

public void myMethod() {
    StringBuilder resultBilder = new StringBuilder();

    resultBuilder.append("foo=");
    appendFooResult(resultBuilder);
    resultBuilder.append("\nbar=");
    appendBarResult(resultBuilder);

    ...
}

private void appendFooResult(StringBuilder builder) {
    String fooResult = null;
    try {
        fooResult = getFoo();
    } catch (NotCalculatedException nce) {
        fooResult = "not calculated.";
    }
    builder.append(fooResult);
}

private void appendBarResult(StringBuilder builder) {
    String barResult = null;
    try {
        barResult = getBar();
    } catch (NotCalculatedException nce) {
        barResult = "not calculated.";
    }
    builder.append(barResult);
}

Answer 4

I think you should leave your code as is. It's verbose, but very easy to tell what it does, and it behaves correctly.

Answer 5

You can store "foo", "bar" etc. in an array. Loop around these, print each one out and then use reflection to find/call the corresponding getFoo(), getBar(). Not nice, I admit.

See the Method object for more information.

EDIT: Alternatively, make use of AspectJ to surround each getXXX() method call for that object and catch the exception.

Answer 6

You could use the Execute Around idiom. Unfortunately the Java syntax is verbose, so it isn't much of a win in simple cases. Assuming NotCalculatedException is an unchekd exception.

appendThing(sb, "foo = ", new GetValue() { public Object get() {
     return getFoo();
}});
appendThing(sb, "bar = ", new GetValue() { public Object get() {
     return getBar();
}});

Another ugly method would combine a loop and switch:

int property = 0;
lp: for (;;) {
    String name = null; // Ugh.
    try { 
        final Object value;
        switch (property) {
            case 0: name= "foo"; value = getFoo(); break;
            case 1: name= "bar"; value = getBar(); break;
            default: break lp;
        }
        ++property;
        sb.append(name).append(" = ").append(value).append('\n');
    } catch (NotCalculatedException exc) {
        sb.append(name).append(" = ").append("not calculated.\n");
    }
}

Alternatively, have an enum and a switch for each parameter. Just don't use reflection!

Answer 7

Seems like Java doesn't have delegates out of the box like C#- however Google showed me that there are ways to roll your own. So the following may be something to try..

public static PrintProperty(JavaDelegateWithAName del, StringBuilder collector)
{
  try
  {
    collector.append( del.Name+ " = " );
    collector.append( del.Target.Invoke() );
  }
  catch(NotCalculatedException nce) 
  { collector.append("NotCalculated"); }
}

... main

foreach(JavaDelegateWithAName entry in collectionOfNamedJavaDelegates)
  SomeUtilityClass.PrintProperty(entry, sb);