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? >> > >
