views:

124

answers:

7

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
A: 

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.

Brian Agnew
Yes, I thought about using the reflection, but I really don't like this solution (in this case)...
romaintaz
+1  A: 

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.

bruno conde
Yes, that's an idea, however it means that I would create 20 new methods...
romaintaz
IMO, this is a bad idea. Don't use an object, use a wrapper that stores the return of the getFoo(), and a code (enum/int/bool whatever) that indicate if the retrieving went good.
Clement Herreman
I only used Object because I don't know what the return type of getFoo or getBar is. Even so, why the downvote?
bruno conde
Using the reflection approach you could tag all the methods you want to invoke with a common annotation, then write some tooling to iterate over those tagged methods, invoking them and catching the exceptions. A bit like http://stackoverflow.com/questions/1400305/tagging-methods-and-calling-them-from-a-client-object-by-tag/1400740#1400740
Tarski
A: 

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!

Tom Hawtin - tackline
Why the comment about reflection ? IS that a stylistic issue, or is there a reason why it won't work ? (I know I've suggested reflection, and I've admitted it's not nice, but I believe it *will* work)
Brian Agnew
Reflection is a good indication that something very, very wrong is going on.
Tom Hawtin - tackline
The first option looks really bad unless Lambdas arrive to Java.If you have a code formating standard in your team (and they might use IDE default).Reflection itself is not bad, the problem is that Java Language does not have a strong typed Method, with good syntax(delegate in C#).
Dennis Cheung
Method is not supposed to be anything like delegates.
Tom Hawtin - tackline
+1  A: 

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);
}
Andreas_D
A: 

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);
Gishu
+1  A: 

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

Sam Barnum
Or, use bruno's suggestion of moving the try/catch into cover methods, that looks a little nicer
Sam Barnum
+1 No point over-elaborating something for the sake of aesthetics.
banjollity
+2  A: 

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"));

Dennis Cheung