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

Update tests two bucket #2877

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jagdish-15
Copy link
Contributor

Pull Request

This PR adds 3 new tests to the Two Bucket exercise to sync with the problem-specifications repository. Two of the newly added tests check for error handling, which was not tested before. I am also unsure about the error message—should I keep it as it is in the problem-specifications repo or modify it to be more informative?

Let me know your thoughts on this.


Reviewer Resources:

Track Policies

@kahgoh
Copy link
Member

kahgoh commented Dec 26, 2024

I think with the error messages I think it is fine to leave them out of the exception. But, I think we should make our own exception instead of reusing IllegalArgumentException. IllegalArgumentException is used to indicate when one of the arguments to the method is bad (like if if one of them is negative). It doesn't suit cases where the arguments are good, but we can't reached the required amount. If we can't reach the value, I'd use a different exception, perhaps making a custom one for the exercise.

@jagdish-15
Copy link
Contributor Author

jagdish-15 commented Dec 26, 2024

@kahgoh, thank you for the suggestion—it does make a lot of sense to create a custom exception for this purpose.
I imagine it would look something like this:

public class UnreachableGoalException extends RuntimeException {
    public UnreachableGoalException() {
        super();
    }

    public UnreachableGoalException(String message) {
        super(message);
    }

    public UnreachableGoalException(String message, Throwable cause) {
        super(message, cause);
    }
}

That said, when I think about this, we've used IllegalArgumentException in similar scenarios before—like when the target amount couldn't be reached with the available denominations in the "Change" exercise. I've noticed this approach across many other exercises as well. (But some exercises like "Calculator Conundrum" do use custom exceptions)

While your suggestion is undoubtedly a more "proper" and robust approach, it might feel a little inconsistent with the conventions used elsewhere. What are your thoughts on this? How do you think I should proceed?

@kahgoh
Copy link
Member

kahgoh commented Dec 28, 2024

I think there is also precedence for making our own exceptions for our exercises, like BinarySearch and TreeBuilding. But yeah, the think the exception would look something like that.

@jagdish-15
Copy link
Contributor Author

Sure, I'll get right on it as soon as possible, since I already have the body of the custom Exception ready. However, I'll implement it the way it's done in exercises like "Binary Search" and "Tree Building," instead of following the approach in "Calculator Conundrum," as that causes build failure issues for Windows users (as highlighted in the forum discussion).

I was also considering implementing similar custom Exceptions in exercises like "Change," since an unassignable change value shouldn’t technically throw an IllegalArgumentException. If you have other exercises in mind that would benefit from custom Exceptions, feel free to let me know. We could even extend this improvement across the entire track—I’d be more than happy to take on that task!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants