That works for me - rename them to the have Required in the method
name. Thanks for reviewing my commit!
On Aug 14, 2008, at 2:43 PM, Les Hazlewood wrote:
I totally agree that what you've done is much better than an NPE -
no doubt
about that.
I'm more or less talking about code readability at this point. the
getRequired* versions are self documenting, whereas the get*
versions, just
by looking at them in code, give no indication that an exception
could be
thrown.
I only bring this up because it is my desired convention to return
null
whenever possible from 'getters', reserving exceptions for more
unique or
unusual method names. The getRequired* methods do that, and make
the code
where they're used more readable (kinda like Spring's
getRequiredApplicationContext kinda concept).
It appears that you don't have a problem with me renaming them (and
letting
the IDE make the corresponding changes in code), so I'll do that if
that's
ok.
I also agree that its probably not worth having regular get* methods
at all
since none of our code would use them. We could always add them in
later if
we find that we need it (at which point the getRequired methods
would be
refactored to delegate to them).
On Thu, Aug 14, 2008 at 2:27 PM, Jeremy Haile <[EMAIL PROTECTED]>
wrote:
Well - we either need to throw an exception in WebUtils or modify
ALL code
that references it to throw an exception. Since NO code that
references it
accounts for the fact that it might return null, I thought it was
appropriate that they throw an exception if it isn't defined - and
I updated
the JavaDoc to reflect.
I'm fine adding the required methods or renaming the existing one,
but NO
current code would reference the other methods so I don't really
see much
value at this point. We could just rename them if that makes you
feel
better, but the JavaDoc is pretty explicit as to the behavior and I
can't
think of why we'd want to call the unrequired methods given our
architectural patterns (subclassing for web versions of things).
This came about on my end because our integration tests were
calling those
methods and receiving NPEs which require you to dig through
JSecurity source
code to figure out why they are occurring. I like the fact that
the new
error message is descriptive and consistent vs. a NPE when things
are setup
wrong.
On Aug 14, 2008, at 1:30 PM, Les Hazlewood wrote:
This is a quick question for Jeremy, but anyone please feel free to
chime
in
if you want.
I noticed that WebUtils.getServletRequest and getServletResponse
were
changed to throw an exception if either weren't bound to the
ThreadContext.
I'm not quite sure I feel this is totally appropriate. I see those
methods
as convenient web-utility wrappers around the ThreadContext only,
which
itself does not throw exceptions when something doesn't exist.
What do you think of adding a WebUtils.getRequiredServletRequest and
getRequiredServletResponse that do what you expect (throw an
exception),
and
the regular get* methods don't?
This seems a tad more in line with how the ThreadContext works to
me, with
the added benefit of being more self documenting as well. Would
that be
ok?
