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.
Summary: | openidex.search API change | ||
---|---|---|---|
Product: | utilities | Reporter: | Martin Roskanin <mroskanin> |
Component: | Search | Assignee: | Martin Roskanin <mroskanin> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | apireviews, issues, rypacek |
Priority: | P1 | Keywords: | API, API_REVIEW_FAST |
Version: | 4.x | ||
Hardware: | PC | ||
OS: | Windows ME/2000 | ||
Issue Type: | TASK | Exception Reporter: | |
Bug Depends on: | |||
Bug Blocks: | 51964 | ||
Attachments: |
diff of desired API changes
Corrected diff. Thanks to Ondra for finding this. SearchPattern methods equals and hashCode + tests implemented |
Description
Martin Roskanin
2005-01-07 16:05:11 UTC
Created attachment 19557 [details]
diff of desired API changes
Martin you need to reassign the issue and mark it with appropriate keywords if you want a real API review, not just put apireviews on CC. See: http://openide.netbeans.org/tutorial/review-steps.html Thanks Jesse. I hope Fast-Track should be enough here. There is an error in previous diff. Missing return statement in SearchHistory.java line 91. I am attaching a corrected diff for completeness. Created attachment 19594 [details]
Corrected diff. Thanks to Ondra for finding this.
Please use diff -u for readability whenever producing patches. Also tip: pass -q to cvs (i.e. "cvs -q di -u ....") to suppress irrelevant messages about folders being traversed. Style/consistency: use List/*<SearchPattern>*/ rather than List/*SearchPattern*/ (will be easier to find when we move to generics). LAST_SELECTED_SEARCH_PATTERN should be "lastSelected" for consistency with JavaBeans conventions, I guess. Otherwise looks fine to me. I'd like to ask for a little change: could you please define SearchPattern.equals (and hashCode) that compare the values of the objects? Thanks. yes, it's a good idea. i will implement it. Thanks. Created attachment 19662 [details]
SearchPattern methods equals and hashCode + tests implemented
I am about to commit the proposed changes to CVS. One last bit from me. I've just looked at the impls of equals and hashCode and it looks like you don't expect SearchPattern.searchExpression to be null. If that's the case, it might be a good idea to assert this in the constructor to make this assumption sound and clear to the users (like me ;-) Thanks Ondra. implemented: public static SearchPattern create(String searchExpression, boolean wholeWords, boolean matchCase, boolean regExp){ assert searchExpression != null : "searchExpression cannot be null"; // NOI18N return new SearchPattern(searchExpression, wholeWords, matchCase, regExp); } Never use assertions to check incoming arguments to an API method! You must use proper exceptions (e.g. IllegalArgumentException), which should be listed in the method's throws clause and mentioned in a @throws tag in Javadoc. As an exception, *all* NB APIs are expected to forbid null parameters unless they explicitly permit a null parameter and assign it a meaning (similar for return values), so it is not strictly necessary to document that a parameter must be non-null (though it never hurts). More on assertions vs. exceptions: http://java.sun.com/j2se/1.5.0/docs/guide/language/assert.html#usage Jesse, thanks for the correction. I removed assertion and put a notice into a javadoc. Implemented in [maintrunk] Checking in api/apichanges.xml; /cvs/openidex/api/apichanges.xml,v <-- apichanges.xml new revision: 1.3; previous revision: 1.2 done Processing log script arguments... More commits to come... RCS file: /cvs/openidex/src/org/openidex/search/SearchHistory.java,v done Checking in src/org/openidex/search/SearchHistory.java; /cvs/openidex/src/org/openidex/search/SearchHistory.java,v <-- SearchHistory.java initial revision: 1.1 done RCS file: /cvs/openidex/src/org/openidex/search/SearchPattern.java,v done Checking in src/org/openidex/search/SearchPattern.java; /cvs/openidex/src/org/openidex/search/SearchPattern.java,v <-- SearchPattern.java initial revision: 1.1 done Processing log script arguments... More commits to come... RCS file: /cvs/openidex/test/unit/src/org/openidex/search/SearchHistoryTest.java,v done Checking in test/unit/src/org/openidex/search/SearchHistoryTest.java; /cvs/openidex/test/unit/src/org/openidex/search/SearchHistoryTest.java,v <-- SearchHistoryTest.java initial revision: 1.1 done Jesse, you are right and you are not. It's true that if the exception is to be part of the behaviour of the program, than it, of course, has to be checked and thrown "manually" (with if and throw). On the other hand, I doubt that every public method in every api has all it's arguments checked for null and other well-formedness conditions. It may be so both because of typing (the if and throw version is quite verbose to type everywhere) and efficiency reasons. In my oppinion, it's better to have assert, which is not part of the semantics of the api at least for the development phase, then to have nothing and purely undefined behaviour. Certainly it's better to have an assert than nothing at all, it's just better still to have a typed exception. Possible exception: in some cases it is prohibitively expensive to do a full check on incoming arguments (this is pretty rare but it happens). Then you could use an assert to save time during normal operation (no -ea), or even better use boolean asserts = false; assert asserts = true; if (asserts) { if (!someComplexArgCheck()) { throw new IllegalArgumentException("..."); } } To be continued on nbdev... |