Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix-#1199/refactor-implementation-smells #1200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good!

Original file line number Diff line number Diff line change
Expand Up @@ -168,17 +168,27 @@ private static void parseRangeSet(RangeSet<Integer> result, String ranges) {
* converted here to {@code 0}-based.
*/
private static Range<Integer> parseRange(String arg) {
final int SINGLE_LINE = 1;
final int RANGE_LINE = 2;
final int BASE_OFFSET = 1;

List<String> args = COLON_SPLITTER.splitToList(arg);

switch (args.size()) {
case 1:
int line = Integer.parseInt(args.get(0)) - 1;
case SINGLE_LINE:
// Handle single line case (e.g., "42")
int line = Integer.parseInt(args.get(0)) - BASE_OFFSET;
return Range.closedOpen(line, line + 1);
case 2:
int line0 = Integer.parseInt(args.get(0)) - 1;
int line1 = Integer.parseInt(args.get(1)) - 1;
return Range.closedOpen(line0, line1 + 1);

case RANGE_LINE:
// Handle line range case (e.g., "1:12")
int startLine = Integer.parseInt(args.get(0)) - BASE_OFFSET;
int endLine = Integer.parseInt(args.get(1)) - BASE_OFFSET;
return Range.closedOpen(startLine, endLine + 1);

default:
throw new IllegalArgumentException(arg);
throw new IllegalArgumentException("Invalid range format: " + arg
+ ". Expected format: 'lineNumber' or 'startLine:endLine'");
}
}

Expand Down
140 changes: 87 additions & 53 deletions core/src/main/java/com/google/googlejavaformat/java/ImportOrderer.java
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactored code looks much better. However, I still get headaches when I see the parameter i.
I understand the approach with ImportAndIndex to make the side effects clearer.
At the moment, I also don't know how to improve it further.

Original file line number Diff line number Diff line change
Expand Up @@ -296,70 +296,104 @@ private static class ImportsAndIndex {
private ImportsAndIndex scanImports(int i) throws FormatterException {
int afterLastImport = i;
ImmutableSortedSet.Builder<Import> imports = ImmutableSortedSet.orderedBy(importComparator);
// JavaInput.buildToks appends a zero-width EOF token after all tokens. It won't match any
// of our tests here and protects us from running off the end of the toks list. Since it is
// zero-width it doesn't matter if we include it in our string concatenation at the end.

while (i < toks.size() && tokenAt(i).equals("import")) {
i++;
if (isSpaceToken(i)) {
i++;
}
boolean isStatic = tokenAt(i).equals("static");
if (isStatic) {
i++;
if (isSpaceToken(i)) {
i++;
}
}
if (!isIdentifierToken(i)) {
throw new FormatterException("Unexpected token after import: " + tokenAt(i));
}
StringAndIndex imported = scanImported(i);
String importedName = imported.string;
i = imported.index;
if (isSpaceToken(i)) {
i++;
}
if (!tokenAt(i).equals(";")) {
throw new FormatterException("Expected ; after import");
}
while (tokenAt(i).equals(";")) {
// Extra semicolons are not allowed by the JLS but are accepted by javac.
ImportAndIndex result = scanSingleImport(i);
imports.add(result.importStatement);
i = result.index;
afterLastImport = i;

// Skip any whitespace but preserve comment positions
while (isNewlineToken(i) || isSpaceToken(i)) {
i++;
}
StringBuilder trailing = new StringBuilder();
}

return new ImportsAndIndex(imports.build(), afterLastImport);
}

private static class ImportAndIndex {
final Import importStatement;
final int index;

ImportAndIndex(Import importStatement, int index) {
this.importStatement = importStatement;
this.index = index;
}
}

private ImportAndIndex scanSingleImport(int i) throws FormatterException {
// Skip 'import' and following space
i++;
if (isSpaceToken(i)) {
i++;
}

// Handle static keyword
boolean isStatic = tokenAt(i).equals("static");
if (isStatic) {
i++;
if (isSpaceToken(i)) {
trailing.append(tokenAt(i));
i++;
}
}

if (!isIdentifierToken(i)) {
throw new FormatterException("Unexpected token after import: " + tokenAt(i));
}

StringAndIndex imported = scanImported(i);
String importedName = imported.string;
i = imported.index;

// Handle semicolon and spaces
if (isSpaceToken(i)) {
i++;
}
if (!tokenAt(i).equals(";")) {
throw new FormatterException("Expected ; after import");
}

// Collect all trailing content (including comments)
StringBuilder trailing = new StringBuilder();
i = collectTrailingContent(i, trailing);

Import importStatement = new Import(importedName, trailing.toString(), isStatic);
return new ImportAndIndex(importStatement, i);
}

private int collectTrailingContent(int i, StringBuilder trailing) {
// Collect all semicolons
while (tokenAt(i).equals(";")) {
i++;
}

// Collect whitespace and newline
if (isSpaceToken(i)) {
trailing.append(tokenAt(i));
i++;
}
if (isNewlineToken(i)) {
trailing.append(tokenAt(i));
i++;
}

// Collect comments and their newlines
while (isSlashSlashCommentToken(i)) {
trailing.append(tokenAt(i));
i++;
if (isNewlineToken(i)) {
trailing.append(tokenAt(i));
i++;
}
// Gather (if any) all single line comments and accompanied line terminators following this
// import
while (isSlashSlashCommentToken(i)) {
trailing.append(tokenAt(i));
i++;
if (isNewlineToken(i)) {
trailing.append(tokenAt(i));
i++;
}
}
while (tokenAt(i).equals(";")) {
// Extra semicolons are not allowed by the JLS but are accepted by javac.
i++;
}
imports.add(new Import(importedName, trailing.toString(), isStatic));
// Remember the position just after the import we just saw, before skipping blank lines.
// If the next thing after the blank lines is not another import then we don't want to
// include those blank lines in the text to be replaced.
afterLastImport = i;
while (isNewlineToken(i) || isSpaceToken(i)) {
i++;
}
}
return new ImportsAndIndex(imports.build(), afterLastImport);

// Collect any remaining semicolons
while (tokenAt(i).equals(";")) {
i++;
}

return i;
}

// Produces the sorted output based on the imports we have scanned.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,28 +42,69 @@ public String rewrite(Tok tok, int maxWidth, int column0) {
if (!tok.isComment()) {
return tok.getOriginalText();
}

String text = formatJavadocIfNeeded(tok, column0);
List<String> lines = parseCommentLines(tok, text);

return formatComment(tok, lines, column0);
}

/**
* Format javadoc comment if needed
*/
private String formatJavadocIfNeeded(Tok tok, int column0) {
String text = tok.getOriginalText();
if (tok.isJavadocComment() && options.formatJavadoc()) {
text = JavadocFormatter.formatJavadoc(text, column0);
return JavadocFormatter.formatJavadoc(text, column0);
}
return text;
}

/**
* Parse comment into lines with appropriate trimming
*/
private List<String> parseCommentLines(Tok tok, String text) {
List<String> lines = new ArrayList<>();
Iterator<String> it = Newlines.lineIterator(text);

while (it.hasNext()) {
String line = it.next();
if (tok.isSlashSlashComment()) {
lines.add(CharMatcher.whitespace().trimFrom(it.next()));
lines.add(CharMatcher.whitespace().trimFrom(line));
} else {
lines.add(CharMatcher.whitespace().trimTrailingFrom(it.next()));
lines.add(CharMatcher.whitespace().trimTrailingFrom(line));
}
}
return lines;
}

/**
* Format the comment based on its type
*/
private String formatComment(Tok tok, List<String> lines, int column0) {
if (tok.isSlashSlashComment()) {
return indentLineComments(lines, column0);
}

return getFormattedBlockComment(tok, lines, column0);
}

/**
* Get formatted block comment, either as parameter comment or regular block comment
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is supposed to read this JavaDoc comment? By default, JavaDoc for private methods is not published. You can only see it in the IDE by hovering over the method name and waiting, or by jumping to the method’s implementation. The method signature is already well chosen.
Even I don’t understand what the comment means. How am I supposed to choose between a parameter comment and a regular block comment?

*/
private String getFormattedBlockComment(Tok tok, List<String> lines, int column0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my taste, the methods are too fragmented. It’s hard to keep track of all the small subroutines in my head. Isn’t there a middle ground between being too small and too complex?

return CommentsHelper.reformatParameterComment(tok)
.orElseGet(
() ->
javadocShaped(lines)
? indentJavadoc(lines, column0)
: preserveIndentation(lines, column0));
.orElseGet(() -> formatBlockComment(lines, column0));
}

/**
* Format block comment based on whether it is javadoc-shaped or not
*/
private String formatBlockComment(List<String> lines, int column0) {
if (javadocShaped(lines)) {
return indentJavadoc(lines, column0);
}
return preserveIndentation(lines, column0);
}

// For non-javadoc-shaped block comments, shift the entire block to the correct
Expand Down