logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Sicker <boa...@gmail.com>
Subject Re: Something's not right with the new plugin builder pattern
Date Thu, 29 May 2014 14:12:17 GMT
I was thinking of using a different annotation for the builder attributes
(at least for PluginAttribute) because the fields themselves have their own
default values. I'm going to refactor that soon to show what I mean. I
noticed the smell, too. ;)


On 28 May 2014 21:58, Gary Gregory <garydgregory@gmail.com> wrote:

> The "normal" plugin pattern looks like this:
>
>     @PluginFactory
>     public static PatternLayout createLayout(
>             @PluginAttribute(value = "pattern", defaultStringValue =
> DEFAULT_CONVERSION_PATTERN) final String pattern,
>             @PluginConfiguration final Configuration config,
>             @PluginElement("Replace") final RegexReplacement replace,
>             @PluginAttribute(value = "charset", defaultStringValue =
> "UTF-8") final Charset charset,
>             @PluginAttribute(value = "alwaysWriteExceptions",
> defaultBooleanValue = true) final boolean alwaysWriteExceptions,
>             @PluginAttribute(value = "noConsoleNoAnsi",
> defaultBooleanValue = false) final boolean noConsoleNoAnsi,
>             @PluginAttribute("header") final String header,
>             @PluginAttribute("footer") final String footer) {
>
> I get that and I'm used to it.
>
> But now I see that the impl has changed:
>
>         return newBuilder()
>             .withPattern(pattern)
>             .withConfiguration(config)
>             .withRegexReplacement(replace)
>             .withCharset(charset)
>             .withAlwaysWriteExceptions(alwaysWriteExceptions)
>             .withNoConsoleNoAnsi(noConsoleNoAnsi)
>             .withHeader(header)
>             .withFooter(footer)
>             .build();
>
> OK, I can see that this is a translation from one style to the other.
>
> But then I see:
>
>     /**
>      * Custom PatternLayout builder. Use the {@link
> PatternLayout#newBuilder() builder factory method} to create this.
>      */
>     public static class Builder implements
> org.apache.logging.log4j.core.util.Builder<PatternLayout> {
>
>         // FIXME: it seems rather redundant to repeat default values (same
> goes for field names)
>         // perhaps introduce a @PluginBuilderAttribute that has no values
> of its own and uses reflection?
>
>         @PluginAttribute(value = "pattern", defaultStringValue =
> PatternLayout.DEFAULT_CONVERSION_PATTERN)
>         private String pattern = PatternLayout.DEFAULT_CONVERSION_PATTERN;
>
>         @PluginConfiguration
>         private Configuration configuration = null;
>
>         @PluginElement("Replace")
>         private RegexReplacement regexReplacement = null;
>
>         @PluginAttribute(value = "charset", defaultStringValue = "UTF-8")
>         private Charset charset = Charsets.UTF_8;
>
>         @PluginAttribute(value = "alwaysWriteExceptions",
> defaultBooleanValue = true)
>         private boolean alwaysWriteExceptions = true;
>
>         @PluginAttribute(value = "noConsoleNoAnsi", defaultBooleanValue =
> false)
>         private boolean noConsoleNoAnsi = false;
>
>         @PluginAttribute("header")
>         private String header = null;
>
>         @PluginAttribute("footer")
>         private String footer = null;
>
> Hold the phone people, the duplication of annotations is a big code smell
> IMO.
>
> What can be done about this? Duplication is a guaranteed recipe for code
> to get out of sync.
>
> What does this builder do for users?
>
> Gary
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>



-- 
Matt Sicker <boards@gmail.com>

Mime
View raw message