commons-lang
commons-lang copied to clipboard
apache
Add and use IllegalArgumentExceptions
Add and use IllegalArgumentExceptions to combine creating IllegalArgumentException instances with formatted messages using String#format().
On Tue, Sep 3, 2019 at 8:59 PM Bruno P. Kinoshita [email protected] wrote:
@kinow approved this pull request.
No objections from me, assuming Travis build passes and others are OK with this. When I read your proposal this morning via e-mail I didn't understand what it would look like, but after seeing this PR I guess it makes sense. Thanks!
@Bruno P. Kinoshita [email protected] TY & YW, I just fixed a Checkstyle issue, rebuilding...
Gary
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apache/commons-lang/pull/450?email_source=notifications&email_token=AAJB6NZAZ5VHI2VLCXD6NUDQH4B6FA5CNFSM4ITMFLY2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDRUWXA#pullrequestreview-283331420, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJB6N2KXP4R4FCLFQ7RRDLQH4B6FANCNFSM4ITMFLYQ .
Coverage increased (+0.007%) to 95.228% when pulling 43b385d9456afcbd681f2ffa99e124cdfa7d9396 on garydgregory:IAE_format into 5e7d64d6b2719afb1e5f4785d80d24ac5a19a782 on apache:master.
Coverage decreased (-0.02%) to 95.202% when pulling 95467b2c7fd665eed465d13be9885039bc1477e4 on garydgregory:IAE_format into 5e7d64d6b2719afb1e5f4785d80d24ac5a19a782 on apache:master.
If there should have an IllegalArgumentExceptions , then there should also have a RuntimeExceptions. then there should also have ArithmeticExceptions. and should also have IndexOutOfBoundsExceptions, FileSystemNotFoundExceptions ......
I don't think IllegalArgumentException is special among them in this way (of having a String constructor). Shouldn't we be fair to all of the throwable classes who have a String constructor?
In the case of IllegalArgumentException, based on the usage within Commons Lang, and in the code bases I've worked on and currently see, IllegalArgumentException is used quite often. As you can see from this PR, there are quite a number of files that benefit from this short cut just within Commons Lang.
RuntimeException feels like a code smell to me, one should only rarely consider throwing such a generic exception, I'd even say it's an anti-pattern.
As a provider of a fairly general purpose library we have to find the balance between being general but also following the YAGNI principle. IMO, RuntimeException is not only YAGNI but also an anti-pattern. Extending outlook to other exceptions is fair and I would consider doing the same for other fairly general purpose exceptions like NullPointerException and IllegalStateException. This is just a small step.
These are factory methods, they don't validate anything.
There are several spots you have changed which are subject to conditions and can use Validate.