Don't allow IndexWriterConfig setters to chain [LUCENE-3756]

[asfimport]

Spinoff from #4810.

I don't like that IndexWriterConfig's setters are chainable; it
results in code in our tests like this:

IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig( TEST_VERSION_CURRENT, new MockAnalyzer(random)).setMaxBufferedDocs(2).setMergePolicy(newLogMergePolicy()));

I think in general we should avoid chaining since it encourages hard
to read code (code is already hard enough to read!).

---

Migrated from LUCENE-3756 by Michael McCandless (@mikemccand), updated Feb 10 2012

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

I disagree :). The fact that the API allows chaining (a builder pattern I think?) doesn't mean you need to use it. I find it very useful and convenient that I can do new IWC().setOpenMode() for example.

If you remove that capability, you will affect many people that like it, while those that don't like it can still call the setters one in each line.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Shai: Thanks for the comment, I have no idea how to fight against this anti-builder mentality. There is nothing that forces you to use chaining.

See also #4810, I brought all arguments.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

The fact that the API allows chaining (a builder pattern I think?) doesn't mean you need to use it.

The problem is, I haven't yet seen a usage of this chaining that is
is "good".

I know this chaining is apparently a core part of being a builder API,
which confuses me because I think sometimes the separate builder is
justified yet chaining doesn't seem necessary (eg FST.Builder).

Sure, with IWC's chaining, it's faster to write, less LOC, feels good,
etc., but the result is large compound expressions in one line, and
less readable code, and I think, especially in open-source
development, readability is more important than writability.

Does anyone think the example above is more readable than the
non-chained alternative? Look at all our tests, where we inline the
creation/building of an IWC: can you find one where chaining actually
helps readability?

Chaining offers a dangerous, toxic degree of freedom/ambiguity that I
think can only cause our source code, but more importantly our
community, harm.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Does anyone think the example above is more readable than the
non-chained alternative? Look at all our tests, where we inline the
creation/building of an IWC: can you find one where chaining actually
helps readability?

The not-chaining does not help for readability, chaining makes it more readable. You can format it like I did in the tests (actually, it was not me, it was Eclipse - does it detect builders?). In contrast you can also write all in one line without chaining.

So where, wtf, is the difference? Everybody can write the code as one like, so I can write a full java class in one line, why not? I can on the opposite also write all parts of a chaining api in separate lines and that's what I generally do (look at my StringBuilders when I code something). Nobody forces me to do it in the one or other variant.

In contrast, using the API with repeating the instance variable every time makes it really unreadable, especially because the important parts are hidden.

If you like, I will ask my Eclipse to rewrite all tests to make chains in new lines (if it can do this, because it did this when coding the test).

Uwe

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Mike, this is purely a style thing. You think that the code is not readable, I think that it is. If you want, we can ask Lucene committers not to chain lots of set() calls in one line, or format the code such that lines are not longer than 120 characters.

But if you change IWC's API contract, you force your preferred style on everyone. To that I cannot agree. There isn't any technical reason to force IWC to return void. If you think however (and that doesn't seem the case here) that any of the methods should return a different value, then that's a whole different story.

IMO, if a class returns void in all its methods, and it is a configuration class, it should be a builder. It's for convenience. When I want, I chain set() calls. Sometimes I have mixed code, when I want to document the reason for a specific set() call. The point is that I have the freedom to decide how to use the class.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I can on the opposite also write all parts of a chaining api in separate lines and that's what I generally do (look at my StringBuilders when I code something).

The problem is that chaining is easily (and, often, from what I've
seen) abused and quickly results in very large compound expressions
that are hard to read (to my eyes anyway). Just grep for
newIndexWriterConfig in our tests to see many examples...

Readability is important.

If you like, I will ask my Eclipse to rewrite all tests to make chains in new lines (if it can do this, because it did this when coding the test).

+1

Is it easy to have Eclipse fully un-chain it? Ie so instead of:

IndexWriter writer = new IndexWriter(dir, newIndexWriterConfig( TEST_VERSION_CURRENT, new MockAnalyzer(random)).setMaxBufferedDocs(2).setMergePolicy(newLogMergePolicy()));

We'd have something like this:

IndexWriterConfig iwc = newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random));
iwc.setMaxBufferedDocs(2);
iwc.setMergePolicy(newLogMergePolicy());

IndexWriter writer = new IndexWriter(dir, iwc);

? That would be better... (I think).

The point is that I have the freedom to decide how to use the class.

Not all freedom is good. Java and C++ (but not Python!) let you
indent code however you want, thanks to { } defining scopes, but that
doesn't mean you should be taking advantage of that freedom by
indenting source code in unexpected ways.

And in fact we are not free to take advantage of that in Lucene
because (thankfully) we have a common coding style that we all follow.

Having this freedom suddenly means we have ambiguity on how we format
these chained expressions, and I don't like adding ambiguity in our
sources: there should [generally] be one clear way of doing something.
We have enough ambiguity as it is...

So, I don't think we should be adding any more builders/chaining to
Lucene's sources... Apps can always create builders on top of Lucene.

[asfimport]

Jason Rutherglen (migrated from JIRA)

+1 I agree with Mike.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

So, I don't think we should be adding any more builders/chaining to
Lucene's sources... Apps can always create builders on top of Lucene.

That's a bit extreme I think. The API should be clear, and easy to consume/use. Why force an app to wrap and object with a Builder if we can do it in Lucene? What else do you want to return from all the setters in IWC? They return void, so why not create the flexibility to the users?

Are you against StringBuilder too?

because (thankfully) we have a common coding style that we all follow

I seriously doubt that we have A coding style, much less one that WE ALL follow. Look at Lucene code – a lot of it is badly formatted. Some people like to write their if statements without {} (if they contain a single instruction), others prefer to always put the braces. Some don't like redundant if-else statements if both the 'if' and 'else' return from the method (Eclipse even has an option to show a warning on it), others prefer to always put an if-else for readability.

Coding style is important, I agree. In my project, we enforce coding style through unit tests - we wrote a framework which checks the source code and asserts all sorts of things like line length (we limit them to 120 chars), spaces before/after opening braces, copyright notices, indentation and what not. We even have a validator that ensures you don't put 2 consecutive empty lines in the code :). The framework is executed as part of 'ant test', so these things are always checked, and unless you don't execute tests before commit, there's no chance to commit code which doesn't follow the coding style that we've decided upon. I thought about contributing it to Lucene, but wasn't sure if the community cares much about code formatting/style.

If we had such a framework, you could easily write a validator which enforces a max-length on a line (we already do), so you wouldn't end up with such long lines in the first place. You can even write a validator that ensures IWC is not used as a Builder pattern in Lucene code. The great thing about this framework is that it doesn't force you to change the API signature. You can still keep IWC as a Builder, but enforce that it's not used in Lucene like that, and I can still enjoy IWC as I do today, chaining calls when I feel it's appropriate.

Mike - if you want to change how IWC is used in Lucene tests, let's do that. But I prefer that we keep the builder API and not enforce coding style on the users.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

This is indicative of a larger worrying trend of people thinking of how to do something the best way and then trying to prohibit all other ways. It ultimately makes Lucene less flexible.

Was #4810 really reverted over this coding style? That's definitely not something we should make a precedent.

Rather than let this be decided by attrition like #3384 (freeze vs builder vs ???), it would seem fairer to perhaps call a straight up vote. This isn't a deep technical issue, but rather a style issue that most committers probably have an opinion on.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

This is indicative of a larger worrying trend of people thinking of how to do something the best way and then trying to prohibit all other ways. It ultimately makes Lucene less flexible.

I strongly agree! As I said in this, in #4810 and other issues. The builder pattern does not prevent anybody from not using it and both patterns can be used to make code unreadable. But I (and I speak also for other committers like Shai) want to have the freedom to chain my method calls if we have builder.

Was #4810 really reverted over this coding style? That's definitely not something we should make a precedent.

Yes, it was reverted because of that. I already committed and closed that issue, but Mike disagreed (in my opinion, he put not enough stress on his answer). On IRC I gave the "OK" to revert it (I wanted to be kept out of the whole process, so I "allowed" him to revert).

Rather than let this be decided by attrition like #3384 (freeze vs builder vs ???), it would seem fairer to perhaps call a straight up vote. This isn't a deep technical issue, but rather a style issue that most committers probably have an opinion on.

Yes, please!

My proposal would be (Robert als suggested that to me privately):

• Allow definition of builder patterns, but dont force users to use them
• Don't use the builders to chain in tests (unless it makes it more readable - it does quite often), at least format the tests like I did in 3736.

[asfimport]

Erick Erickson (@ErickErickson) (migrated from JIRA)

+/- INFINITY.

Personally, these kinds of discussions are a monumental waste of time and energy. Not to mention you can't avoid pissing people off no matter what is decided.

I'd rather get work out of volunteers than scare them off with this kind of discussion. And it seems that code gets partitioned pretty much by default so the style starts to take on the characteristics of the person doing the heavy lifting eventually anyway.

If someone's willing to put the time into writing the code, the least I can do is spend a bit of time adjusting to the convention used. If I'm going to modify it, then I'll feel free to make it more readable to my eyes. I happen to agree that chaining can be confusing. Tough. I'll figure it out.

See Martin Fowler's excellent book "Refactoring". One of my primary take-aways was that going in and gratuitously refactoring working code (which we would be doing if we enforced any policy by, say, removing the pattern) is almost always wrong. If it works, don't "fix" it. If you have to go into the code anyway, then feel free.

I guess my approach rather depends on how heavily I'm going into some code. A minor code change and I'll do my best to respect the style already there. Major surgery and I'm much more willing to re-do things to match my preferences. Heck, I'm even managing to refrain from reformattiong the whole file, what more do you want?

That's all I want to say about that....

[asfimport]

Steven Rowe (@sarowe) (migrated from JIRA)

I completely agree with Erick.

And as Shai says, if style compliance is going to be a continuing issue, we should agree on a style, and automate compliance checking.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Are you against StringBuilder too?

Actually I think that's a very appropriate use of builder. The
chaining on one line bothers me less here, I think because it's nearly
always append, and it's nearly always short things being appended. I
don't usually see it abused...

FST.Builder is another appropriate usage of builder (without chaining).

Other uses (eg for creating a ParallelReader), I don't think are
appropriate: it's an API change vs 3.x, it's extra work for the common
case, it creates API inconsistency (other IRs don't require builders).

I seriously doubt that we have A coding style, much less one that WE ALL follow.

In fact we do have a standard coding style (it's referenced from
HowToContribute on the wiki).

It's true, we don't uniformly/aggressively enforce it. We can
consider doing so... it's neat you get a failure from ant test if you
break the style ;) But I have mixed feelings on whether we should...

What's important is we do have a clear standard, so if there is any
doubt, disagreement, etc., it's quickly settled, instead of huge
discussions (like this!): there is no (little) ambiguity; we all can
fix the code style as we work in a given area.

Eg, when I see a single line if statement missing the { }'s, I add
them.

You can even write a validator that ensures IWC is not used as a Builder pattern in Lucene code.

+1

I don't like the ambiguity and easy "abuse" made possible by
chaining. If we adopt additions to our coding style (Sun's guidelines
don't seem to cover chained methods ;) )... then I think this will
address my concerns.

My proposal would be (Robert als suggested that to me privately):

• Allow definition of builder patterns, but dont force users to use them
• Don't use the builders to chain in tests (unless it makes it more readable - it does quite often), at least format the tests like I did in 3736.

+1, I think this is a good compromise. The builder is optional (ie
you can invoke a big-ctor instead), and we have clear code style when
builders are invoked in Lucene.

This is indicative of a larger worrying trend of people thinking of how to do something the best way and then trying to prohibit all other ways. It ultimately makes Lucene less flexible.

Hmm can you give other examples of this trend?