This page exists for two purposes. 1) is to propose a concrete commit for changes to PersonDirNameFinder, PersonDirNameFinderFactory, and an associated testcase. 2) is to provide an example of refactoring uPortal 2 code to be more testable and less tightly coupled to static services.
The players
PersonDirNameFinder is the IEntityNameFinder implementation which knows how to lookup displayName attributes for entities representing uPortal users.
/**
* PersonDirectory implementation of <code>IEntityNameFinder</code> for <code>IPersons</code>.
* @version $Revision: 1.12 $
*/
public class PersonDirNameFinder
implements IEntityNameFinder {
// Singleton instance:
private static IEntityNameFinder singleton;
private static IPersonAttributeDao pa;
// Our cache of entity names:
private SoftHashMap names;
/**
* ReferenceIPersonNameFinder constructor comment.
*/
private PersonDirNameFinder () throws SQLException
{
super();
pa = PersonDirectory.getPersonAttributeDao();
names = new SoftHashMap();
}
/**
* Given the key, returns the entity's name.
* @param key java.lang.String
*/
public String getName (String key) throws Exception {
if (primGetNames().get(key) == null) {
primGetNames().put(key, primGetName(key));
}
return (String)primGetNames().get(key);
}
/**
* Given an array of keys, returns the names of the entities. If a key
* is not found, its name will be null.
* @param keys java.lang.String[]
*/
public java.util.Map getNames (java.lang.String[] keys) throws Exception {
Map selectedNames = new HashMap();
for (int i = 0; i < keys.length; i++) {
String name = getName(keys[i]);
selectedNames.put(keys[i], name);
}
return selectedNames;
}
/**
* Returns the entity type for this <code>IEntityFinder</code>.
* @return java.lang.Class
*/
public Class getType () {
return org.jasig.portal.security.IPerson.class;
}
/**
* put your documentation comment here
* @param key
* @return
* @exception java.sql.SQLException
*/
private String primGetName (String key) throws java.sql.SQLException {
String name = key;
Map userInfo = pa.getUserAttributes(name);
Object displayName = userInfo.get("displayName");
String displayNameStr = "";
if (displayName != null)
{
if (displayName instanceof java.util.List)
{
List displayNameList = (List) displayName;
if (! displayNameList.isEmpty() )
{ displayNameStr = (String)displayNameList.get(0); }
}
else displayNameStr = (String)displayName;
if (! displayNameStr.trim().equals(""))
{ name = displayNameStr; }
}
return name;
}
/**
* @return java.util.Map
*/
private Map primGetNames () {
return names;
}
/**
* @return IEntityNameFinder
*/
public static synchronized IEntityNameFinder singleton () throws SQLException {
if (singleton == null) {
singleton = new PersonDirNameFinder();
}
return singleton;
}
}
PersonDirNameFinder is a static singleton. It is implemented by delegation to the PersonDirectory static service to lookup user attributes. Its constructor is private and it provides a public static synchronized singleton() method whereby code wishing to use PersonDirNameFinder can get one.
PersonDirNameFinderFactory is the IEntityNameFinderFactory implementation which produces PersonDirNameFinder instances.
/**
* Factory for creating <code>PersonDirNameFinders</code>.
* @author Alex Vigdor
* @version $Revision: 1.5 $
*/
public class PersonDirNameFinderFactory implements IEntityNameFinderFactory {
private static final Log log = LogFactory.getLog(PersonDirNameFinderFactory.class);
/**
* ReferencePersonNameFinderFactory constructor comment.
*/
public PersonDirNameFinderFactory() {
super();
}
/**
* Return a finder instance.
* @return org.jasig.portal.groups.IEntityNameFinder
* @exception org.jasig.portal.groups.GroupsException
*/
public IEntityNameFinder newFinder() throws GroupsException
{
try
{ return PersonDirNameFinder.singleton(); }
catch ( SQLException sqle )
{
log.error( "ReferencePersonNameFinderFactory.newFinder(): " + sqle);
throw new GroupsException(sqle);
}
}
}
Its "newFinder()" method doesn't really create a new instance of PersonDirNameFinder – it retrieves the static instance of the class.
A bug
Dan Ellentuck noticed the following bug: IPersonAttributeDao returns null in the case of an unknown user, and the PersonDirNameFinder implementation above doesn't check for the null return value from IPersonAttributeDao.
And a fix for the bug
Here's the changed primGetName() implementation to fix this bug:
private String primGetName (String key) throws java.sql.SQLException {
String name = key;
Map userInfo = pa.getUserAttributes(name);
if (userInfo != null)
{
Object displayName = userInfo.get("displayName");
String displayNameStr = "";
if (displayName != null)
{
if (displayName instanceof java.util.List)
{
List displayNameList = (List) displayName;
if (! displayNameList.isEmpty() )
{ displayNameStr = (String)displayNameList.get(0); }
}
else displayNameStr = (String)displayName;
if (! displayNameStr.trim().equals(""))
{ name = displayNameStr; }
}
}
return name;
}
Refactoring
Payoff: a testcase
Conclusion
Chagrined at having introduced this NullPointerException-yielding bug, I got interested in producing a JUnit testcase to test this code. With this testcase in place, the next time I touch this code I won't as easily introduce this particular bug.
I'd like to commit the refactored PersonDirNameFinder, PersonDirNameFinderFactory, and associated testcase.