[jsword-devel] Another patch

[Joe Walker]

DM Smith wrote:

> When I submit a patch, I usually find it easier to wait before doing 
> more changes. So I frequently go the Synchronize view and check for 
> updates.
> 
> Anyway, thanks for applying it.
> 
> Right now I am working on some performance improvements. I have found 
> that some code is being invoked redundantly for a single action. By 
> fixing it, the program is much more responsive in displaying large 
> passages.

Thanks - we are in need of a bit of this. I've been in "premature 
optimization is the root of all evil" mode (to quote someone, but I'm 
not sure who) So I've been ignoring this type of issue, but we do need 
to look at it pre 1.0.

> I am also going through and reviewing the use of exceptions.
> 
> I have found quite a bit of the following construct:
> 
> if (xxx == null)
> {
>    throw new NullPointerException();
> }
> xxx.doSomething();
> 
> While in a few cases there was a message, it did not add to the meaning.
> 
> Where the test represents an expectation of the programmer to do the 
> right thing, I am changing it to:
> assert xxx != null;
> xxx.doSomething();
> or getting rid of it all together.

How about:

if (xxx == null)
{
     assert false;
}
else
{
     xxx.doSomething();
}

That way in live if xxx is null then we avoid the NPE.


> I also have found some overridden methods of the form:
> void doSomething()
> {
>     throw new NullPointerException("Not implemented");
> }

Do I guess you are changing to:

void doSomething()
{
     assert false : "Not implemented";
}

> I am also changing the signature of setBookData to not throw a 
> BookException. It was only needed in one place where the setting Book 
> data also displayed the text. In that one case, I have it reporting the 
> exception directly.

This is on BookDataDisplay?
I guess you are right in saying that only TextPaneBookDataDisplay makes 
use of the exception, however that is because it is the only impl that 
*really* displays the text, the others just proxy to it to add 
scrollbars, tabs and so on.

If we add other ways of displaying like the native browser widgets Sun 
have been demoing recently, they would be similar to 
TextPaneBookDataDisplay.

Is there a benefit to doing this?

Joe.