Should multiple service layer objects share a DAO?

Question

I have a Contact class that contains a PortalAccount object. When I want to create a "Portal Account" for a contact, an account is created remotely on a portal application using soap/axis and then the contact's portalAccount is populated and the contact is saved (local database holds information about the remote account, like user id and username, etc).

So I have a service class PortalServiceImpl that has methods to actually create a user on a remote portal, given a Contact instance.

Given all of this information, my question then is: should the PortalServiceImpl get an instance of a ContactDAO object and actually do the saving, or should the PortalServiceImpl class just create the remote user, modify the passed in Contact object, and let the client be responsible for saving?

Method 1:

class ServiceFacadeImpl {
  public void createPortalAccount(Contact contact) {
    // here the contact is implicitly saved
    this.portalService.createPortalAccount(contact);
  }
}

Method 2:

class ServiceFacadeImpl {
  public void createPortalAccount(Contact contact) {
    // here contact is implicitly modified
    this.portalService.createPortalAccount(contact);
    this.contactDAO.save(contact);
  }
}

Both methods feel wrong to me. Method 1 feels wrong because the PortalService is creating a remote user AND saving the contact to the database (albeit through a DAO interface). Method 2 feels wrong because I have to assume that the PortalService is modifying the Contact I'm passing to it.

I also have a feeling that I'm not seeing some other gotchas, like potentially not handling transactions consistently.

(BTW, I've already used both methods, and don't want to continue refactoring in an endless circle. Something just seems wrong here.)

Answer 1

Are you sure it's a good idea that you have different contact IDs locally and remotely? It seems wrong to me, but maybe I just don't know your domain.

In my application all new contacts are sent through the webservice to remote portal and saved there. So, when I save new contact locally, it is sent to a remote portal and saved there. Maybe you need the same?

If the above thoughts are unacceptable for you, then I would do it like this:

class ServiceFacadeImpl {
  public void CreatePortalAccountAndSaveContact(Contact contact) {
    try
    {
      contact.portalAccount = this.portalService.createPortalAccount(contact);
      this.contactDAO.save(contact);
    }
    catch(...)
    {
      // do cleanup, for example do you need to delete account from remote 
      // portal if it couldn't be saved locally?
      // If yes, delete it from portal and set contact.portalAccount = null;
    }
  }
}

Some may say, that CreatePortalAccountAndSaveContact break single responsibility principle but imo in this situation it's absolutely normal because, as I understand, you need this operation to be atomic. Right?

Or you can add boolean flag to the method, indicating if you want to save contact. But if you always need to save contact with PortalAccount straight after getting it from remote portal - then boolean flag is not needed.

PS. Why do you use "this" keyword? Is portalService private member? If yes, then maybe you need to reconsider your naming convention and name private members with prefix "_" for example (I think it's the most popular one), like _portalService - then it will be easy to understand that _portalService is a private member. Sorry for offtopic.

Good luck.