http://codereview.appspot.com/1726041/diff/6001/7002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java (right):
http://codereview.appspot.com/1726041/diff/6001/7002#newcode171 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:171: protected String getBaseHref(Document doc) { On 2010/06/17 21:21:39, gagan.goku wrote:
On 2010/06/17 20:59:54, johnfargo wrote: > this can probably be private -- when would you see a need to
customize?
made private. IMO reusable helper functions should be made protected,
but this
is fine.
Agreed in general, but in that case they should be public static methods (none of these require state). In this case I don't think that's necessary - I'd just make this private. You test it by way of testing the actual functionality of AbsolutePathReferenceVisitor. http://codereview.appspot.com/1726041/diff/11001/12001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java (right): http://codereview.appspot.com/1726041/diff/11001/12001#newcode48 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:48: } nit: newline here http://codereview.appspot.com/1726041/diff/11001/12001#newcode52 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:52: } same http://codereview.appspot.com/1726041/diff/11001/12001#newcode114 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:114: Element baseTag = elem("base", "href", "http://www.google.com/";); nit: convention is www.example.org http://codereview.appspot.com/1726041/diff/11001/12001#newcode132 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:132: assertNull(visitor.getBaseHref(html.getOwnerDocument())); IMO if you're testing the contract of AbsolutePathReferenceVisitor you don't have to test the helper methods. I'd do so only if you choose to expose them as public static, which seems unnecessary to me. http://codereview.appspot.com/1726041/diff/11001/12001#newcode141 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:141: Element baseTag2 = elem("base", "href", "http://www.google.com/";); the href value should be different to ensure the first base tag is being used. http://codereview.appspot.com/1726041/show
