Improve condition for more readability

[ngocnhan-tran1996]

I read code in package org.springframework.amqp.core and I think some codebase conditions are too complex and we can make it simple for readability purpose.

That's why I create draft PR and I wonder whether I can continue to do this, at least in package org.springframework.amqp.core. Please let me know your thoughts

[artembilan]

This is not OK. You are breaking an atomicity on the property and we may and up with a ConcurrentModificationException from somewhere.

Another my concern that I don't see a reason in so complex logic here.
The declaringAdmins property must be final for better performance.

The code of this method should be as simple as:

public void setAdminsThatShouldDeclare(Object... adminArgs) {
		Assert.notNull(adminArgs, "'admins' must be provided");
		Assert.noNullElements(adminArgs, "'admins' cannot contain null elements");
		this.declaringAdmins.addAll(Arrays.asList(adminArgs));
}

[ngocnhan-tran1996]

Yes, it's more concise but it will break testcase. Will we have any plan to improve codebase in next version? If not, I am not a big fan of else/else-if condition and I will try to contribute that reduce else/else-if condition for more readability

spring-amqp/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/core/RabbitAdminDeclarationTests.java

Lines 296 to 317 in ef23aa9

	public void testAddRemove() {
	Queue queue = new Queue("foo");
	ConnectionFactory cf = mock(ConnectionFactory.class);
	RabbitAdmin admin1 = new RabbitAdmin(cf);
	RabbitAdmin admin2 = new RabbitAdmin(cf);
	queue.setAdminsThatShouldDeclare(admin1, admin2);
	assertThat(queue.getDeclaringAdmins()).hasSize(2);
	queue.setAdminsThatShouldDeclare(admin1);
	assertThat(queue.getDeclaringAdmins()).hasSize(1);
	queue.setAdminsThatShouldDeclare(new Object[] {null});
	assertThat(queue.getDeclaringAdmins()).hasSize(0);
	queue.setAdminsThatShouldDeclare(admin1, admin2);
	assertThat(queue.getDeclaringAdmins()).hasSize(2);
	queue.setAdminsThatShouldDeclare();
	assertThat(queue.getDeclaringAdmins()).hasSize(0);
	queue.setAdminsThatShouldDeclare(admin1, admin2);
	assertThat(queue.getDeclaringAdmins()).hasSize(2);
	queue.setAdminsThatShouldDeclare((AmqpAdmin) null);
	assertThat(queue.getDeclaringAdmins()).hasSize(0);
	queue.setAdminsThatShouldDeclare(admin1, admin2);
	assertThat(queue.getDeclaringAdmins()).hasSize(2);
	queue.setAdminsThatShouldDeclare((Object[]) null);

[artembilan]

I think that is fine to keep then if for null check to avoid a breaking change.

[ngocnhan-tran1996]

I see this changed file takes more your energy reviewing, so I will revert it and create a new PR if we have a plan for breaking change in next version.

In this PR, I will try to improve if-else without breaking change and equals methods for performance in module spring-amqp

[artembilan]

OK. Sounds like a plan.

[artembilan]

This is OK with me.
Do you plan to add more changes?
Or we are good to turn this into a real PR and merge?
Thanks

[ngocnhan-tran1996]

I changed more files and I think it's ready to turn into a real PR.

[artembilan]

@ngocnhan-tran1996 ,

thank you for contribution; looking forward for more!