This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 129884 - API review of SourceForBinaryQueryImplemenation2Base
Summary: API review of SourceForBinaryQueryImplemenation2Base
Alias: None
Product: java
Classification: Unclassified
Component: Classpath (show other bugs)
Version: 6.x
Hardware: All All
: P2 blocker (vote)
Assignee: Tomas Zezula
Depends on: 130201
  Show dependency tree
Reported: 2008-03-12 11:36 UTC by Tomas Zezula
Modified: 2008-03-14 21:51 UTC (History)
0 users

See Also:
Issue Type: TASK
Exception Reporter:

Diff (29.82 KB, patch)
2008-03-12 12:55 UTC, Tomas Zezula
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Zezula 2008-03-12 11:36:35 UTC
Added a support base class for delegating SourceForBinaryQueryImplementation2.
The base class provides a method for creating a wrapper converting SFBQ.Result to SFBQImpl2.Result.
Stable in 6.1, covered by unit tests.
Comment 1 Tomas Zezula 2008-03-12 12:55:23 UTC
Created attachment 58225 [details]
Comment 2 Jaroslav Tulach 2008-03-12 13:11:05 UTC
Y01 It would imho be better to have static method asResult for converting the results (P4), this kind of protection - 
e.g. protected method with no needed instance context in a publicly subclassable class reminds me 
TextAction.getLastComponent() - can be easily workarounded

Y02 You might use instanceof check in the asResult method to return the argument directly if it is already of the 
right type.
Comment 3 Tomas Zezula 2008-03-12 13:30:04 UTC
Thanks Jarda.
Y1: I m not very friend of static support classes. The instance context seems to me as the best solution.
This method is needed only by proxy SFBQImpls - currently there are two, if we make this method public I am afraid someone will use
it for something completely else.
Y2: Right, I agree and I will add it. 
Comment 4 Andrei Badea 2008-03-12 14:42:24 UTC
Looks good to me. Just a small nitpick, not meant as an integration blocker. I'm not a big fan of methods that allow
null as a valid parameter value and begin with:

if (parameter == null) { return null; }
Comment 5 Tomas Zezula 2008-03-12 15:15:21 UTC
Yes, I understand this. Originally I hadn't null as a valid argument. But allowing null asa valid argument simplified ProjectSFBQ code from:

Result r = delegate.findSourceRoot (url);
if (r == null) {
    return null;
return asResult (r);

return asResult (delegate.findSourceRoot (url));

Comment 6 Milos Kleint 2008-03-12 15:23:32 UTC
looks good to me.
Comment 7 Tomas Zezula 2008-03-13 09:24:18 UTC
I am going to integrate it.
Comment 8 Andrei Badea 2008-03-13 11:03:11 UTC
Re. desc6:

Result r = delegate.findSourceRoot (url);
return r != null ? asResult(r) : null;

is a bit worse than your way, but OTOH it makes the API more clear.
Comment 9 Tomas Zezula 2008-03-13 11:10:39 UTC
ab: OK, I have no strong opinion on this. I will change this - null will not be allowed.

Comment 10 Tomas Zezula 2008-03-13 13:21:56 UTC
Fixed in: