== Picketlink IDM API Review == === General === * We should probably implement a good hashcode and equals for our simple implementations in the API. === AbstractIdentityType / IdentityType === * getCreationDate should return a copy / clone of the date * setAttribute might be easier to use if it were vararg * getAttribute Would returning the first value be expected? Maybe it should be renamed to getFirstAttributeValue? * getAttributeValues should also return a clone of the array === Range === * The private no args constructor is redundant because there's already a ctor with args === Queries === * Maybe it's just me, but these are all a type of query and have some things in common. It makes sense to me, and we could make things DRY by pulling those things out into a common interface (maybe also use generics) and cut down on the implementation a bit. === IdentityStore ==== * Having two executeQuery methods, one without the Range would be a little more user friendly when you don't want to use a range or don't need one. * I see no point in having all the get, set, remove methods on IdentityStore. It blurs the line of what you would expect it to do based on its name and doesn't separate concerns. I would figure it would have retrieve and store methods for IdentityType, Group, Role and User then you'd modify those objects as needed and store them when you're done just like with JPA entities. === SecurityConfigurationException / SecurityException === * We should remove the ctors that don't set a message. An exception w/o a message for the user is worthless. -- Jason Porter http://lightguard-jp.blogspot.com http://twitter.com/lightguardjp Software Engineer Open Source Advocate Author of Seam Catch - Next Generation Java Exception Handling PGP key id: 926CCFF5 PGP key available at: keyserver.net, pgp.mit.edu