Do various code cleanups

[jbduncan]

Including:

• [x] Remove BasicMapEntry - superseded by AbstractMap.SimpleImmutableEntry.
• [x] Add braces to all if/else/for/while statements.
• [ ] Run IntelliJ inspections and fix the ones deemed appropriate.
• [ ] Rename variables, fields, parameters and method names from snake_case to camelCase.
• [x] Replace cases of if (condition) { throw *Exception() } with com.google.common.base.Preconditions as appropriate.
• [ ] Add final modifier to classes and other places in the code base as appropriate.
• [ ] Use immutable collections classes like Immutable{List,Set,Map} as appropriate.
• [ ] Use Collections API interfaces more consistently, e.g.
  1. ArrayList<String> strings = new ArrayList<>(); --> List<String> strings = new ArrayList<>():
  2. LinkedHashSet<String> methodName(int parameter1, LinkedList<Integer> parameter2) { ... } --> Set<String> methodName(int parameter1, Iterable<Integer> parameter2) { ... }.
• [ ] Use Streams, lambda expressions and other Java 8 idioms more consistently and as appropriate.
• [ ] Separate out variable declarations like int a, b; into int a; int b;.
• [ ] Use @FunctionalInterface annotation wherever applicable.
• [x] Replace usages of Stack with ArrayDeque for likely performance improvements.
• [x] Use java.util.function functional interfaces instead of com.google.common.base ones.

[jrtom]

I agree that several of these are on point and uncontroversial:

• consistent bracing
• use of Preconditions
• use of Immutable classes and final modifiers where appropriate

I don't think that there are any public method names that use snake_case. If there are, those should definitely be made camelCase.
Personally I still have a residual feeling (even after 9+ years at Google) that it can be useful to distinguish between methods (using camelCase) and parameters/variables/fields (using snake_case) but I won't insist on that distinction. In any event, I don't attach a high importance to this particular change for internal code.

Revisiting parameter and return types is reasonable, but we need to be careful about blanket changes here.

I'm pretty on board with using lambdas where it helps readability. I'm less immediately convinced that we want to be using the Stream idiom much.

[jbduncan]

Personally I still have a residual feeling (even after 9+ years at Google) that it can be useful to distinguish between methods (using camelCase) and parameters/variables/fields (using snake_case) but I won't insist on that distinction.

Ah okay. I see where you're coming from, but I personally feel that it would be better to follow the mostly established convention of camelCase in this case, since it's less likely to surprise future maintainers. So, just to double check with you, would it be alright with you if I renamed parameters/variables/fields to use camelCase?

Revisiting parameter and return types is reasonable, but we need to be careful about blanket changes here.

That makes a degree of sense to me. However I admit it's not 100% clear to me yet what would sort of changes in this area would be crossing the line, so I'd be very open to feedback if I go too far in future PRs!

...I'm less immediately convinced that we want to be using the Stream idiom much.

Hrm, yeah... although I personally enjoy the declarative nature of streams, I agree with you in the sense that I'm aware that streams tend to be a little slower than standard for loops and if statements.

How about this as a compromise? We keep using built-in language constructs (like for and if) for old code, but for new code (like when we're writing new classes or doing class redesigns), we adopt streams wherever we both agree that they improve readability, and then we only change specific stream usages into for/ifs if profiling shows that they're a speed bottleneck or taking up too much memory?

[jbduncan]

Updated list with a new entry: "Separate out variable declarations like int a, b; into int a; int b;."

[jrtom]

Yeah, we can universally use camelCase in that instance, that ship has sailed. :)

The parameter/return types thing essentially breaks down into two elements:

• In some cases we need to be careful about not painting ourselves into a corner in re: future expansions. There are cases in Guava where I've changed a parameter type to Iterable and then had to change it to something broader later, which is kind of a pain.
• In some cases one might actually want to return a concrete type (Immutable* are the obvious examples) if that type conveys relevant information about the semantics of the thing being returned that a plain Set or List would not.

Regarding Streams, I tend to agree with the Guava developers that in general, I'm not so much concerned about performance (which is subject to change anyway) as I am about readability. And in my opinion, streams don't always help with readability. So it's the kind of thing I'd want to consider on a case-by-case basis.

[jbduncan]

Updated list with a new entry: "Use @FunctionalInterface annotation wherever applicable."

[jbduncan]

There are cases in Guava where I've changed a parameter type to Iterable and then had to change it to something broader later, which is kind of a pain.

Can you elaborate for me? Do you mean changing the type from Iterable to a subtype like List? If so, can you remember what situation(s) forced you to make such a change?

[jbduncan]

Regarding Streams, I tend to agree with the Guava developers that in general, I'm not so much concerned about performance (which is subject to change anyway) as I am about readability. And in my opinion, streams don't always help with readability. So it's the kind of thing I'd want to consider on a case-by-case basis.

Sounds sensible to me! Let's consider Streams on a case-by-case basis then. :)

[jrtom]

@jbduncan, please make sure that you update this issue as you check off items on the list.

[jbduncan]

Oops! Many thanks for reminding me @jrtom. Done, and will aim to keep doing. 👍

[jbduncan]

Updated list with new entry: "Replace usages of Stack with ArrayDeque for likely performance improvements."

[jbduncan]

Updated list with new entry: "Use java.util.function functional interfaces instead of com.google.common.base ones".

[jrtom]

FYI, I'm working on converting common.base.Function to java.util.Function.

[jbduncan]

Cool, thanks for letting me know! :+1:

[jrtom]

Conversion from Guava functional interfaces to use the java.util.function.{Predicate, Function, Supplier} interfaces instead is now complete.

[jbduncan]

Updated list with new entry: "Remove all "Created by..." comments."

[jbduncan]

Given @tomnelson's pushback to removing "Created by..." comments, I've removed the bullet point. :)

[wumpz]

Is spotless able to check those syntactic preferences, like camelcase, parentheses usage ... If so you should activate it

[jbduncan]

Ooh, good idea @wumpz! If I ever get around to finishing off this issue, I'll investigate Spotless (or an alternative like Checkstyle) for this purpose. 👍

[wumpz]

@jbduncan I use Checkstyle without linefeed checks. I prefer the IDE to do all formatting,. But that is a matter of taste I think, as the whole formatting thing is. I remember beautiful discussions about indention size and tab Vs spaces. :(

[jbduncan]

A future note for myself or whoever takes on this issue: I don't think that Spotless can enforce camel-case and parentheses usage out of the box, because it delegates all formatting to other formatters, and AFAICT the Spotless Maven plugin doesn't have any formatters that enforce those things.

So alternatively, consider using error-prone and writing a number of custom "checks" that refactor or enforce camel-case/less parentheses, similarly to https://github.com/google/error-prone/commit/42a9a65f103ec1b691b22a0afebee5305a6cd312#diff-abf53a1f4062a2d2fe403ca2ac709234.

[wumpz]

As a quick start, this is one of my checkstyle configurations. As you see, I first tried as well to check for linefeeds, but for me that was a dead end. ;)

<plugin>
	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-checkstyle-plugin</artifactId>
	<version>3.1.0</version>
	<executions>
		<execution>
			<id>verify-style</id>
			<phase>process-classes</phase>
			<goals>
				<goal>check</goal>
			</goals>
		</execution>
	</executions>
	<configuration>
		<logViolationsToConsole>true</logViolationsToConsole>
		<includeTestSourceDirectory>true</includeTestSourceDirectory>
		<sourceDirectories>${project.build.sourceDirectory}</sourceDirectories>
		<checkstyleRules>
			<module name="Checker">
				<module name="SuppressWarningsFilter" />
				<module name="FileTabCharacter" />
				<!-- git checkout may change linefeeds on the fly
				<module name="RegexpMultiline">
					<property name="format" value="(?s:(\r\n|\r).*)" />
					<property name="message" value="CRLF and CR line endings are prohibited, but this file uses them." />
				</module>
				-->
				<module name="TreeWalker">
					<module name="AvoidNestedBlocks" />
					<module name="ConstantName" />
					<module name="EmptyCatchBlock" />
					<module name="EmptyStatement" />
					<module name="MissingOverride" />
					<module name="MultipleVariableDeclarations" />
					<module name="ParameterAssignment" />
					<module name="StringLiteralEquality" />
					<module name="RedundantImport" />
					<module name="UnusedImports" />
					
					<module name="WhitespaceAfter" />
					
					<module name="NeedBraces" />
					<module name="UnnecessaryParentheses" />
					<module name="LeftCurly" />
					<module name="RightCurly" />
					
					<module name="SuppressWarningsHolder" />
				</module>
			</module>
		</checkstyleRules>
	</configuration>
	<dependencies>
		<dependency>
			<groupId>com.puppycrawl.tools</groupId>
			<artifactId>checkstyle</artifactId>
			<version>8.22</version>
		</dependency>
	</dependencies>
</plugin>