Bug 189749 - [api][breaking] cleanup ISystemProfileManager
Summary: [api][breaking] cleanup ISystemProfileManager
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 2.0   Edit
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: 2.0   Edit
Assignee: David Dykstal CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks:
 
Reported: 2007-05-29 14:28 EDT by David Dykstal CLA
Modified: 2008-08-13 13:20 EDT (History)
0 users

See Also:
mober.at+eclipse: review+


Attachments
Removing extraneous methods from ISystemProfileManager (14.35 KB, patch)
2007-05-29 14:57 EDT, David Dykstal CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Dykstal CLA 2007-05-29 14:28:20 EDT
There are several inconvenient "convenience" methods cluttering this interface that I'd like to get rid of before R2.0.
Comment 1 David Dykstal CLA 2007-05-29 14:28:52 EDT
I will attach patches of the changes for review.
Comment 2 David Dykstal CLA 2007-05-29 14:57:36 EDT
Created attachment 69118 [details]
Removing extraneous methods from ISystemProfileManager

Please review the attached patch. It removes methods from ISystemProfileManager that should better be left up to consumers of the interface. Also removes the methods from the SystemProfileManager implementation.
Comment 3 Martin Oberhuber CLA 2007-05-29 16:00:29 EDT
Patch looks good to me.
Note that it's a breaking API change because you remove methods.

Note that there is constructor
   Vector(Collection c)
which allows you to write the code slightly more elegant:
   String[] nameArray = manager.getSystemProfileNames();
   Vector names = new Vector(Arrays.asList(nameArray));

Then, taking into account that Vector is synchronized on all access which is not necessary here, a further optimization could be made by allowing ValidatorProfileName and all other validators take a List argument instead of a Vector argument. List is a superinterface of Vector and more generic than Vector. Doing so, you could pass in an ArrayList rather than a Vector -- the ArrayList is not synchronized.

Feel free to make these changes or not, it's up to you.
Comment 4 David Dykstal CLA 2007-05-29 18:14:17 EDT
Patch applied.
Martin's suggestions made as well. All validators changed from Vector to List arguments which generalized and simplifies the code. This bit is a non-breaking change since List is a superInterface of Vector.
Comment 5 Martin Oberhuber CLA 2007-05-30 04:41:51 EDT
(In reply to comment #4)
> This bit is a non-breaking change since List is a superInterface of Vector.

Just for completeness, it would have broken binary compatibility.

But yes, it is source compatible. Which means that client code needs to be recompiled against the new sources in order to run. This means, that we could not have made this change in the 2.0.1 stream. Good to have it now.

BTW, are the validators defined to access the List passed into them in a read-only manner? That would be an important part of Javadoc. Because if they access it read-only, we can pass in arrays by

   ValidatorNames(Arrays.asList(myArray))

directly without having to create a new, modifiable array before. I'd very much recommend specifying them as accessing the list read-only (if they do need to make modifications they could copy the list locally).

Comment 6 Martin Oberhuber CLA 2007-05-30 05:00:37 EDT
Actually, would a Collection rather than a List also work?

Collection is a superinterface of List, and as long as you only like to
  * know whether an element is there or not
  * convert into an array
  * dont depend on any particular sorting or uniqueness constraints
You you should be fine with accepting a Collection as well. This would allow also passing in things like a HashSet if the client prefers such data structures.

But the "read-only" property mentioned before is certainly more interesting than the "Collection or List" property.
Comment 7 David Dykstal CLA 2007-05-30 07:58:23 EDT
I usually steer away from putting Collections in interface definitions from habit since they are so general, but I think in this case the only thing that's needed is to have a exclusion list - in which case a collection would be perfectly OK. I'll take a look.
Comment 8 David Dykstal CLA 2007-05-30 08:07:25 EDT
The read-only idea is good. I'll document it in the javadoc.
Comment 9 David Dykstal CLA 2007-05-30 11:54:47 EDT
Changed list arguments to collections and documented read-only behavior for them.
Comment 10 Martin Oberhuber CLA 2008-08-13 13:20:07 EDT
[target cleanup] 2.0 RC2 was the original target milestone for this bug