some remarks on LoadableDetachableModel.getObject()

[Peter Dotchev <do...@gmail.com>]

Hi,

I'm new to wicket so I'm still learning from the source.
So I came across this method in LoadableDetachableModel

	public Object getObject()
	{
		if (!attached)
		{
			attached = true;
			transientModelObject = load();

			if (log.isDebugEnabled())
			{
				log.debug("loaded transient object " + transientModelObject + " for " +
this
						+ ", requestCycle " + RequestCycle.get());
			}

			onAttach();
		}
		return transientModelObject;
	}

IMHO there two things that might be improved.

The flag attached is set before doing the actual job. This is not exception
safe. As load() is an abstract method, it might be implemented to do
anything. If it throws an exception, attached has been set but
transientModelObject is left in an unknown state. Next time this method is
called, it will return some garbage.
I think it will be better if attached is set after the cal to load().

The other thing is the use of log.isDebugEnabled(). It seems to be employed
in quite number of places in wicket. I guess the idea is to improve
performance. The same effect can be achieved in a simpler way, just do
log.debug("loaded transient object {} for {}, requestCycle {}", new Object[]
{transientModelObject, this, RequestCycle.get()});
(Yes, I also would like slf4j to use varargs from Java 5.)
Follow this link for more info.
http://www.slf4j.org/faq.html#logging_performance
http://www.slf4j.org/faq.html#logging_performance 

Best regards,
Peter

-- 
View this message in context: http://www.nabble.com/some-remarks-on-LoadableDetachableModel.getObject%28%29-tf4313719.html#a12281762
Sent from the Wicket - Dev mailing list archive at Nabble.com.

Re: some remarks on LoadableDetachableModel.getObject()

[Eelco Hillenius <ee...@gmail.com>]

> If just4log (http://just4log.sourceforge.net) supported slf4j, then it would
> give the best of both worlds.

Haha, that's a good one. I was actually wondering why no-one wrote
that when I wrote the first reply, but of course someone did :)

I don't know, we can't just keep switching log frameworks ever other
Wicket version. I'm ok with the current situation for now. Maybe we
should bring the thread up when we're starting with 1.4 :)

Eelco

Re: some remarks on LoadableDetachableModel.getObject()

[Kent Tong <ke...@cpttm.org.mo>]

Eelco Hillenius wrote:
> 
> It would perform slightly better when debugging is on, but a bit worse
> if it is off. That single isDebugEnabled would still be cheaper than
> letting log.debug demine whether the log level is on *and*
> constructing the string and getting the request cycle.
> 

If just4log (http://just4log.sourceforge.net) supported slf4j, then it would
give the best of both worlds.
-- 
View this message in context: http://www.nabble.com/some-remarks-on-LoadableDetachableModel.getObject%28%29-tf4313719.html#a12306192
Sent from the Wicket - Dev mailing list archive at Nabble.com.

Re: some remarks on LoadableDetachableModel.getObject()

[Eelco Hillenius <ee...@gmail.com>]

> I'm new to wicket so I'm still learning from the source.
> So I came across this method in LoadableDetachableModel
>
>         public Object getObject()
>         {
>                 if (!attached)
>                 {
>                         attached = true;
>                         transientModelObject = load();
>
>                         if (log.isDebugEnabled())
>                         {
>                                 log.debug("loaded transient object " + transientModelObject + " for " +
> this
>                                                 + ", requestCycle " + RequestCycle.get());
>                         }
>
>                         onAttach();
>                 }
>                 return transientModelObject;
>         }
>
> IMHO there two things that might be improved.
>
> The flag attached is set before doing the actual job. This is not exception
> safe. As load() is an abstract method, it might be implemented to do
> anything. If it throws an exception, attached has been set but
> transientModelObject is left in an unknown state. Next time this method is
> called, it will return some garbage.
> I think it will be better if attached is set after the cal to load().

We can consider that, though I wonder whether it is an actual problem
since if an exception occurs (mind you: runtime exception), the
component/ page shouldn't be rendered, period. If it should, you would
be misusing exceptions.

> The other thing is the use of log.isDebugEnabled(). It seems to be employed
> in quite number of places in wicket. I guess the idea is to improve
> performance. The same effect can be achieved in a simpler way, just do
> log.debug("loaded transient object {} for {}, requestCycle {}", new Object[]
> {transientModelObject, this, RequestCycle.get()});

It would perform slightly better when debugging is on, but a bit worse
if it is off. That single isDebugEnabled would still be cheaper than
letting log.debug demine whether the log level is on *and*
constructing the string and getting the request cycle.

Eelco