Suggestion: Increases readability of PageableExecutionUtils code

[erie0210]

I noticed PageableExecutionUtils code could potentially benefit from refactoring. I found it too complex due to multiple branches of if statements. I think these adjustments could improve readability of the codebase. I am open to any feedback or suggestions you might have. I would be happy to open a pull request with these changes if needed. Please let me know your thoughts.

Here's the suggested code. All tests passed.

Thank you.

AS_IS

public abstract class PageableExecutionUtils {

	private PageableExecutionUtils() {}

	public static <T> Page<T> getPage(List<T> content, Pageable pageable, LongSupplier totalSupplier) {

		Assert.notNull(content, "Content must not be null");
		Assert.notNull(pageable, "Pageable must not be null");
		Assert.notNull(totalSupplier, "TotalSupplier must not be null");

		if (pageable.isUnpaged() || pageable.getOffset() == 0) {

			if (pageable.isUnpaged() || pageable.getPageSize() > content.size()) {
				return new PageImpl<>(content, pageable, content.size());
			}

			return new PageImpl<>(content, pageable, totalSupplier.getAsLong());
		}

		if (content.size() != 0 && pageable.getPageSize() > content.size()) {
			return new PageImpl<>(content, pageable, pageable.getOffset() + content.size());
		}

		return new PageImpl<>(content, pageable, totalSupplier.getAsLong());
	}
}

TO_BE

public abstract class PageableExecutionUtils {

	private PageableExecutionUtils() {}

	public static <T> Page<T> getPage(List<T> content, Pageable pageable, LongSupplier totalSupplier) {

		Assert.notNull(content, "Content must not be null");
		Assert.notNull(pageable, "Pageable must not be null");
		Assert.notNull(totalSupplier, "TotalSupplier must not be null");

		if (pageable.isUnpaged() || isFirstPageWithLessThanOneFullPage(content, pageable)) {
			return new PageImpl<>(content, pageable, content.size());
		}

		if (isSubsequentPageWithLessThanOneFullPage(content, pageable)) {
			return new PageImpl<>(content, pageable, pageable.getOffset() + content.size());
		}

		return new PageImpl<>(content, pageable, totalSupplier.getAsLong());
	}

	private static <T> boolean isFirstPageWithLessThanOneFullPage(List<T> content, Pageable pageable) {
		return pageable.getOffset() == 0 && pageable.getPageSize() > content.size();
	}

	private static <T> boolean isSubsequentPageWithLessThanOneFullPage(List<T> content, Pageable pageable) {
		return pageable.getOffset() != 0 && !content.isEmpty() && pageable.getPageSize() > content.size();
	}
}

[mp911de]

Readability is subjective. I find isSubsequentPageWithLessThanOneFullPage hard to parse although it makes the concept explicit. It's always a balance between tradeoffs.

[erie0210]

Thank you for your feedback on the naming of the method isSubsequentPageWithLessThanOneFullPage. I understand the importance of balancing explicitness and readability in method names. I took the current name directly from the test code to maintain consistency and clarity regarding its functionality. However, I agree that the name can be quite lengthy and difficult to parse.

If you think it's worth refactoring, I could try changing the name. @mp911de

[schauder]

If you think it's worth refactoring, I could try changing the name. @mp911de

In general a refactoring would be a good thing.

Maybe the solution for good names comes with splitting the methods differently, like separating the question first/subsequent Page from the question of full/partial page.

[schauder]

Do you want to come up with a PR?

[erie0210]

Sure, thank you! I'll work on the PR and let you know. @schauder

[erie0210]

Submitted PR for the issue above. Your feedback is appreciated! :) Thank you! @schauder