CODEWITHSUNDEEP
javadesign-patterns
Gorgan Razvan
Gorgan Razvan

Reputation: 401

How to refactor a return of the same value in both catch and try?

I've been noted by Sonar that this is a smelly code. How could I fix it?

invokeFieldAccessor(property.getField(), this.instance, theValue,
    new FieldAccessorHandler() {
        @Override
        public synchronized Object accessField(
                final Field field, final Object objectInstance,
                final Object value) {
            try {
                field.set(objectInstance, value);
            } catch (Exception e) {
                return null;
            }
            return null;
        }
    });

EDIT: The desired output is to return null with or without exception

Upvotes: 1

Views: 1773

Answers (5)

Tryliom
Tryliom

Reputation: 109

Just do this:

invokeFieldAccessor(property.getField(), this.instance, theValue,
    new FieldAccessorHandler() {
        @Override
        public synchronized void accessField(
                final Field field, final Object objectInstance,
                final Object value) {
            try {
                field.set(objectInstance, value);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    });

Upvotes: 0

marekmuratow
marekmuratow

Reputation: 404

In the catch clause you should log the exception or rethrow it. Your implementation just hides it. And besides that, returning null value is not the best approach.

Upvotes: 0

ernest_k
ernest_k

Reputation: 45329

You are returning null regardless of the outcome of the try/catch block. What makes this "smelly" is that there's a return null in the catch block, whereas the try block doesn't have any.

It's probable that there's a bug here.

new FieldAccessorHandler() {
    @Override
    public synchronized Object accessField(
            final Field field, final Object objectInstance,
            final Object value) {
        try {
            field.set(objectInstance, value);

            //Why is nothing being returned here?
        } catch (Exception e) {
            return null;
        }
        return null;
    }
});

From what is visible in this code, this method should be a void method. It's setting a value, not reading one, which is why you're returning null by default anyway.

So it's likely that you're implementing the wrong interface, or, if you have to return a value, send null only after the try/catch:

new FieldAccessorHandler() {
    @Override
    public synchronized Object accessField(
            final Field field, final Object objectInstance,
            final Object value) {
        try {
            field.set(objectInstance, value);
        } catch (Exception e) {
           logger.error(e); //log the exception
        }
        return null;
    }
});

Upvotes: 0

Andy Turner
Andy Turner

Reputation: 140494

Just remove the return from the catch:

        try {
          // ...
        } catch (Exception e) {
          // Fall through to the return afterwards...
        }
        return null;

But I suggest that it is a very bad idea to do nothing with the Exception.

It's not advisable to catch Exception, either: you should probably be catching ReflectiveOperationException.

Upvotes: 3

drum
drum

Reputation: 5651

You can use finally to do so.

try {
...
}
catch {
...
}
finally {
    return null;
}

Upvotes: 0

Related Questions