garydgregory edited a comment on pull request #73: URL: https://github.com/apache/commons-csv/pull/73#issuecomment-873598257
@dota17 and all: I've brought this patch in locally and it does not make sense to me. The new `Builder` class does not, in fact, follow the builder pattern as it does not build anything, it does not have a `build()` method. That's easy to fix, so I did that (locally) and it still does not make sense. The point of a builder class is usually to call the methods on a single builder (in the fluent style or not, that does not matter) and then build a single domain object. So you have two instances in play. The builder which you can call 100 times, and then a single call to a `build()` method to give you a new domain object, here a `CSVFormat`. Without this patch, if you call 100 `CSVFormat#with` methods, you get 100 new `CSVFormat` objects, whether you need them or not, which is not great. With this PR, if you call 100 `CSVFormat#with` methods, not only do you still get 100 new `CSVFormat` objects, but you also get 100 new `Builder` objects! It makes the current situation in Git master even worse! For what? I don't get it. I think what we really want is a single instance of a Builder object that you can call 100 times which creates no extra Builder objects and no extra domain objects. This PR does the opposite, so -1. I provided a proper builder, used it in the current code base, and deprecated the old `with` methods. The only drawback is that the deprecated methods use a temporary builder which could be changed back to using the contructor. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
