Of course. Yes, create() is supposed to validate.
Cheers, Paul On Mon, Jun 16, 2014 at 9:26 AM, Gary Gregory <[email protected]> wrote: > On Mon, Jun 16, 2014 at 10:23 AM, Remko Popma <[email protected]> > wrote: > >> Gary, good question. >> To be honest, I'm not that convinced that builders are by definition >> better. >> For example, if you miss a parameter with the factory method, you get a >> compilation error. >> If you forget to call one of the builder.setValue(value) methods, you >> may never notice... >> > > Presumably, the builders would/should validate on build()/create(), unless > the ctor of the target object already does that, which it should as well? > > Gary > >> >> I see the argument that builders give names to the arguments, and so if >> you have many null arguments the builder code is still readable, but I >> already demonstrated that there are other ways to achieve the same >> readability. >> >> I'm thinking this is just a matter of style and personal preference. >> >> >> >> On Mon, Jun 16, 2014 at 11:15 PM, Gary Gregory <[email protected]> >> wrote: >> >>> FWIW, it does seem like a *lot* of work to redo some if not all plugins >>> and I do understand Matt's POV. Would it even be possible to add builders >>> later? Would we have to keep factory methods around for BC? I do not think >>> there is a right and wrong pattern here, it's just that we have something >>> that works now, indeed, with some pains here and there (default values for >>> example). >>> >>> Here is an alternate question: if we could wave a magic wand and get the >>> work done now, which solution would we want? >>> >>> Gary >>> >>> >>> >>> On Mon, Jun 16, 2014 at 9:41 AM, Remko Popma <[email protected]> >>> wrote: >>> >>>> Matt, >>>> >>>> Ralph's argument (and I agree) was that we want only one way to do >>>> plugins, either with a factory method or with a builder. >>>> >>>> Currently only two plugins use the new builder: PatternLayout and >>>> HtmlLayout. >>>> So we can either revert these two back to use a factory method, or we >>>> can convert the remaining 147 uses of the factory method to builders. >>>> That last option would mean writing the builders for those plugins, and >>>> modifying all JUnit tests that currently use the factory methods. >>>> >>>> I agree with Ralph that converting all remaining 147 places to use >>>> builders would take up a lot of our time and energy for very little gain. >>>> >>>> Matt, you mentioned that you could live with factory methods if there >>>> was a better way to write JUnit tests than >>>> FileAppender.createAppender("true", "true", "true", "true", "true", >>>> "true", "true", "4096", null, null, "false", null, null); >>>> >>>> I believe I've demonstrated several ways in which such JUnit test code >>>> can be improved. >>>> Are you okay with removing the builders and reverting back to the >>>> factory methods? >>>> >>>> >>>> >>>> On Mon, Jun 16, 2014 at 10:23 PM, Matt Sicker <[email protected]> wrote: >>>> >>>>> I'm unable to load that page for some reason, but yes, using a factory >>>>> method as such is really not extensible. If the factory method took a Map >>>>> or some sort of configuration object (which itself could use the fluent >>>>> builder pattern), that would be less tightly coupled. >>>>> >>>>> Wouldn't it also make sense for the factory methods to be private or >>>>> similar? That way they're only accessible through reflection. >>>>> >>>>> >>>>> On Sunday, 15 June 2014, Paul Benedict <[email protected]> wrote: >>>>> >>>>>> You don't want factory methods that take umpteen arguments. That's no >>>>>> way to make your builder extensible for future enhancements. Instead, you >>>>>> want a builder object that has a fluent API. >>>>>> >>>>>> >>>>>> http://docs.spring.io/spring/docs/3.2.8.RELEASE/javadoc-api/org/springframework/beans/factory/support/BeanDefinitionBuilder.html >>>>>> >>>>>> >>>>>> Cheers, >>>>>> Paul >>>>>> >>>>>> >>>>>> On Sun, Jun 15, 2014 at 10:04 PM, Ralph Goers < >>>>>> [email protected]> wrote: >>>>>> >>>>>> Matt, >>>>>> >>>>>> The only objection I have to builders is that there should only be >>>>>> one way to configure plugins and I have neither the time or energy to >>>>>> convert all plugins from factories to builders. With 130+ open issues I >>>>>> think our time is better focused there instead of fixing something that >>>>>> already works. >>>>>> >>>>>> And, FWIW, the only place you will see createAppender coded like that >>>>>> is in unit tests and there are a bunch of ways to make that clearer. >>>>>> >>>>>> Ralph >>>>>> >>>>>> On Jun 15, 2014, at 7:19 PM, Matt Sicker <[email protected]> wrote: >>>>>> >>>>>> I'm really against using factory methods due to language limitations >>>>>> in Java. You can't specify default values, for one. Two, the more >>>>>> parameters a factory takes, the crazier the method is. Seriously, tell me >>>>>> what this method is specifying: >>>>>> >>>>>> FileAppender.createAppender("true", "true", "true", "true", "true", >>>>>> "true", "true", "4096", null, null, "false", null, null); >>>>>> >>>>>> >>>>>> On 15 June 2014 21:05, Remko Popma <[email protected]> wrote: >>>>>> >>>>>> I'm fine with just the factory methods too. >>>>>> >>>>>> Sent from my iPhone >>>>>> >>>>>> On 2014/06/16, at 9:44, Scott Deboy <[email protected]> wrote: >>>>>> >>>>>> +1 >>>>>> On Jun 15, 2014 4:05 PM, "Ralph Goers" <[email protected]> >>>>>> wrote: >>>>>> >>>>>> Do we need the builders? As I said, I prefer only one way for >>>>>> creating plugins. >>>>>> >>>>>> Ralph >>>>>> >>>>>> On Jun 15, 2014, at 2:49 PM, Remko Popma <[email protected]> >>>>>> wrote: >>>>>> >>>>>> I see. I agree that the original format is much nicer. >>>>>> >>>>>> Matt, do you think you can achieve this with the builders? >>>>>> >>>>>> Sent from my iPhone >>>>>> >>>>>> On 2014/06/16, at 1:29, Ralph Goers <[email protected]> >>>>>> wrote: >>>>>> >>>>>> While you improved some of the existing messages, you really didm’t >>>>>> address what I wanted fixed. The previous debug logs would have had >>>>>> messages similar to: >>>>>> >>>>>> Calling createLayout on class >>>>>> org.apache.logging.log4j.core.layout.PatternLayout for element >>>>>> PatternLayout with params(pattern="%d{HH:mm:ss.SSS} [%t] %-5level >>>>>> %logger{36} - %msg%n", >>>>>> Configuration(D:\rista\eclipsekws\.metadata\.plugins\org.eclipse.wst.server.core\tmp1\wtpwebapps\log4j2.0-test\WEB-INF\classes\test-log4j.xml), >>>>>> null, charset="null", alwaysWriteExceptions="null") >>>>>> >>>>>> Calling createAppender on class >>>>>> org.apache.logging.log4j.core.appender.ConsoleAppender for element >>>>>> Console >>>>>> with params(PatternLayout(%d{HH:mm:ss.SSS} [%t] %-5level %logger{36} - >>>>>> %msg%n), null, target="SYSTEM_OUT", name="console", follow="null", >>>>>> ignoreExceptions="null") >>>>>> >>>>>> Calling createAppenderRef on class >>>>>> org.apache.logging.log4j.core.config.AppenderRef for element appender-ref >>>>>> with params(ref="console", level="null", null) >>>>>> >>>>>> >>>>> >>>>> -- >>>>> Matt Sicker <[email protected]> >>>>> >>>> >>>> >>> >>> >>> -- >>> E-Mail: [email protected] | [email protected] >>> 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 >>> >> >> > > > -- > E-Mail: [email protected] | [email protected] > 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 >
