Description
Log4j 2.24..3
The DefaultConfigurationBuilder is not final but it practically not extensible because access to the root and other components is private.
This implementation could benefit from a bit more adherence to the Open/Closed Principle: - Open for Extension, Closed for Modification
public class DefaultConfigurationBuilder<T extends BuiltConfiguration> implements ConfigurationBuilder<T> {
private static final String INDENT = " ";
private final Component root = new Component();
private Component loggers;
private Component appenders;
private Component filters;
private Component properties;
private Component customLevels;
private Component scripts;
private final Class<T> clazz;
private ConfigurationSource source;
private int monitorInterval;
private Level level;
private String destination;
private String packages;
private String shutdownFlag;
private long shutdownTimeoutMillis;
private String advertiser;
private LoggerContext loggerContext;
private String name;
Since the components (and other attributes) are private, it is not possible to extend this (for example to add a custom component type) - this would be important because a BuiltConfiguration only gets the root component as an argument in the constructor instantiated by reflection.
It also might be useful if this inittialization of the root component was moved out of the constructor to a protected / overridable method.
public DefaultConfigurationBuilder(final Class<T> clazz) {
if (clazz == null) {
throw new IllegalArgumentException("A Configuration class must be provided");
}
this.clazz = clazz;
final List<Component> components = root.getComponents();
properties = new Component("Properties");
components.add(properties);
scripts = new Component("Scripts");
components.add(scripts);
customLevels = new Component("CustomLevels");
components.add(customLevels);
filters = new Component("Filters");
components.add(filters);
appenders = new Component("Appenders");
components.add(appenders);
loggers = new Component("Loggers");
components.add(loggers);
}
i.e.
protected Component getRootComponent()protected void initializeComponents()
maybe a method like this would help?
protected Component getOrCreateComponent(String name) {
// Retrieve an existing component or create a new one
return this.root.components.computeIfAbsent(name, key -> new Component(key));
}
This would significantly simplify the constructor.
public DefaultConfigurationBuilder(final Class<T> clazz) {
if (clazz == null) {
throw new IllegalArgumentException("A Configuration class must be provided");
}
this.clazz = clazz;
properties = this.getOrCreateComponent("Properties");
scripts = this.getOrCreateComponent("Scripts");
customLevels = this.getOrCreateComponent("CustomLevels");
filters = this.getOrCreateComponent("Filters);
appenders = this.getOrCreateComponents("Appenders");
loggers = this.getOrCreateComponentts("Loggers");
}
Another nice-to-have would be moving the instantation of the Configuratio to a separate method so that a custom constructor could be called.
For example, from:
@Override
public T build(final boolean initialize) {
T configuration;
try {
...
final Constructor<T> constructor =
clazz.getConstructor(LoggerContext.class, ConfigurationSource.class, Component.class);
configuration = constructor.newInstance(loggerContext, source, root);
}
...
}
@Override
public T build(final boolean initialize) {
T configuration;
try {
...
configuration = constructor.newInstance(loggerContext, source, root);
}
...
}
protected T createNewConfiguration() {
final Constructor<T> constructor =
MyCustomClass.getConstructor(LoggerContext.class, ConfigurationSource.class, Component.class, FooBar.class);
return constructor..newInstance(loggerContext, source, root, this.foobar);
}
This would allow passing custom information to a custom configuration's constructor.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status