NightOwl888 commented on issue #648:
URL: https://github.com/apache/lucenenet/issues/648#issuecomment-1279802334
More issues we **don't want to change**:
1. Consider reworking this 'switch' to reduce the number of 'case's from xxx
to at most 30
2. Merge this if statement with the enclosing one.
3. Take the required action to fix the issue indicated by this 'FIXME'
comment. - These were the original TagSoup comments and we can ignore them.
4. Change return type to 'void'; not a single caller uses the returned value.
5. All 'Xxx' method overloads should be adjacent.
6. Loops should be simplified with "LINQ" expressions. - This would need to
be benchmarked to get the okay. In general, LINQ is slow and almost certainly
performs inconsistently across target frameworks.
7. Loop should be simplified by calling Select(r => r.value)). - This would
need to be benchmarked to get the okay. In general, LINQ is slow and almost
certainly performs inconsistently across target frameworks.
8. Replace this 'for' loop with a 'while' loop.
9. Extract the assignment of 'lastType' from this expression.
10. Use 'string.Contains' instead of 'string.IndexOf' to improve readability
11. Make this an auto-implemented property and remove its backing field.
12. Use the opposite operator ('>') instead.
13. Use the opposite operator ('!=') instead.
14. Extract this nested code block into a separate method.
15. Constructor has x parameters, which is greater than the 7 authorized.
16. Remove this unnecessary null check; 'is' returns false for nulls. - This
is a performance enhancement. It is far faster to check for null first before
calling `is`.
17. Remove this silly bit operation.
18. Remove this unnecessary cast to 'float'. - Where noted, this is a
floating point precision workaround for 32 bit .NET Framework.
More issues we **can change**:
1. Replace this type-check-and-cast sequence with an 'as' and a null check.
- No comment needed. This is more readable and is understood to be a better
translation from Java to .NET.
2. Remove the ToString call in order to use a strongly-typed StringBuilder
overload - Add a comment to indicate code analysis or other issue description
3. Remove the unused internal class 'FSTEntry'. - This can be removed and
replaced with a comment `// LUCENENET specific - removed FSTEntry because it is
not in use`.
4. Remove this call from a constructor to the overridable 'xxx'
method/property. - This is a difference in initialization order between Java
and .NET. In Java it is safe to make a call to a virtual member from a
constructor, but in .NET it is not. Same issue in #408. I still haven't found a
good solution to this, but it would be nice to have a list of all of the places
where it is a problem so we can consider what solution(s) will work.
5. Remove this empty statement. - When these are extra `;` characters, they
are most likely just typos. For [labeled continue
statements](https://www.programiz.com/java-programming/continue-statement#labeled-continue),
we inconsistently use `;` and `{ }`, both which trigger this warning. For
those, we should probably use the latter approach and add an inline comment `/*
LUCENENET: intentionally blank */` between the curly brackets.
6. Either remove or fill this block of code. - The comment associated with
the line can be moved inside of the curly brackets to make it non-empty. If
there is no comment, just add `// LUCENNET: intentionally empty`. Exception:
empty finally blocks - we can remove the entire try/finally out of the methods
in
7. Correct this '&' to '&&'. - Check the Java source on this to see whether
this is a translation bug or a bug carried over from Lucene, and report
appropriately.
8. Use 'StringBuilder.Append(char)' instead of
'StringBuilder.Append(string)' when the input is a constant unit string - No
comment needed. This is more efficient and is understood to be a better
translation from Java to .NET.
9. Prefer 'AsSpan' over 'Substring' when span-based overloads are available
- We can conditionally reference the package
[System.Memory](https://www.nuget.org/packages/System.Memory) in `net462` and
`netstandard2.0` to fix this. Other targets don't need to reference the
package. No comment needed. This is more efficient and is understood to be a
better translation from Java to .NET. Do note the overload of `Substring(int,
int)` differs between Java (second parameter is `end`) and .NET (second
parameter is `length`. To correct use `length = end - start`. We should have
done this already.
10. Fix this implementation of 'IDisposable' to conform to the dispose
pattern. - Note this is part of #265. Most of the issues I have spotted were
failure to call `base.Dispose(disposing)` after exiting the `if (disposing)`
block. Others are because they are on a private class that should be sealed,
but it is not (Enumerators).
11. Use nameof in place of string literal 'xxx'
12. Change the visibility of this constructor to 'protected'. - These should
all be abstract classes. Add a comment `// LUCENENET specific - changed from
public to protected` after each changed line. Exception: some classes are noted
that the constructor must be public for Reflection calls to work.
13. Rename 'xxx' which hides the field with the same name. - We can fix this
in the static initializer code. In regular instance methods, we should leave it
alone unless it diverges from Lucene.
14. Change the visibility of 'EOS_FLAG_BIT' or make it 'const' or
'readonly'. - Make it const.
15. Replace the control character at position 2 by its escape sequence
'\u3000'. - This should be marked with a `// LUCENENET specific - changed from
invisible whitespace character to '\u3000' for readability.
16. Add a nested comment explaining why this method is empty, throw a
'NotSupportedException' or complete the implementation. - Looks like the
TagSoup members weren't all reviewed to ensure that members were marked
`virtual` in .NET if they aren't explicitly marked `final` in Java:
https://android.googlesource.com/platform/external/tagsoup/+/donut-release/src/org/ccil/cowan/tagsoup/PYXWriter.java.
17. The parameter name 'WeightedPhraseInfo' is not declared in the argument
list. - [issue
link](https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&id=nikcio_lucenenet&open=AYPAuQn4hbfJOGLOocAt).
This is a bug - we need to use the correct parameter name.
18. When implementing IComparable<T>, you should also override ==, !=, <,
<=, >, and >=. - If the class is public, we should definitely implement these.
If not public, it probably doesn't matter.
<!--
Special cases:
1. Use an immutable collection or reduce the accessibility of the public
static field 'xxx'.
a. Array - we can fix this by calling
[`array.AsReadOnly()`](https://github.com/NightOwl888/J2N/blob/ceaa3d17f3866a5ec4865f9be600b76c5303764c/src/J2N/Collections/Generic/Extensions/ListExtensions.cs#L53).
In this case, we are diverging from Lucene, so we should add a `// LUCENENET
specific - made immutable` comment.
b. CharArraySet - These are already immutable if calling
`CharArraySet.UnmodifiableSet(value)`.
c. CharArrayMap - These are already immutable if calling
`CharArrayMap.UnmodifiableMap(value)`.
-->
--
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]
