I'm starting to get some rather stinky code in one of my projects and wanted to
ask for some advice. (
Its a Struts2/Spring2/JPA(Hibernate) based project. I'm using a slightly
modified version of the Generic DAO pattern shown in the Java persistence with
Hibernate book and/or the IBM ThoughtWorks very similar example. (Modified to
allow Spring Based Injection of the JPA EntityManger, while falling back to a
native Hibernate session inside the DAOs to allow a few more optimizations).
So its basically
Business Objects (POJOs) <---1:1---> DAOs
which is a relatively normal pattern I beleive.
Now I'm working on an Action that creates an object that contains lots of
references to other objects. A bug tracker would be a reasonable facsimile of
my domain object -- when creating a bug, you select a severity, a project, a
module, a found in version, possibly you're assigning it to someone, etc. Most
of these options come from a drop-down on the view page, however they are all
backed by their own domain object -- ie its not just id/string display name
lookup-table backing the drop down.
So the form submits a whole suite of IDs or String natural keys and I need to
build an Defect/Bug and then persist it. In building the Defect I need to
convert all the keys to domain objects. This is where things begin to fall
apart. To covert the keys to domain object seems to require injecting all the
subordinate object DAOs into the Action, its not a big deal but its starting to
feel like the subordinate DAOs are taking over the action. Furthermore, its
slightly annoying from a performance stand-point that I need to retrieve all
the objects from the DB, just to set a foreign key -- I will never be updating
the details on the subordinate object from the master only changing which one
I'm linked to. [As a side issue: this is a place where the Get (returning a
proxy/lazy load thunk with only the ID set without hitting the DB) versus Load
could be useful, but I've never seen any of the generic DAO approaches expose
that level of control in their API -- does anyone know why?]
Here's an example (not from live code, please ignore any minor typos)
representing the current state of application and test code. Afterwards I'll
list a few of the approaches I've considered for cleaning it up:
AddBug.java
code:
public class AddBug extends ActionSupport {
private BugDAO bugService;
private UserDAO userService;
private ProjectDAO projectService;
private ModuleDAO moduleService;
private VersionDAO versionService;
private String submitterUsername;
private String assigneeUsername;
private Long projectID;
private Long moduleID;
private List<Long> affectedVersionIDs;
public String execute() {
Bug bug = new Bug();
// The following lines are what feel unclean to me
bug.setSubmitter(userService.findByUsername(getSubmitterUsername()));
bug.setAssignee(userService.findByUsername(getAssigneeUsername()));
bug.setProject(projectService.find(getProjectID()));
bug.setModule(moduleService.find(getModuleID()));
for( Long versionID : getAffectedVersionIDs()) {
bug.addAffectedVersion(versionService.find(versionID));
}
bugDAO.persist(bug);
return SUCCESS;
}
// This block of setters also seems to be wrong... Only the first one
// should really be required by this action I think....
public void setBugService(final BugDAO bs) {bugService=bs;}
public void setUserService(final UserDAO us) {userService=us;}
public void setProjectService(final ProjectDAO ps) {projectService=ps;}
public void setModuleService(final ModuleDAO ms) {moduleService=ms;}
public void setVersionService(final VersionDAO vs) {versionService=vs;}
public void setSubmitterUsername(final String username)
{submitterUsername=username;}
public void setAssigneeUsername(final String username)
{assigneeUsername=username;}
public void setProjectID(final Long id) {projectID=id;}
public void setModuleID(final Long id) {moduleID=id;}
public void setAffectedVersionIDs(final List<Long> ids)
{affectedVersionIDs=ids;}
public String getSubmitterUsername() {return submitterUsername;}
public String getAssigneeUsername() {return assigneeUsername;}
public Long getProjectID() {return projectID;}
public Long getModuleID() {return moduleID;}
public List<Long> getAffectedVersionIDs() {return affectedVersionIDs;}
}
TestAddBug.java
(Sorry this is purely from memory so I know I'm missing some of my
infrastructure)
code:
....
public void testAddBug() {
String submitterUsername="TestUser";
String assigneeUsername="TestVictim";
User submitter=new User(submitterUsername);
User assignee=new User(assigneeUsername);
UserDAO userService = createMock(UserDAO.class);
expect(userService.findByUsername(submitterUsername)).andReturn(submitter);
expect(userService.findByUsername(assigneeUsername)).andReturn(assignee);
Long projectID =1L;
Project project = new Project(projectID,"Some Projectname");
ProjectDAO projectService = createMock(ProjectDAO.class);
expect(projectService.find(projectID)).andReturn(project);
Long moduleID=1L;
Module module = new Module(moduleID,"Some ModuleName");
ModuleDAO moduleService = createMock(ModuleDAO.class);
expect(moduleService.find(moduleID)).andReturn(module);
Long versionOneID=1L;
Long versionTwoID=2L;
Version versionOne = new Version(versionOneID,"A Version");
Version versionTwo = new Version(vertionTwoID,"Another Version");
List<Long> versionIDs = new ArrayList<Long>();
List<Version> versions = new ArrayList<Version>();
versionIDs.add(versionOneID);
versionIDs.add(versionTwoID);
versions.add(versionOne);
versions.add(versionTwo);
VersionDAO = versionService = createMock(VersionDAO.class);
expect(versionService.buildVersionlist(versionIDs)).andReturn(versions);
Bug bug = new Bug();
BugDAO bugService = createMock(BugDAO.class);
expect(bugService.persist(bug));
replay(userSerive);
replay(projectService);
replay(moduleService);
replay(versionService);
replay(bugService);
AddBug action = new AddBugAction();
action.setBugService(bugService);
action.setProjectService(projectService);
action.setModuleService(moduleService);
action.setVersionService(versionService);
action.setUserService(userService);
// In reality this would probably be pulled from the session map, but...
action.setSubmitterUsername(submitterUsername);
action.setAssigneeUsername(assigneeUsername);
action.setProjectID(projectID);
action.setModuleID(moduleID);
action.setAffectedVersionIDs(versionIDs);
assertEquals("success",action.execute());
verify(bugService);
verify(userService);
verify(projectService);
verify(moduleService);
verify(versionService);
}
...
OK. Now that you've seen the mess... I've thought of two ways to clean it up:
1) Introduce a business tier between the domain objects and the DAOs. Expose a
buildBug call in this layer that takes in the keys and handles the promotion to
subordinate domain objects. This business tier would need to have all the DAO
injected into it. Actions would still have their most relevant DAO(s) injects,
plus the business tier relevant objects. Testing the business tier would still
require the mess of mocks, but at least the action only needs to mock the
single call on the business tier. However I don't think this approach would
play too well with migrating to a Model-Driven strategy in the future as the
domain objects can't accept the bare keys. I'm also having a hard time figuring
out what type of methods would end up in this layer -- it seems like mostly it
will be these kinds of initial builders... Most of other use cases are either
correctly cascaded/handled by Hibernate (updates/deletes) and the DAOs or they
belong on the POJOs (real business logic)... Leading to a very anemic layer,
which doesn't seem worth the added complexity...
2) Setup a series of type converters to promote the keys to objects on the
incoming HTTP request end of things. The type converters would need access to
the DAOs. Testing them should be rather simple as each singular one is simple.
Testing the action no longer requires mocking the subordinate objects and I can
just use POJOs there. I believe this approach would work with the Model-Driven
idea, but it feels a little odd to me to mark up the domain object with
HTTP-specific details.... then again the domain object is already annotated
with Hibernate/JPA annotations so its not like its really 'pure'.. I've seen
some talk on the mailing list of this approach, but it seems to have some
"black holes" regarding converting back to string for redsplay at times.
Comments, alternate approaches?
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
