You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by David Blevins <da...@visi.com> on 2007/01/17 08:51:06 UTC
Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
On Jan 16, 2007, at 1:48 PM, adc@apache.org wrote:
> Author: adc
> Date: Tue Jan 16 13:48:35 2007
> New Revision: 496867
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=496867
> Log:
> First step at state based contexts and operation checking
Wow, the reworked CoreContext and subclasses are surprisingly clean
and simple.
-David
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Jan 18, 2007, at 2:09 AM, David Blevins wrote:
>
> On Jan 17, 2007, at 10:36 PM, David Blevins wrote:
>
>>
>> On Jan 17, 2007, at 10:02 PM, Alan D. Cabrera wrote:
>>
>>>
>>> On Jan 16, 2007, at 11:51 PM, David Blevins wrote:
>>>
>>>>
>>>> On Jan 16, 2007, at 1:48 PM, adc@apache.org wrote:
>>>>
>>>>> Author: adc
>>>>> Date: Tue Jan 16 13:48:35 2007
>>>>> New Revision: 496867
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=496867
>>>>> Log:
>>>>> First step at state based contexts and operation checking
>>>>
>>>> Wow, the reworked CoreContext and subclasses are surprisingly
>>>> clean and simple.
>>>
>>> Thanks. I wanted to consolidate all the allowed operations into
>>> one place. All checks will go through this class.
>>
>> I like it. I mean that chunk of code could go write on the
>> website as documentation. It leaves us with one set of rules for
>> each bean type, and all the real code is in one class only.
>>
>
> I see you're now removing some of the code I like :) I don't
> think you need all the subclasses anymore, I think what you had was
> teetering on perfection.
>
> If you replaced your constants with enums and massaged your
> comments into checks, you'd have some pretty sweet code that would
> even throw extremely specific error messages like
> "getCallerPrinciple cannot be called in ACTIVATE"
>
> Here's what I mean: http://cwiki.apache.org/confluence/display/
> OPENEJB/EjbContext+Whiteboard
I think that I understand your point but I don't think that I agree.
It seems to me a lot of work just to to get information in the
exception; this information can easily be obtained by the call
stack. It also seems, imho, to obfuscate what operations are
permissible and which are not. Using my current paradigm, the inner
classes map directly to the spec tables.
Regards,
Alan
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by Dain Sundstrom <da...@iq80.com>.
On Jan 18, 2007, at 9:51 PM, Alan D. Cabrera wrote:
>
> On Jan 18, 2007, at 7:18 PM, David Blevins wrote:
>
>>
>> On Jan 18, 2007, at 5:47 PM, Alan D. Cabrera wrote:
>>
>>> I'm not a big fan of switch statements and I think that the code
>>> looks messy, jmho. Also, there's a lot more going on than just
>>> checks for illegal states. There's access to methods that needs
>>> to be controlled. Using this new paradigm, check statements and
>>> impls have sprouted all over the place. Using the "old" method,
>>> the logic/decisions are in a single place.
>>>
>>
>> Easy come easy go I guess :) I will mourn the loss of the Alan
>> code I love and learn to love the new Alan code :)
>>
>> Couple design notes though. Use named inner classes (or top level
>> classes) instead of anonymous inner classes as the Foo$3.class is
>> hard to read in a stack trace.
>
> Agreed. Anonymous inner classes should be restricted to teeny ones
> that are a few lines long, e.g. listeners.
Listeners are dangerous as anonymous inner classes since they hold an
implicit reference to the outer class. This can lead to memory
leaks, so use carefully.
-dain
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Jan 18, 2007, at 7:18 PM, David Blevins wrote:
>
> On Jan 18, 2007, at 5:47 PM, Alan D. Cabrera wrote:
>
>> I'm not a big fan of switch statements and I think that the code
>> looks messy, jmho. Also, there's a lot more going on than just
>> checks for illegal states. There's access to methods that needs
>> to be controlled. Using this new paradigm, check statements and
>> impls have sprouted all over the place. Using the "old" method,
>> the logic/decisions are in a single place.
>>
>
> Easy come easy go I guess :) I will mourn the loss of the Alan
> code I love and learn to love the new Alan code :)
>
> Couple design notes though. Use named inner classes (or top level
> classes) instead of anonymous inner classes as the Foo$3.class is
> hard to read in a stack trace.
Agreed. Anonymous inner classes should be restricted to teeny ones
that are a few lines long, e.g. listeners.
> Also use enums instead of int constants.
Agreed. I think that I'm already doing that.
> And finally, for the love of the rest of us who have to maintain
> all this, try and keep all the impls in one spot if you can.
Yep. I think that we're going to end up w/ the code being in the
bean context w/ encoded references deferring access decisions to the
current bean context.
I was thinking that maybe we could wrap relevant instances obtained
from encoded references and injected instances w/ code that also
checks the current bean context. I know that the spec says that such
references are undefined when the bean is in certain states but I
think that we would be doing our users a service by throwing an
IllegalStateException rather than exhibiting bizarre behavior.
Regards,
Alan
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by David Blevins <da...@visi.com>.
On Jan 18, 2007, at 5:47 PM, Alan D. Cabrera wrote:
>
> On Jan 18, 2007, at 4:36 PM, Dain Sundstrom wrote:
>
>>
>> On Jan 18, 2007, at 4:23 PM, Alan D. Cabrera wrote:
>>
>>> Really? You have to grok those nested case statements and
>>> construct the states of allowable operations in your head.
>>
>> switch (currentOperation) {
>> case SET_CONTEXT:
>> case UNSET_CONTEXT:
>> switch(methodCategory){
>> case getCallerPrincipal:
>> case getRollbackOnly:
>> case isCallerInRole:
>> case setRollbackOnly:
>> case getEJBObject:
>> case getPrimaryKey:
>> case getUserTransaction:
>> throw new IllegalStateException
>> (methodCategory + "cannot be called in " + currentOperation);
>> }
>> What's to grock? For the setContext and unsetContext operations
>> only the listed methods can be called. Looks straight forward to
>> me.
>
> I'm not a big fan of switch statements and I think that the code
> looks messy, jmho. Also, there's a lot more going on than just
> checks for illegal states. There's access to methods that needs to
> be controlled. Using this new paradigm, check statements and impls
> have sprouted all over the place. Using the "old" method, the
> logic/decisions are in a single place.
>
Easy come easy go I guess :) I will mourn the loss of the Alan code
I love and learn to love the new Alan code :)
Couple design notes though. Use named inner classes (or top level
classes) instead of anonymous inner classes as the Foo$3.class is
hard to read in a stack trace. Also use enums instead of int
constants. And finally, for the love of the rest of us who have to
maintain all this, try and keep all the impls in one spot if you can.
-David
>
>
> Regards,
> Alan
>
>
>
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Jan 18, 2007, at 4:36 PM, Dain Sundstrom wrote:
>
> On Jan 18, 2007, at 4:23 PM, Alan D. Cabrera wrote:
>
>> Really? You have to grok those nested case statements and
>> construct the states of allowable operations in your head.
>
> switch (currentOperation) {
> case SET_CONTEXT:
> case UNSET_CONTEXT:
> switch(methodCategory){
> case getCallerPrincipal:
> case getRollbackOnly:
> case isCallerInRole:
> case setRollbackOnly:
> case getEJBObject:
> case getPrimaryKey:
> case getUserTransaction:
> throw new IllegalStateException
> (methodCategory + "cannot be called in " + currentOperation);
> }
> What's to grock? For the setContext and unsetContext operations
> only the listed methods can be called. Looks straight forward to me.
I'm not a big fan of switch statements and I think that the code
looks messy, jmho. Also, there's a lot more going on than just
checks for illegal states. There's access to methods that needs to
be controlled. Using this new paradigm, check statements and impls
have sprouted all over the place. Using the "old" method, the logic/
decisions are in a single place.
Regards,
Alan
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by Dain Sundstrom <da...@iq80.com>.
On Jan 18, 2007, at 4:23 PM, Alan D. Cabrera wrote:
> Really? You have to grok those nested case statements and
> construct the states of allowable operations in your head.
switch (currentOperation) {
case SET_CONTEXT:
case UNSET_CONTEXT:
switch(methodCategory){
case getCallerPrincipal:
case getRollbackOnly:
case isCallerInRole:
case setRollbackOnly:
case getEJBObject:
case getPrimaryKey:
case getUserTransaction:
throw new IllegalStateException
(methodCategory + "cannot be called in " + currentOperation);
}
What's to grock? For the setContext and unsetContext operations only
the listed methods can be called. Looks straight forward to me.
> You really like that check method better?
Yes. Once you get the switch right, the code never changes (until
sun releases a new ejb spec), and it is very concise and readable.
-dain
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Jan 18, 2007, at 8:35 AM, Dain Sundstrom wrote:
> On Jan 18, 2007, at 2:09 AM, David Blevins wrote:
>
>> On Jan 17, 2007, at 10:36 PM, David Blevins wrote:
>>
>>> On Jan 17, 2007, at 10:02 PM, Alan D. Cabrera wrote:
>>>
>>>> On Jan 16, 2007, at 11:51 PM, David Blevins wrote:
>>>>
>>>>> On Jan 16, 2007, at 1:48 PM, adc@apache.org wrote:
>>>>>
>>>>>> Author: adc
>>>>>> Date: Tue Jan 16 13:48:35 2007
>>>>>> New Revision: 496867
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=496867
>>>>>> Log:
>>>>>> First step at state based contexts and operation checking
>>>>>
>>>>> Wow, the reworked CoreContext and subclasses are surprisingly
>>>>> clean and simple.
>>>>
>>>> Thanks. I wanted to consolidate all the allowed operations into
>>>> one place. All checks will go through this class.
>>>
>>> I like it. I mean that chunk of code could go write on the
>>> website as documentation. It leaves us with one set of rules for
>>> each bean type, and all the real code is in one class only.
>>>
>>
>> I see you're now removing some of the code I like :) I don't
>> think you need all the subclasses anymore, I think what you had
>> was teetering on perfection.
>>
>> If you replaced your constants with enums and massaged your
>> comments into checks, you'd have some pretty sweet code that would
>> even throw extremely specific error messages like
>> "getCallerPrinciple cannot be called in ACTIVATE"
>>
>> Here's what I mean: http://cwiki.apache.org/confluence/display/
>> OPENEJB/EjbContext+Whiteboard
>>
>> Thoughts?
>
> Wow this code is very readable. It is amazing how the new Java5
> stuff like enums are really improving readability.
Really? You have to grok those nested case statements and construct
the states of allowable operations in your head. You really like
that check method better?
Regards,
Alan
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by Dain Sundstrom <da...@iq80.com>.
On Jan 18, 2007, at 2:09 AM, David Blevins wrote:
> On Jan 17, 2007, at 10:36 PM, David Blevins wrote:
>
>> On Jan 17, 2007, at 10:02 PM, Alan D. Cabrera wrote:
>>
>>> On Jan 16, 2007, at 11:51 PM, David Blevins wrote:
>>>
>>>> On Jan 16, 2007, at 1:48 PM, adc@apache.org wrote:
>>>>
>>>>> Author: adc
>>>>> Date: Tue Jan 16 13:48:35 2007
>>>>> New Revision: 496867
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=496867
>>>>> Log:
>>>>> First step at state based contexts and operation checking
>>>>
>>>> Wow, the reworked CoreContext and subclasses are surprisingly
>>>> clean and simple.
>>>
>>> Thanks. I wanted to consolidate all the allowed operations into
>>> one place. All checks will go through this class.
>>
>> I like it. I mean that chunk of code could go write on the
>> website as documentation. It leaves us with one set of rules for
>> each bean type, and all the real code is in one class only.
>>
>
> I see you're now removing some of the code I like :) I don't
> think you need all the subclasses anymore, I think what you had was
> teetering on perfection.
>
> If you replaced your constants with enums and massaged your
> comments into checks, you'd have some pretty sweet code that would
> even throw extremely specific error messages like
> "getCallerPrinciple cannot be called in ACTIVATE"
>
> Here's what I mean: http://cwiki.apache.org/confluence/display/
> OPENEJB/EjbContext+Whiteboard
>
> Thoughts?
Wow this code is very readable. It is amazing how the new Java5
stuff like enums are really improving readability.
-dain
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by David Blevins <da...@visi.com>.
On Jan 17, 2007, at 10:36 PM, David Blevins wrote:
>
> On Jan 17, 2007, at 10:02 PM, Alan D. Cabrera wrote:
>
>>
>> On Jan 16, 2007, at 11:51 PM, David Blevins wrote:
>>
>>>
>>> On Jan 16, 2007, at 1:48 PM, adc@apache.org wrote:
>>>
>>>> Author: adc
>>>> Date: Tue Jan 16 13:48:35 2007
>>>> New Revision: 496867
>>>>
>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=496867
>>>> Log:
>>>> First step at state based contexts and operation checking
>>>
>>> Wow, the reworked CoreContext and subclasses are surprisingly
>>> clean and simple.
>>
>> Thanks. I wanted to consolidate all the allowed operations into
>> one place. All checks will go through this class.
>
> I like it. I mean that chunk of code could go write on the website
> as documentation. It leaves us with one set of rules for each bean
> type, and all the real code is in one class only.
>
I see you're now removing some of the code I like :) I don't think
you need all the subclasses anymore, I think what you had was
teetering on perfection.
If you replaced your constants with enums and massaged your comments
into checks, you'd have some pretty sweet code that would even throw
extremely specific error messages like "getCallerPrinciple cannot be
called in ACTIVATE"
Here's what I mean: http://cwiki.apache.org/confluence/display/
OPENEJB/EjbContext+Whiteboard
Thoughts?
-David
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by David Blevins <da...@visi.com>.
On Jan 17, 2007, at 10:02 PM, Alan D. Cabrera wrote:
>
> On Jan 16, 2007, at 11:51 PM, David Blevins wrote:
>
>>
>> On Jan 16, 2007, at 1:48 PM, adc@apache.org wrote:
>>
>>> Author: adc
>>> Date: Tue Jan 16 13:48:35 2007
>>> New Revision: 496867
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=496867
>>> Log:
>>> First step at state based contexts and operation checking
>>
>> Wow, the reworked CoreContext and subclasses are surprisingly
>> clean and simple.
>
> Thanks. I wanted to consolidate all the allowed operations into
> one place. All checks will go through this class.
I like it. I mean that chunk of code could go write on the website
as documentation. It leaves us with one set of rules for each bean
type, and all the real code is in one class only.
-David
>
> Regards,
> Alan
>
>
Re: Nice work (Re: svn commit: r496867 - in /incubator/openejb/trunk/openejb3/container/openej)
Posted by "Alan D. Cabrera" <li...@toolazydogs.com>.
On Jan 16, 2007, at 11:51 PM, David Blevins wrote:
>
> On Jan 16, 2007, at 1:48 PM, adc@apache.org wrote:
>
>> Author: adc
>> Date: Tue Jan 16 13:48:35 2007
>> New Revision: 496867
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=496867
>> Log:
>> First step at state based contexts and operation checking
>
> Wow, the reworked CoreContext and subclasses are surprisingly clean
> and simple.
Thanks. I wanted to consolidate all the allowed operations into one
place. All checks will go through this class.
Regards,
Alan