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

chore: migrate preconditions to use non-null error messages #381

Closed
Tracked by #309
ata-nas opened this issue Dec 5, 2024 · 2 comments · Fixed by #415
Closed
Tracked by #309

chore: migrate preconditions to use non-null error messages #381

ata-nas opened this issue Dec 5, 2024 · 2 comments · Fixed by #415
Assignees
Labels
Block Node Issues/PR related to the Block Node. Improvement Code changes driven by non business requirements P2 Required to be completed in the assigned milestone, but may or may not impact release schedule.
Milestone

Comments

@ata-nas
Copy link
Contributor

ata-nas commented Dec 5, 2024

Description

Migrate the preconditions class to use non-null error messages as seen in this comment

e.g.

    private static final String OUT_OF_RANGE_DEFAULT_MESSAGE =
            "The input value [%d] is required to be in the range [%d, %d], inclusive.";
    /**
     * This method asserts a given int is within a range (boundaries
     * included). If the given int is within the range, then we return it,
     * else, an {@link IllegalArgumentException} is thrown.
     *
     * @param toCheck the int value to test
     * @param lowerBoundary the lower boundary
     * @param upperBoundary the upper boundary
     * @return the input {@code toCheck} if it is within the range (boundaries
     * included)
     * @throws IllegalArgumentException if the input int does not pass the test
     */
    public static int requireInRange(final int toCheck, final int lowerBoundary, final int upperBoundary) {
        return requireInRange(toCheck, lowerBoundary, upperBoundary, OUT_OF_RANGE_DEFAULT_MESSAGE);
    }
    /**
     * This method asserts a given int is within a range (boundaries
     * included). If the given int is within the range, then we return it,
     * else, an {@link IllegalArgumentException} is thrown.
     *
     * @param toCheck the int value to check
     * @param lowerBoundary the lower boundary
     * @param upperBoundary the upper boundary
     * @param errorMessage A format string {@see Formatter#syntax} with three
     *     decimal parameters for {@code toCheck}, {@code lowerBoundary},
     *     and {@code upperBoundary}.<br/>
     *     Example: {@code "The input (%d) is out of range (%d, %d)"}
     * @return the input {@code toCheck} if it is within the range (boundaries
     * included)
     * @throws IllegalArgumentException if the input int does not pass the test
     */
    public static int requireInRange(
            final int toCheck, final int lowerBoundary, final int upperBoundary, @NonNull final String errorMessage) {
        if (toCheck >= lowerBoundary && toCheck <= upperBoundary) {
            return toCheck;
        } else {
            final String message = errorMessage.formatted(toCheck, lowerBoundary, upperBoundary);
            throw new IllegalArgumentException(message);
        }
    }
@ata-nas ata-nas self-assigned this Dec 5, 2024
@ata-nas ata-nas added Improvement Code changes driven by non business requirements P2 Required to be completed in the assigned milestone, but may or may not impact release schedule. Block Node Issues/PR related to the Block Node. labels Dec 5, 2024
@ata-nas ata-nas removed their assignment Dec 5, 2024
@ata-nas ata-nas added this to the 0.3.0 milestone Dec 5, 2024
@ata-nas
Copy link
Contributor Author

ata-nas commented Dec 11, 2024

@jsync-swirlds something I have been thinking about, if we do move to this approach, then we would be computing the formatting of the error message every time the method with the default message is invoked. That is redundant since we generally do not expect to have to use this message. This seems like it would have a slight performance hit? Yet again the JVM does a lot of magic behind the scenes, I am unaware if it has some optimizations for such cases, although I would not rely on that. With the current implementation, the default message is computed only after the precondition checks have already failed.

That being said, I think we can omit this change, if this will hurt the performance, which I think it will do, preconditions are used everywhere.

Other option would be to have the default messages not compute anything, but that does not allow us to log values used. Of course, this is what the overload is for, in case we want to add a custom message, but I fear that it would be a great hustle to have to manually interpolate an error message where we need to use the precondition, just to get something that I would generally expect by default. I think the overload to provide a custom message should be used only if it is absolutely necessary and in places where it would hardly be invoked if it would be the case where our custom message will have to be prematurely computed.

I do agree about the point about the error message to not be null which can be enforced with static code analysis, but if the negatives of us having to basically interpolate a String prematurely without actually expecting to use it, we should probably consider to not move on with this.

Opinion?

@jsync-swirlds
Copy link
Member

Producing the message is fairly inexpensive, but if we want to be extra efficient, it isn't hard to do both (make non-null constraint and also only perform interpretation when needed).
Basically, just make the message the format string, and do the interpretation when needed, but still handle the default part via method overload so we get better static analysis.

@ata-nas ata-nas modified the milestones: 0.3.0, 0.4.0 Dec 17, 2024
@ata-nas ata-nas linked a pull request Dec 17, 2024 that will close this issue
2 tasks
@ata-nas ata-nas self-assigned this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Node Issues/PR related to the Block Node. Improvement Code changes driven by non business requirements P2 Required to be completed in the assigned milestone, but may or may not impact release schedule.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants