Skip to main content
address comments, reorganise a bit
Source Link
JvR
  • 2.8k
  • 12
  • 25

The setCurrentUsedStorage is what kind of storage I'm using. So RAM, SQL or Serial implementation. How do you suggest to deal with this?

Ideally, remove the field. If you have a clear and compelling use case that can not be reasonably solved otherwise, keep the field for now, but clearly document the why and the how (and use the HACK pseudo-tag), and consider adding it to your project's to-do list. This really feels like a bug waiting to happen.

It should not be a factory's duty to keep track of what type you are currently interested in. Either the container (configuration) or the client code should know what it wants/needs (and pass it into the factory method): factories are middle men.

#General Remarks#

The currentUsedStorage field in DAOFactory looks out of place. What is its purpose? Consider deleting it. In that same vein, setCurrentUsedStorage is not declared static.#Documentation#

Auto-generated getter and setter comments are not useful as documentation: they simply repeat the name of the function. Remove these to reclaim some vertical space.

Document what things like active mean in your method parameters. Does it make sense ever not to pass a boolean in there?

#General Remarks#

The currentUsedStorage field in DAOFactory looks out of place. What is its purpose? Consider deleting it. In that same vein, setCurrentUsedStorage is not declared static.

Auto-generated getter and setter comments are not useful as documentation: they simply repeat the name of the function. Remove these to reclaim some vertical space.

The setCurrentUsedStorage is what kind of storage I'm using. So RAM, SQL or Serial implementation. How do you suggest to deal with this?

Ideally, remove the field. If you have a clear and compelling use case that can not be reasonably solved otherwise, keep the field for now, but clearly document the why and the how (and use the HACK pseudo-tag), and consider adding it to your project's to-do list. This really feels like a bug waiting to happen.

It should not be a factory's duty to keep track of what type you are currently interested in. Either the container (configuration) or the client code should know what it wants/needs (and pass it into the factory method): factories are middle men.

#Documentation#

Auto-generated getter and setter comments are not useful as documentation: they simply repeat the name of the function. Remove these to reclaim some vertical space.

Document what things like active mean in your method parameters. Does it make sense ever not to pass a boolean in there?

Source Link
JvR
  • 2.8k
  • 12
  • 25

I might be missing the point here of the factory and implementing incorrectly?

Factories are an abstraction for how to obtain instances, as distinct from explicitly pooling, singletonning, or creating instances (through constructors) from client code. Exactly how far into this abstraction you want to go is a matter of debate and, in some cases, ridicule, but this base abstraction is what you need to keep in mind when designing them.

Because factories are a means to an end, it's hard to tell whether your factory will be helpful without seeing your use case. What will your use of the pattern help you accomplish? (As a hunch, I'd recommend you to look into dependency injection.)

Is a singleton necessary with my implementation within the factory and more specifically with each DAO?

No. A singleton is never really necessary anywhere(1) and it's usually a convenience hack to prevent having to pass a reference into a method or store it in context; a bypass of sorts. That's not to say that singletons serve no useful purpose, but that the decision to use a singleton should be an implementation choice, and never a design choice.

(1) It can even be downright hazardous in server environments with managed containers.


That's for the abstract part; let's get to concrete suggestions.

#Pull singletons up to the factory level#

The concern of the DAO implementations is data storage and retrieval. The concern of the factory is how to supply instances. Lifting the singletons from the individual implementations leaves the factory in control of instantiation and life cycle.

Here's a straightforward example of such an approach:

// I like that boilerplate.  That's some nice boilerplate.
public class SimpleStoringFactory extends DAOFactory {
  private final GenericDAO<Trade, String, Boolean> tradeDAO;
  private final GenericDAO<User, String, Boolean> userDAO;
  private final GenericDAO<User, String, Boolean> logDAO;
  private final GenericDAO<ServiceRequest, String, Boolean> requestDAO;
  
  public SimpleStoringFactory(
      GenericDAO<Trade, String, Boolean> tradeDAO,
      GenericDAO<User, String, Boolean> userDAO,
      GenericDAO<User, String, Boolean> logDAO,
      GenericDAO<ServiceRequest, String, Boolean> requestDAO) {
    this.tradeDAO = tradeDAO;
    this.userDAO = userDAO;
    this.logDAO = logDAO;
    this.requestDAO = requestDAO;
  }
  
  public GenericDAO<Trade, String, Boolean> getTradeDAO() { return tradeDAO; }
  public GenericDAO<User, String, Boolean> getUserDAO() { return userDAO; }
  public GenericDAO<User, String, Boolean> getLogDAO() { return logDAO; }
  public GenericDAO<ServiceRequest, String, Boolean> getRequestDAO() { return requestDAO; }
}

#GenericDAO#

public boolean update(U user, B active) throws NullUserException,
        UserExistException, NullListException, UserInstanceException,
        ReceivedNullParameterException;

That's... quite something. Only list exceptions that you expect clients to handle/catch, and see whether built-in, available exception types like IllegalArgumentException suffice.

If this is not feasible, document when exceptions are expected to be thrown, and when the return value is true or false. I could not make this out from the method signature. Compare with:

/**
 * Finds the record identified by <var>user</var> and updates it to the given values.
 * Returns true if changes were made to the record, and false if the record was found,
 * but all current values matched given values (i.e. no update occurred).
 *
 * @return {@code true} iff changes were made to the record.
 * @throws NotFoundException if no record for this user was found.
 */
public boolean update(U user, B active) throws NotFoundException;

#General Remarks#

The currentUsedStorage field in DAOFactory looks out of place. What is its purpose? Consider deleting it. In that same vein, setCurrentUsedStorage is not declared static.

Auto-generated getter and setter comments are not useful as documentation: they simply repeat the name of the function. Remove these to reclaim some vertical space.