Polish AbstractDependsOnBeanFactoryPostProcessor to avoid using try {} catch
block
Comment From: nosan
Hi @snicoll,
Thank you very much for your feedback.
The idea of this PR is avoiding unnecessary try { } catch
block and don't rely on exception throwing.
Comment From: nosan
Maybe it makes sense to add support for a Programmatically registered singleton.
Currently, it is not supported by AbstractDependsOnBeanFactoryPostProcessor
.
Comment From: snicoll
@nosan you lost me I am afraid. We went from polishing to something that's not supported. I guess that's what I was trying to get at with https://github.com/spring-projects/spring-boot/pull/19039#discussion_r347849409
Comment From: nosan
You are right @snicoll,
Sorry about my last comment, it seems I have mixed two different things in one PR.
The initial request was about avoiding usage of the try-catch block and don't rely on NoSuchBeanDefinitionException
.
My concern that currently instead of use a containsBeanDefinition + getBeanDefinition
, only getBeanDefinition
is used. Of course, it is quite a rare case when BeanFactory has more than one parent in the hierarchy, and probably there is no performance issue here, but I feel that this PR makes this part more readable and understandable.
For instance:
#1
private static String getText() {
Map<String, Object> maps = new LinkedHashMap<>();
try {
return maps.get("key").toString();
}
catch (NullPointerException ex) {
return "";
}
}
#2
private static String getText() {
Map<String, Object> maps = new LinkedHashMap<>();
Object val = maps.get("key");
return (val != null) ? val.toString() : "";
}
I prefer the second approach, and I want to achieve something similar here.
Comment From: snicoll
Thanks for the PR @nosan. We've discussed this at the team meeting and while removing flow control in exception handling brings is the right thing to do, we're not keen to create the low-level framework exception ourselves as I've mentioned in the review.
There might be another way however, I've created #19164 to investigate that.