WebUtils mods

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

WebUtils mods

Les Hazlewood
Administrator
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?
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: WebUtils mods

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: WebUtils mods

Les Hazlewood
Administrator
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 <[hidden email]> 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?
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: WebUtils mods

Jeremy Haile
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 <[hidden email]>  
> 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?
>>>
>>
>>

Loading...