search

kalpeshasuthar / android-daisy-epub-reader

Automatically exported from code.google.com/p/android-daisy-epub-reader
0 stars 0 forks source link

NewStory: improving the quality of the code - using MetaDataHandler.java as the example #80

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
This story is for a worked example of how to improve the quality of our code. 
The aim is to improve the testability, our confidence in the code, and 
ultimately the code in use as part of the DaisyReader app. I don't claim the 
code will be exceptional or brilliant when we're done. Rather it'll be somewhat 
better than the current implementation. I'll include links and references to 
articles, code snippets, etc.

We start with MetaDataHandler as it's been failing with a NullPointerException 
(NPE) after Sprint 4. So it's a good candidate for improvement as part of 
addressing whatever the underlying trigger of the NPE is.

Here's the stack trace for the exception.

E/AndroidRuntime(10812): java.lang.NullPointerException
E/AndroidRuntime(10812):    at 
java.lang.AbstractStringBuilder.<init>(AbstractStringBuilder.java:86)
E/AndroidRuntime(10812):    at java.lang.StringBuffer.<init>(StringBuffer.java:83)
E/AndroidRuntime(10812):    at 
org.apache.harmony.xml.dom.CharacterDataImpl.setData(CharacterDataImpl.java:88)
E/AndroidRuntime(10812):    at 
org.apache.harmony.xml.dom.CharacterDataImpl.<init>(CharacterDataImpl.java:39)
E/AndroidRuntime(10812):    at 
org.apache.harmony.xml.dom.TextImpl.<init>(TextImpl.java:36)
E/AndroidRuntime(10812):    at 
org.apache.harmony.xml.dom.DocumentImpl.createTextNode(DocumentImpl.java:360)
E/AndroidRuntime(10812):    at 
org.apache.harmony.xml.dom.DocumentImpl.createTextNode(DocumentImpl.java:48)
E/AndroidRuntime(10812):    at 
org.androiddaisyreader.metadata.MetaDataHandler.WriteDataToXmlFile(MetaDataHandl
er.java:137)
E/AndroidRuntime(10812):    at 
org.androiddaisyreader.service.DaisyEbookReaderService$1.run(DaisyEbookReaderSer
vice.java:60)
E/AndroidRuntime(10812):    at java.lang.Thread.run(Thread.java:841)
W/ActivityManager(  376):   Force finishing activity 
org.androiddaisyreader.apps/.DaisyReaderDownloadBooks

Original issue reported on code.google.com by julianharty on 21 Aug 2013 at 7:54

GoogleCodeExporter commented 9 years ago 
First things first. I loaded the project in Eclipse and looked through the 
relevant method calls:
* DaisyEbookReaderService
* WriteDataToXmlFile
and in this method the error is triggered in the following line of code: 
eTitle.appendChild(doc.createTextNode(daisybook.getTitle())); 

My guess is the problem may be caused if daisy book.getTitle() returns null. 
But I don't know for sure. Perhaps we can isolate the call to 
createTextNode(...) and write some skeletal code to see how it behaves when 
it's passed null. We can do this in a separate JUnit test case. 

I also notice this method combines two tasks:
1. creating the document hierarchy in memory
2. writing the result to a file
Perhaps we can usefully improve the code and the testability by splitting the 
code into separate distinct method calls?

Also the method returns void (nothing) and several exceptions are caught and 
logged but otherwise suppressed. NPEs are not caught which caused the NPE to 
cause the activity DaisyReaderDownloadBooks to be terminated. So perhaps we can 
look at what the method should do to communicate relevant problems back to its 
callers?

Original comment by julianharty on 21 Aug 2013 at 8:02

GoogleCodeExporter commented 9 years ago 
When we get to thinking about exceptions and error handling & reporting 
resources such as:
http://www.oracle.com/technetwork/java/effective-exceptions-092345.html and the 
Exceptions chapter of Josh Block's Effective Java (second edition in my case) 
seem relevant

Original comment by julianharty on 21 Aug 2013 at 8:03

GoogleCodeExporter commented 9 years ago 
Right. I've been able to isolate one cause of the NPE which only occurs in the 
Android run time, not J2SE 6. See commits:
https://github.com/julianharty/new-android-daisy-reader/commit/16748aef78fda1448
f7f594583a3c04dfc01577d (where I've described the findings so far)
https://github.com/julianharty/new-android-daisy-reader/commit/f32a59f8272f788ef
9f05243d629b12a21158189 (merge of changes with recent additions from LogiGear)
https://github.com/julianharty/new-android-daisy-reader/commit/7f3475b2cb6d4feff
1a4ac978fbb38ef0fe26d1f (my first spike)

Next step is to read the docs.

Original comment by julianharty on 21 Aug 2013 at 8:40

GoogleCodeExporter commented 9 years ago 
Hmmm 
http://developer.android.com/reference/org/w3c/dom/Document.html#createTextNode(
java.lang.String) tells me nothing relevant :(

Original comment by julianharty on 21 Aug 2013 at 8:41

GoogleCodeExporter commented 9 years ago 
As a thought, perhaps we could replace null with a blank string for logical 
elements such as a book's title if we can't obtain a title? Or should we make 
the title optional in the data structure?

Original comment by julianharty on 21 Aug 2013 at 8:43