[jsword-devel] Another patch

[DM Smith]

Joe Walker wrote:

> DM Smith wrote:
>
>> 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.
>

The problem with premature optimizations is that it adds a level of 
rigidity to the code that is hard to compensate for. The code tends more 
toward algorithmic, top down, procedural code rather than OO. I find 
that the former is harder to enhance and maintain.

These optimizations are simple coding problems.

>> 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.
In many instances this would be good. That is, it is good when not 
calling doSomething will provide reasonable behavior for the program. 
However, when it is a contractual obligation of the caller to pass a 
non-null pointer so the the method can return a valid value, then dieing 
on a npe or an assert is preferable.

The value of an assert in a robust testing environment is that all uses 
of the method are exercised. (BTW, I have never seen such a robustness! 
And I think that it is theoretically impossible;)

The way the program is currently, it is stating that non-nullness is a 
precondition of the method.

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

The benefit is minimal. It makes the code marginally more maintainable. 
There is no need to check for a checked exception as it will not be thrown.

The reason I did it is that it seemed that the method setBookData was a 
means of establishing what book and passage was being represented by a 
BookDataDisplay. The book and passage should have already been 
established as valid. The invalid combination of the two results in an 
empty BookData (e.g. requesting an OT verse from a NT only bible.) In 
practice the only problem that setBookData would encounter is in the 
accessing of the data or its transformation. These really are not 
BookExceptions. These are IOExceptions, SAXExceptions and the like.

Since Reporter is a valid way from any sw*ng code (e.g. bibledesktop) to 
communicate a problem, I think that it is the better way to go.