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 <garydgreg...@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: garydgreg...@gmail.com | ggreg...@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 <boa...@gmail.com>
