Skip to content
New issue

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

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

Already on GitHub? # to your account

No Issue : Internal Refactoring to improve code readability. #3422

Merged

Conversation

chickenchickenlove
Copy link
Contributor

Motivation:

  • There are some codes using *.size() > 0 or *.size() == 0 in spring-kafka.
  • There is code where the constructor could be reused, but it was not.
  • There is code where a null value can occur, but it does not indicate that null is possible.

Modifications:

  • Replace a.size() > 0 with !a.isEmpty().
  • Replace a.size() == 0 with a.isEmpty().
  • Reuse constructor already existed to remove duplicate constructor logic.
  • Add @nullable annotations to field, and method. Also, add warning log.

Result:

  • There is only one side effect which developer can observe.
    • when BatchMessagingMessageListenerAdapter#setBatchMessageConverter() is executed, new warning log can be occur.

* @since 1.1
*/
public class BatchMessagingMessageConverter implements BatchMessageConverter {

protected final LogAccessor logger = new LogAccessor(LogFactory.getLog(getClass())); // NOSONAR

@Nullable
Copy link
Contributor Author

@chickenchickenlove chickenchickenlove Aug 15, 2024

Choose a reason for hiding this comment

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

FYI

RecordMessageConverter can be null. Please refer to codes below.

// BatchMessagingMessageConverter.java
	public BatchMessagingMessageConverter() {
		this(null);
	}

	public BatchMessagingMessageConverter(RecordMessageConverter recordConverter) {
		this.recordConverter = recordConverter;
		if (JacksonPresent.isJackson2Present()) {
			this.headerMapper = new DefaultKafkaHeaderMapper();
		}
	}

Then, getRecordMessageConverter() can return null.
BatchMessagingMessageListenerAdapter#setBatchMessageConverter() call getRecordMessageConverter() internally to set messageConverter.

If user created BatchMessagingMessageConverter by using default constructor, there is no message converter even if BatchMessagingMessageListenerAdapter#setBatchMessageConverter() is completed without any warning logs.

@sobychacko sobychacko added this to the 3.3.0-M2 milestone Aug 15, 2024
@sobychacko sobychacko merged commit 78d7724 into spring-projects:main Aug 15, 2024
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants