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

Implement stable hashCode for records. #102

Closed
wants to merge 9 commits into from
Closed

Conversation

LLITCHEV
Copy link
Contributor

Fixes #95

Lubomir Litchev added 3 commits September 15, 2023 11:37
@github-actions
Copy link

github-actions bot commented Sep 19, 2023

JUnit Test Report

     37 files       37 suites   59s ⏱️
   491 tests    490 ✔️ 1 💤 0
2 679 runs  2 670 ✔️ 9 💤 0

Results for commit 3a2f175.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 19, 2023

Integration Test Report

86 754 tests   86 754 ✔️  2m 53s ⏱️
     213 suites           0 💤
     213 files             0

Results for commit 3a2f175.

♻️ This comment has been updated with latest results.

Signed-off-by: Lubomir Litchev <[email protected]>
Signed-off-by: Lubomir Litchev <[email protected]>
@jasperpotts
Copy link
Member

Indenting for equals() is messed up

       /**
 * Override the default equals method for
 */
@Override
public boolean equals(Object that) {
	if (that == null || this.getClass() != that.getClass()) {
		return false;
	}

	Signature thatObj = (Signature)that;

        if (signature == null && thatObj.signature != null) {
             return false;
         }
         if (signature != null && !signature.equals(thatObj.signature)) {
	return false;
}
                   return true;
    }

It could also be simplified for this case in Signature.java

       /**
        * Override the default equals method for
        */
       @Override
       public boolean equals(Object that) {
	      if (that == null || this.getClass() != that.getClass()) {
		      return false;
	      }

	      Signature thatObj = (Signature)that;
              if (signature == null) {
                   return thatObj.signature == null;
              } else {
	           return signature.equals(thatObj.signature);
             }
    }

@jasperpotts
Copy link
Member

Looking at generated Setting.java the hashCode() method is missing. Also equals formatting is a mess:

          /**
 * Override the default equals method for
 */
@Override
public boolean equals(Object that) {
	if (that == null || this.getClass() != that.getClass()) {
		return false;
	}

	Setting thatObj = (Setting)that;

        if (name == null && thatObj.name != null) {
             return false;
         }
         if (name != null && !name.equals(thatObj.name)) {
	return false;
}
if (value == null && thatObj.value != null) {
             return false;
         }
         if (value != null && !value.equals(thatObj.value)) {
	return false;
}
if (data == null && thatObj.data != null) {
             return false;
         }
         if (data != null && !data.equals(thatObj.data)) {
	return false;
}
                   return true;
    }

Also can be simplified similar to above comment.

@jasperpotts
Copy link
Member

We have put a lot of effort into the code generated being nicely formatted and commented just like it was hand written. It makes it super nice to read when debugging on services code that uses these classes.

The comment for equals() method is missing a conclusion, end with "for". Also would be nice for the bonnet to say why equals() is overridden so that it logically matches hashCode()

           /**
 * Override the default equals method for
 */

@jasperpotts
Copy link
Member

hashCode() needs to ignore fields whose value is default value. If it includes them in hash then protobuf's way of adding new fields does not work and all objects schemas will not be extendable. You need to be able to add a new field to protobuf without effecting the hashCode or equals methods of existing objects. Otherwise you store an object in a Map then come back later to fetch it after a new field has been added and the Map will tell you that object is not contained in the map. Because the hashCode value will have changed while the object is the same object.

The stable implementation of hashCode and equals is very comparable to the JDK version of these.

Here are the results from the bench:
JDK - no stable overrides of hashCode and equals
Benchmark                           Mode  Cnt   Score    Error  Units
EqualsHashCodeBench.benchEquals     avgt    5  ≈ 10⁻³           ns/op
EqualsHashCodeBench.benchHashCode   avgt    avgt    5  ≈ 10⁻³           ns/op
EqualsHashCodeBench.benchNotEquals  avgt    5   0.001 ±  0.001  ns/op

Stable overrides of hashCode and equals.
Benchmark                           Mode  Cnt   Score    Error  Units
EqualsHashCodeBench.benchEquals     avgt    5  ≈ 10⁻³           ns/op
EqualsHashCodeBench.benchHashCode   avgt    5  ≈ 10⁻³           ns/op
EqualsHashCodeBench.benchNotEquals  avgt    5   0.001 ±  0.001  ns/op

Signed-off-by: Lubomir Litchev <[email protected]>
@LLITCHEV
Copy link
Contributor Author

@jasperpotts As we discussed, I need to return only if there is false equals. Otherwise it should fallthrough the rest of the fields. That is why the ifs are a bit more involved.
Fixed the indenting - thanks for letting me know how it works exactly in this project.
Added a benchmark to measure the JDK's hashCode() and equals() implementation and compare it with the stable implementation we have.
No differences in perf detected.
JDK...
Benchmark Mode Cnt Score Error Units
EqualsHashCodeBench.benchEquals avgt 5 ≈ 10⁻³ ns/op
EqualsHashCodeBench.benchHashCode avgt avgt 5 ≈ 10⁻³ ns/op
EqualsHashCodeBench.benchNotEquals avgt 5 0.001 ± 0.001 ns/op

Our stable implementation...
Benchmark Mode Cnt Score Error Units
EqualsHashCodeBench.benchEquals avgt 5 ≈ 10⁻³ ns/op
EqualsHashCodeBench.benchHashCode avgt 5 ≈ 10⁻³ ns/op
EqualsHashCodeBench.benchNotEquals avgt 5 0.001 ± 0.001 ns/op

@LLITCHEV LLITCHEV self-assigned this Sep 22, 2023
@LLITCHEV
Copy link
Contributor Author

@OlegMazurov per talking to @jasperpotts , could you please take a look. We just want to make sure there are no obvious issues with scalability here. Thanks!!

@LLITCHEV LLITCHEV requested a review from artemananiev October 17, 2023 17:43
Lubomir Litchev and others added 2 commits October 17, 2023 10:47
return """
/**
* Builder class for easy creation, ideal for clean code where performance is not critical. In critical performance
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 139).

The issue identified by Checkstyle is that the line of code exceeds the maximum length of 120 characters, which is a common code style guideline to ensure code readability. Long lines can be difficult to read, especially in environments where screen space is limited, such as code reviews, or when viewing code on smaller screens.

To fix this issue, you can break the comment into multiple lines so that each line does not exceed the 120-character limit. Here's one way to reformat the comment:

Suggested change
* Builder class for easy creation, ideal for clean code where performance is not critical. In critical performance
/**
* Builder class for easy creation, ideal for clean code where performance is not
* critical. In critical performance paths, use the constructor directly.
*/

This comment was generated by an experimental AI tool.

dataBuffer3.getBytes(0, readBytes);
assertArrayEquals(bytes.toByteArray(), readBytes);
// Test JSON Reading
final $modelClassName jsonReadPbj = $modelClassName.JSON.parse(JsonTools.parseJson(charBuffer), false);
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 143).

The code provided has a line that exceeds the maximum length of 120 characters as per the coding style enforced by Checkstyle. Long lines of code can be difficult to read, especially if the reader has to scroll horizontally to see the entire line. To make the code more readable and compliant with the line length limit, you can break the long line into multiple shorter lines.

One way to fix this issue is by splitting the line into separate statements or by introducing a local variable to hold intermediate results. Here's how you could refactor the problematic line:

// Original code with a long line
final $modelClassName jsonReadPbj = $modelClassName.JSON.parse(JsonTools.parseJson(charBuffer), false);

// Refactored code with shorter lines
ParsedJson parsedJson = JsonTools.parseJson(charBuffer);
final $modelClassName jsonReadPbj = $modelClassName.JSON.parse(parsedJson, false);

By introducing a new local variable parsedJson, the code becomes clearer and each line is now within the 120-character limit.

Here's the code suggestion wrapped in the requested code block:

Suggested change
final $modelClassName jsonReadPbj = $modelClassName.JSON.parse(JsonTools.parseJson(charBuffer), false);
ParsedJson parsedJson = JsonTools.parseJson(charBuffer);
final $modelClassName jsonReadPbj = $modelClassName.JSON.parse(parsedJson, false);

This comment was generated by an experimental AI tool.

for (Field f : fields) {
if (f.parent() != null) {
final OneOfField oneOfField = f.parent();
generatedCodeSoFar += getFieldsHashCode(oneOfField.fields(), generatedCodeSoFar);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium Code Style issue: Avoid reassigning parameters such as 'generatedCodeSoFar'

The code is problematic because it reassigns the parameter generatedCodeSoFar inside the loop. This is generally considered a bad practice because it can lead to confusing and hard-to-maintain code. Parameters are expected to be inputs to a method, and reassigning them can lead to side effects that are difficult to track. Additionally, this code uses string concatenation inside a loop, which can be inefficient due to the creation of many temporary String objects.

To fix this issue, you should avoid reassigning the parameter generatedCodeSoFar. Instead, use a local variable to accumulate the changes and return the result at the end of the method. Also, consider using a StringBuilder to improve efficiency when concatenating strings in a loop.

Here's how you can refactor the method:

public static String getFieldsHashCode(final List<Field> fields, final String initialCode) throws RuntimeException {
    StringBuilder codeBuilder = new StringBuilder(initialCode);
    for (Field f : fields) {
        if (f.parent() != null) {
            final OneOfField oneOfField = f.parent();
            codeBuilder.append(getFieldsHashCode(oneOfField.fields(), codeBuilder.toString()));
        }

        if (f.optionalValueType()) {
            codeBuilder.append(getOptionalHashCodeGeneration(codeBuilder.toString(), f));
        } else if (f.repeated()) {
            codeBuilder.append(getRepeatedHashCodeGeneration(codeBuilder.toString(), f));
        } else if (f.nameCamelFirstLower() != null) {
            // ... rest of the code that appends to codeBuilder
        }
        // ... other conditions and code that appends to codeBuilder
    }
    return codeBuilder.toString();
}

And here's the suggested code change:

Suggested change
generatedCodeSoFar += getFieldsHashCode(oneOfField.fields(), generatedCodeSoFar);
StringBuilder codeBuilder = new StringBuilder(generatedCodeSoFar);
for (Field f : fields) {
if (f.parent() != null) {
final OneOfField oneOfField = f.parent();
codeBuilder.append(getFieldsHashCode(oneOfField.fields(), codeBuilder.toString()));
}
// ... rest of the code that appends to codeBuilder
}
return codeBuilder.toString();

This comment was generated by an experimental AI tool.

* Unit Test for %s model object. Generate based on protobuf schema.
*/
public final class %s {
%s
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: File contains tab characters (this is the first instance).

The code provided seems to be a template for generating unit test classes in Java. The issue identified by Checkstyle is the use of tab characters for indentation instead of spaces. Checkstyle, by default, enforces the use of spaces over tabs to maintain consistency in the indentation across different environments and editors.

The use of tabs can lead to misaligned code when viewed in different text editors or IDEs, as the width of a tab character can vary, whereas spaces provide a consistent look. To fix this issue, you should replace all tab characters with spaces, usually at a conversion rate of one tab to four spaces (or whatever the project's style guide dictates).

Here's how you can fix the problematic line by replacing the tab character with spaces:

Suggested change
%s
%s

Remember to apply this change to all lines that contain tab characters, not just the one identified by the linter.


This comment was generated by an experimental AI tool.

sb.append(
"""

// handle special case where protobuf does not have destination between a OneOf with optional
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 149).

The issue here is that the line of code exceeds the maximum length allowed by the coding style guide, which is typically set to 120 characters to ensure readability. Long lines can be hard to read, especially in environments where screen space is limited or when multiple files are open side by side.

To fix this issue, you can break the comment into multiple lines, ensuring that each line stays within the 120-character limit. Here's a revised version of the problematic line, split into two lines:

Suggested change
// handle special case where protobuf does not have destination between a OneOf with optional
// handle special case where protobuf does not have a clear distinction
// between a OneOf with an optional empty value and an unset OneOf.

This suggestion keeps the code within the recommended line length, making it more readable and compliant with the style guide.


This comment was generated by an experimental AI tool.

import com.hedera.pbj.runtime.MalformedProtobufException;
import com.hedera.pbj.runtime.io.buffer.BufferedData;
import com.hedera.pbj.runtime.io.buffer.Bytes;
import com.hedera.pbj.runtime.io.stream.ReadableStreamingData;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unused import - com.hedera.pbj.runtime.io.stream.ReadableStreamingData.

The code issue identified by Checkstyle is an unused import statement. Unused imports are problematic because they clutter the codebase, make the code less readable, and can sometimes lead to confusion about class dependencies. Additionally, they can slightly increase the size of the compiled bytecode, although the Java compiler and JVM are quite good at optimizing this.

In Java, it is considered good practice to remove any import statements that are not used by the class. This helps to keep the code clean and maintainable. Most modern IDEs have the ability to automatically identify and remove unused imports, and there are also build tools and plugins that can do this as part of the build process.

To fix this issue, simply remove the unused import statement from the code:

Suggested change
import com.hedera.pbj.runtime.io.stream.ReadableStreamingData;
// Remove the unused import statement

This comment was generated by an experimental AI tool.

byteBuffer.flip();

// read proto bytes with ProtoC to make sure it is readable and no parse exceptions are thrown
final $protocModelClass protoCModelObj = $protocModelClass.parseFrom(byteBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 121).

The code line in question exceeds the maximum length of 120 characters as defined by the code style enforced by Checkstyle. Long lines of code can be harder to read and maintain, especially when working with version control systems or code review tools that may not display long lines completely without scrolling.

To fix this issue, you can break the line into multiple lines to ensure that no single line exceeds the 120-character limit. For example, you can split the assignment and the method call into two lines, or you can introduce a variable to hold the result of byteBuffer before passing it to the parseFrom method.

Here's how you could reformat the problematic line to comply with the character limit:

Suggested change
final $protocModelClass protoCModelObj = $protocModelClass.parseFrom(byteBuffer);
final $protocModelClass protoCModelObj =
$protocModelClass.parseFrom(byteBuffer);
```
Alternatively, if applicable, you could introduce a local variable to hold the `byteBuffer` before passing it to the `parseFrom` method:
```suggestion
ByteBuffer bufferToParse = byteBuffer;
final $protocModelClass protoCModelObj = $protocModelClass.parseFrom(bufferToParse);

Either of these suggestions will help you adhere to the 120-character line length limit.


This comment was generated by an experimental AI tool.

if (!hashCodeGenerated) {
// Generate a call to private method that iterates through fields
// and calculates the hashcode.
statements = Common.getFieldsHashCode(fields, statements);
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unnecessary use of fully qualified name 'Common.getFieldsHashCode' due to existing static import 'com.hedera.pbj.compiler.impl.Common.*'

The issue raised by PMD is related to the code style, specifically the unnecessary use of a fully qualified name for a method call. In Java, when a static import is used, it allows for the members (methods or fields) of a class to be used without specifying the class in which the field is defined.

In the provided code context, it seems that there is already a static import for the Common class methods, indicated by the comment import com.hedera.pbj.compiler.impl.Common.*. This means that all static methods of Common can be called directly without needing to prefix them with the class name. However, the code is still using the fully qualified name Common.getFieldsHashCode(fields, statements) instead of simply getFieldsHashCode(fields, statements).

To fix this issue, you should remove the class name prefix from the method call, as the static import already covers it.

Here's the fixed line of code:

Suggested change
statements = Common.getFieldsHashCode(fields, statements);
statements = getFieldsHashCode(fields, statements);

This comment was generated by an experimental AI tool.

import com.hedera.pbj.test.proto.pbj.Hasheval;
import com.hedera.pbj.test.proto.pbj.Suit;
import com.hedera.pbj.test.proto.pbj.TimestampTest;
import com.hedera.pbj.test.proto.pbj.tests.HashevalTest;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unused import - com.hedera.pbj.test.proto.pbj.tests.HashevalTest.

The issue highlighted by Checkstyle is about an unused import statement. In Java, it's considered good practice to remove any import statements that are not used in the code, as they can lead to unnecessary clutter and can sometimes cause confusion or even name conflicts if there are classes with the same name in different packages.

Unused imports can occur for several reasons, such as after refactoring code where a class is no longer used, or if an import was added automatically by an IDE during auto-completion but ended up not being used.

To fix this issue, simply remove the unused import statement from the code. This will clean up the code and ensure that it adheres to the code style guidelines enforced by Checkstyle.

Here's the suggestion to remove the unused import:

Suggested change
import com.hedera.pbj.test.proto.pbj.tests.HashevalTest;
// Remove the following unused import statement
// import com.hedera.pbj.test.proto.pbj.tests.HashevalTest;

By applying this change, the code will be cleaner and comply with the code style rules. Remember to check if the import is indeed unused in the entire file before removing it to avoid accidentally causing compilation errors.


This comment was generated by an experimental AI tool.

dataBuffer2.resetPosition();
assertTrue($modelClassName.PROTOBUF.fastEquals(modelObj, dataBuffer2));
// Test JSON Writing
final CharBufferToWritableSequentialData charBufferToWritableSequentialData = new CharBufferToWritableSequentialData(charBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 169).

The problematic line of code exceeds the maximum line length of 120 characters as defined by the Checkstyle rule for code style. Long lines of code can be difficult to read and maintain, especially when viewing the code on devices or editors with limited horizontal space. To fix this issue, you can break the line into multiple lines to ensure that no single line goes beyond the 120-character limit.

Here's a suggestion to split the long line into two lines:

Suggested change
final CharBufferToWritableSequentialData charBufferToWritableSequentialData = new CharBufferToWritableSequentialData(charBuffer);
final CharBufferToWritableSequentialData charBufferToWritableSequentialData =
new CharBufferToWritableSequentialData(charBuffer);

By breaking the line at an appropriate place (such as after the = sign), the code becomes more readable and adheres to the Checkstyle 120-character limit rule.


This comment was generated by an experimental AI tool.

}

private static int processForBetterDistribution(int val) {
val += val << 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium Code Style issue: Avoid reassigning parameters such as 'val'

The issue highlighted by PMD is about the practice of reassigning the value of a method parameter. Reassigning parameters can lead to confusing code because it changes the input value that was passed into the method, which can make the code harder to read and maintain. It's generally a good practice to keep parameters as final, which means their value does not change throughout the method execution.

In the provided code, the parameter val is being reassigned multiple times in the method processForBetterDistribution. To fix this issue, you can introduce a new local variable that represents the result and assign the modified val to it. This way, the original parameter remains unchanged, and the code becomes clearer.

Here's how you could refactor the method to avoid reassigning the parameter:

Suggested change
val += val << 30;
private static int processForBetterDistribution(int val) {
int result = val;
result += result << 30;
result ^= result >>> 27;
result += result << 16;
result ^= result >>> 20;
result += result << 5;
result ^= result >>> 18;
result += result << 10;
result ^= result >>> 24;
result += result << 30;
return result;
}

This change creates a new local variable result that is used for the calculation, and val remains unchanged. This makes the method more predictable and easier to understand, as the input parameter val is not modified.


This comment was generated by an experimental AI tool.

import static com.hedera.pbj.runtime.ProtoTestTools.getThreadLocalByteBuffer;
import static com.hedera.pbj.runtime.ProtoTestTools.getThreadLocalDataBuffer;
import static com.hedera.pbj.runtime.ProtoTestTools.getThreadLocalDataBuffer2;
import static org.junit.jupiter.api.Assertions.assertEquals;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unused import - org.junit.jupiter.api.Assertions.assertEquals.

The code includes an import statement for assertEquals from org.junit.jupiter.api.Assertions, which is a method commonly used in writing test cases to assert that two values are equal. However, the provided code snippet doesn't show any usage of assertEquals, which means the import is not necessary for the current code to function. Unused imports can clutter the codebase, make it harder to read, and can potentially lead to confusion about code dependencies.

To fix this issue, you should remove the unused import statement from the code. This will clean up the code and ensure that only necessary imports are included.

Here's the suggested change to remove the unused import:

Suggested change
import static org.junit.jupiter.api.Assertions.assertEquals;
// Remove the unused import statement
// import static org.junit.jupiter.api.Assertions.assertEquals;

This comment was generated by an experimental AI tool.

}

/**
* Return a new builder for building a model object. This is just a shortcut for <code>new Model.Builder()</code>.
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 146).

The issue identified by Checkstyle is that the comment line exceeds the maximum length allowed by the project's code style guidelines, which is 120 characters per line. Long lines of code can be harder to read, especially in environments where screen space is limited or when working with side-by-side diff views in version control systems.

To fix this issue, you should split the comment into multiple lines, ensuring that each line does not exceed the 120-character limit. Here's how you could reformat the comment:

Suggested change
* Return a new builder for building a model object. This is just a shortcut for <code>new Model.Builder()</code>.
/**
* Return a new builder for building a model object.
* This is just a shortcut for <code>new Model.Builder()</code>.
*
* @return a new builder
*/
public static Builder newBuilder() {
return new Builder();
}

By breaking the comment into two lines, each line now adheres to the code style guideline of not exceeding 120 characters.


This comment was generated by an experimental AI tool.


import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.nio.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Using the '.' form of import should be avoided - java.nio..

The code provided includes a wildcard import statement for the java.nio package:

import java.nio.*;

Wildcard imports, indicated by the .*, import all classes from a package. While this can make it easier to access all classes within a package without listing them individually, it can also lead to several issues:

  1. Namespace Clarity: It becomes unclear which classes are actually being used from the package, making the code less readable and maintainable.
  2. Potential Conflicts: If two packages have classes with the same name, wildcard imports could cause name conflicts, leading to compilation errors or the need for fully qualified class names.
  3. Compile Time: Wildcard imports can slightly increase compile time because the compiler has to resolve all the classes in the package, although this impact is usually negligible.
  4. Code Style and Best Practices: Many style guides and linters, like Checkstyle, discourage the use of wildcard imports in favor of explicit class imports.

To fix the issue, you should replace the wildcard import with specific class imports for only the classes you are actually using from the java.nio package. For instance, if you are only using ByteBuffer and CharBuffer from java.nio, you would import them explicitly:

import java.nio.ByteBuffer;
import java.nio.CharBuffer;

Without knowing which specific classes from java.nio are being used in your code, I can't provide the exact imports you need. However, you should look through your code, identify which classes from java.nio are used, and import them individually.

Here is a code suggestion to replace the wildcard import with placeholder individual imports:

Suggested change
import java.nio.*;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
// Add other specific imports from java.nio as needed

This comment was generated by an experimental AI tool.

import com.hedera.pbj.runtime.MalformedProtobufException;
import com.hedera.pbj.runtime.io.buffer.BufferedData;
import com.hedera.pbj.runtime.io.buffer.Bytes;
import com.hedera.pbj.runtime.io.stream.ReadableStreamingData;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unused import - com.hedera.pbj.runtime.io.stream.ReadableStreamingData.

The issue highlighted by Checkstyle is that the code contains an import statement for com.hedera.pbj.runtime.io.stream.ReadableStreamingData, but this class is not being used anywhere in the code snippet provided. Unused imports can lead to unnecessary clutter in the codebase, making it harder to read and understand, and can potentially cause confusion about the code's dependencies.

To fix this issue, you should remove the unused import statement from your code. This will clean up the code and ensure that only necessary dependencies are included, which is a good practice for maintaining a clean and efficient codebase.

Here's the code suggestion to remove the unused import:

Suggested change
import com.hedera.pbj.runtime.io.stream.ReadableStreamingData;
// Remove the unused import statement
// import com.hedera.pbj.runtime.io.stream.ReadableStreamingData;

This comment was generated by an experimental AI tool.

* @return one of value or null if one of is not set or a different one of value
*/
public @Nullable $javaFieldType $fieldName() {
return $oneOfField.kind() == $enumName.$enumValue ? ($javaFieldType)$oneOfField.value() : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: File contains tab characters (this is the first instance).

The problem with the provided code snippet is that it contains tab characters for indentation, which is against the coding style enforced by Checkstyle. Checkstyle prefers the use of spaces over tabs for indentation to maintain consistency across different environments and editors.

To fix this issue, you should replace the tab characters with spaces. The number of spaces used for each indentation level is usually determined by the project's style guide (commonly 2 or 4 spaces per indentation level).

Here is how you can fix the problematic line by replacing the tab character with spaces (assuming 4 spaces per indentation level):

Suggested change
return $oneOfField.kind() == $enumName.$enumValue ? ($javaFieldType)$oneOfField.value() : null;
return $oneOfField.kind() == $enumName.$enumValue ? ($javaFieldType)$oneOfField.value() : null;

Make sure to apply this change consistently throughout the entire codebase to adhere to the style guide enforced by Checkstyle.


This comment was generated by an experimental AI tool.

import com.hedera.pbj.intergration.test.TestHashFunctions;
import com.hedera.pbj.runtime.MalformedProtobufException;
import com.hedera.pbj.runtime.io.buffer.BufferedData;
import com.hedera.pbj.runtime.io.buffer.Bytes;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unused import - com.hedera.pbj.runtime.io.buffer.Bytes.

The code provided includes an import statement for com.hedera.pbj.runtime.io.buffer.Bytes which is flagged by Checkstyle as an unused import. Unused imports are problematic because they can:

  1. Lead to unnecessary compilation overhead.
  2. Make the code less readable and maintainable, as they can cause confusion about which classes are actually being used in the code.
  3. Potentially cause name conflicts and compilation errors if there are other classes with the same name in different packages.

To fix the issue, you should remove the unused import statement from the code. This will clean up the codebase, making it more maintainable and avoiding any unnecessary compilation overhead.

Here's the code suggestion to fix the problem:

Suggested change
import com.hedera.pbj.runtime.io.buffer.Bytes;
// Remove the unused import statement
// import com.hedera.pbj.runtime.io.buffer.Bytes;

This comment was generated by an experimental AI tool.

@@ -0,0 +1,116 @@
package com.hedera.pbj.intergration.test;

import com.google.protobuf.CodedOutputStream;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unused import - com.google.protobuf.CodedOutputStream.

In Java, having unused imports can lead to several problems:

  1. Readability: Unused imports can clutter the code, making it harder to read and understand. It can be misleading to other developers as they might expect that the imported classes are used somewhere in the code.
  2. Compile-Time Overhead: Each import statement is processed at compile-time, which can slightly increase the compilation time. Although the effect is minimal for a single import, it can add up in larger projects with many unused imports.
  3. Potential Conflicts: Unused imports can sometimes lead to name conflicts if different classes with the same name are imported but not used.

To fix the issue, you should remove the unused import statement from the code. This will clean up the codebase and adhere to good coding practices. In this case, since the CodedOutputStream class is not used anywhere in the provided code context, the import statement for CodedOutputStream should be removed.

Here's how you can update the code to resolve the issue:

Suggested change
import com.google.protobuf.CodedOutputStream;
// Removed unused import

This comment was generated by an experimental AI tool.

FIELD_INDENT
+ """
if ($fieldName == null) {
throw new NullPointerException("Parameter '$fieldName' must be supplied and can not be null");
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 174).

The issue with the code is that it violates a common code style guideline which recommends that lines of code should not exceed a certain number of characters, often 80 or 120. This is to ensure readability and maintainability of the code, as excessively long lines can be difficult to read, especially on smaller screens or when viewing multiple files side by side.

To fix this, you can break the long line into multiple shorter lines, ensuring that each line does not exceed the recommended length. In Java, you can concatenate strings across multiple lines to maintain the code's functionality.

Here is how you could refactor the problematic line to adhere to the line length restriction:

Suggested change
throw new NullPointerException("Parameter '$fieldName' must be supplied and can not be null");
throw new NullPointerException(
"Parameter '" + f.nameCamelFirstLower() + "' must be supplied and cannot be null");

By breaking the line and using string concatenation, the code becomes more readable and adheres to the style guideline. Note that I have also replaced the $fieldName placeholder with f.nameCamelFirstLower() directly in the string concatenation, assuming that f.nameCamelFirstLower() is a method call that returns the field's name in camel case with the first letter in lowercase.


This comment was generated by an experimental AI tool.

// Object[] objArray = objects.toArray();
// for (int i = 0; i < objArray.length; i++) {
// for (int j = i; j < objArray.length; j++) {
// if (objArray[i].hashCode() != objArray[i].hashCode()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 127).

The code provided has a line that exceeds the maximum length of 120 characters as defined by the coding style enforced by Checkstyle. Long lines of code can be difficult to read and maintain, especially when viewing the code in environments with limited horizontal space, such as code review tools or editors with multiple panes.

To fix this issue, you can break the long line into multiple lines. This will improve readability and ensure that the code adheres to the Checkstyle rule for maximum line length. Here's how you can break the line to comply with the rule:

Suggested change
// if (objArray[i].hashCode() != objArray[i].hashCode()) {
// if (objArray[i].hashCode() != objArray[i].hashCode()) {
// fail("Same object, different hash.");
// }

In the above suggestion, the comment is split into multiple lines, each within the 120-character limit. However, since this is a commented-out portion of the code, it might not be necessary to fix it unless you plan to uncomment and use it in the future. If that's the case, consider refactoring the code when you do so to ensure it meets the style guidelines.


This comment was generated by an experimental AI tool.

String equalsStatements = "";
// Generate a call to private method that iterates through fields
// and calculates the hashcode.
equalsStatements = Common.getFieldsEqualsStatements(fields, equalsStatements);
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unnecessary use of fully qualified name 'Common.getFieldsEqualsStatements' due to existing static import 'com.hedera.pbj.compiler.impl.Common.*'

The code provided seems to be part of a larger codebase, where there is an attempt to generate hashCode and equals methods dynamically for a class. The specific issue mentioned is related to the use of a fully qualified method name, which is unnecessary because there is a static import in place that makes it possible to call the method directly without the class name.

When a static import is used, such as import static com.hedera.pbj.compiler.impl.Common.*;, it allows for the static members and methods of the imported class to be used without specifying the class name. This can make the code more readable by reducing verbosity, as long as it doesn't introduce ambiguity.

The linter PMD has flagged the use of Common.getFieldsEqualsStatements as unnecessary because the method can be called directly due to the static import. To fix this issue, you should remove the class name Common and call the method getFieldsEqualsStatements directly.

Here's how you can fix the problematic line:

Suggested change
equalsStatements = Common.getFieldsEqualsStatements(fields, equalsStatements);
equalsStatements = getFieldsEqualsStatements(fields, equalsStatements);

This comment was generated by an experimental AI tool.

}
// check measure methods
dataBuffer2.resetPosition();
assertEquals(protoBufByteCount, $modelClassName.PROTOBUF.measure(dataBuffer2));
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 120 characters (found 135).

This code line exceeds the maximum length of 120 characters as recommended by many style guides, including Checkstyle for Java. Long lines of code can be harder to read and understand, and they may not display well in various environments without wrapping or horizontal scrolling.

To resolve this issue, you can break the line into multiple lines. This will make the code more readable and maintainable. For instance, you could store the result of the measure method in a variable and then use that variable in the assertEquals statement.

Here's a refactored version of the problematic line:

Suggested change
assertEquals(protoBufByteCount, $modelClassName.PROTOBUF.measure(dataBuffer2));
int measuredByteCount = $modelClassName.PROTOBUF.measure(dataBuffer2);
assertEquals(protoBufByteCount, measuredByteCount);

This change ensures that the line length complies with the 120-character limit and maintains the readability of the code.


This comment was generated by an experimental AI tool.

import com.google.protobuf.CodedInputStream;
import com.google.protobuf.CodedOutputStream;
import com.hedera.pbj.intergration.test.TestHashFunctions;
import com.hedera.pbj.runtime.MalformedProtobufException;
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Unused import - com.hedera.pbj.runtime.MalformedProtobufException.

The issue highlighted by Checkstyle is that the MalformedProtobufException class is imported but not used anywhere in the code. Unused imports can lead to unnecessary clutter in the codebase, making it harder to read and maintain. It's considered good practice to remove any imports that are not being used.

To fix this issue, you should simply remove the import statement for MalformedProtobufException from the code. Here's the suggested change:

Suggested change
import com.hedera.pbj.runtime.MalformedProtobufException;
// Remove the unused import statement
// import com.hedera.pbj.runtime.MalformedProtobufException;

After making this change, ensure that the rest of your code does not actually use the MalformedProtobufException class. If it's used, you should not remove the import statement. Otherwise, removing it will clean up the code and resolve the Checkstyle warning.


This comment was generated by an experimental AI tool.

}

/**
* Create a stream of all test permutations of the %s class we are testing. This is reused by other tests
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ Codacy found a minor Code Style issue: Line is longer than 80 characters (found 121).

The issue with the code is that it violates the code style rule that requires lines to be no longer than 80 characters. This is a common practice in many code style guidelines as it improves readability, especially in environments with limited screen space or when viewing code side-by-side in a diff tool.

To fix this issue, you can break the comment into multiple lines so that each line does not exceed 80 characters. Here's how you can rewrite the comment:

Suggested change
* Create a stream of all test permutations of the %s class we are testing. This is reused by other tests
* Create a stream of all test permutations of the %s class.
* This stream is reused by other tests as well that have
* model objects with fields of this type.
*
* @return stream of model objects for all test cases

This comment was generated by an experimental AI tool.

@imalygin imalygin closed this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated PBJ objects should have proper hashCode() implementation
4 participants