spring-framework icon indicating copy to clipboard operation
spring-framework copied to clipboard

Clarify intention of `toString()` usage in log messages in `HandlerMethod`

Open wonyongg opened this issue 5 months ago • 3 comments

The formatErrorForReturnValue method in ServletInvocableHandlerMethod previously included an unnecessary call to toString() and a dangling "in" at the end of the formatted string.

Since the surrounding context already provides sufficient information, both the toString() call and the " in" suffix have been removed to improve clarity.

No functional changes.

wonyongg avatar Jun 15 '25 12:06 wonyongg

After revisiting the HandlerMethod implementation, I realized that I misunderstood the original purpose of the toString() call.

Initially, I thought the usage of toString() was redundant or unclear. However, I now understand that it returns the description string, which actually provides useful context like ClassName#methodName(args...). sorry for the confusion.

That said, I'd like to propose a small improvement.

The method name toString() by itself doesn't make the intention very clear to someone unfamiliar with the HandlerMethod class. I believe replacing it with getShortLogMessage() could make the code more readable and self-explanatory, especially for contributors or maintainers not deeply familiar with this class.

The output remains similar, but the method name communicates its purpose more directly.

If this suggestion doesn't align with the direction of the project, please feel free to suggest an alternative approach or close this PR if it's unnecessary.

wonyongg avatar Jun 15 '25 15:06 wonyongg

The method name toString() by itself doesn't make the intention very clear to someone unfamiliar with the HandlerMethod class. I believe replacing it with getShortLogMessage() could make the code more readable and self-explanatory, especially for contributors or maintainers not deeply familiar with this class.

The output remains similar, but the method name communicates its purpose more directly.

If this suggestion doesn't align with the direction of the project, please feel free to suggest an alternative approach or close this PR if it's unnecessary.

I'm not so sure that it's worth changing things here.

If we were to change something, I suppose we could either make the HandlerMethod.description field protected or introduce a getDescription() method in HandlerMethod.

We'll discuss it within the team and get back to you.

sbrannen avatar Jun 16 '25 14:06 sbrannen

@sbrannen Thank you for your feedback! I’ll be waiting to hear back after your team discussion.

wonyongg avatar Jun 16 '25 15:06 wonyongg

Instead of changing the code or introducing a dedicated method to retrieve the description, I think we should just update the Javadoc for HandlerMethod.toString() to document that it's used in log/error messages and should typically include the method signature of the handler method.

If you would like to repurpose this PR to do that, feel free to do so.

Otherwise, I'll just add the Javadoc myself.

sbrannen avatar Jun 18 '25 08:06 sbrannen

Thanks for the feedback! 🙏 I’ve removed the previous changes and updated this PR to only include the Javadoc for HandlerMethod.toString() as suggested. I’d appreciate it if you could take a quick look and let me know if the updated Javadoc looks good to you.

wonyongg avatar Jun 18 '25 09:06 wonyongg

This has been merged into main.

Thanks

sbrannen avatar Jun 18 '25 11:06 sbrannen