0
\$\begingroup\$

I'm learning about SRP and had to reduce the responsibility of a customer object, which contained too much info. So I turned that into just a Pojo and pulled out the database logic. I'm trying to find the best design for this setup and am split between making the DatabaseManager class be a parent class, or a singleton that can return the single connection object. This system will be used to insert and delete customers from the database. I don't want my domain objects/DAOs to worry about the connection details. Is the design I currently have a strong follower of OOP design principles?

public class DatabaseManager {
 private Connection conn;
 private static DatabaseManager managerInstance = new DatabaseManager();
 private DatabaseManager() {
 }
 public static DatabaseManager getInstance() {
 return managerInstance;
 }
 /**
 * contains connection details
 * 
 * @throws SQLException
 */
 public void connect() throws SQLException {
 System.out.println("Established Database Connection...");
 conn = DriverManager.getConnection("Some/Database/URL");
 }
 public Connection getConnectionObject() {
 return conn;
 }
 public void disconnect() throws SQLException {
 conn.close();
 System.out.println("Disconnected from Database...");
 }
}

Here is the Customer Object:

public class Customer {
 private int id;
 private String name;
 private boolean active;
 public Customer(int id, String name, String department, boolean working) {
 super();
 this.id = id;
 this.name = name;
 this.department = department;
 this.working = working;
 }
 @Override
 public String toString() {
 return "Customer [id=" + id + ", name=" + name + ", department="
 + department + ", working=" + working + "]";
 }
}

The customer DAO:

public class CustomerDAO {
 public CustomerDAO() {
 }
 public void addCustomer(Customer Customer) throws SQLException {
 DatabaseManager.getInstance().getConnectionObject().prepareStatement("some sql... ");
 }
 public void removeCustomer(Customer Customer) throws SQLException {
 DatabaseManager.getInstance().getConnectionObject().prepareStatement("some sql... ");
 // implementation details avoided
 }
}
Phrancis
20.5k6 gold badges69 silver badges155 bronze badges
asked Aug 22, 2014 at 1:25
\$\endgroup\$
4
  • 1
    \$\begingroup\$ For more info: programmers.stackexchange.com/search?q=dao+design (that's the Programmers.SE I'm talking about) \$\endgroup\$ Commented Aug 22, 2014 at 1:59
  • \$\begingroup\$ Sorry if I'm coming off as too curt, but comments like // implementation details avoided is a sign that your question doesn't really fit here, just like your previous question... \$\endgroup\$ Commented Aug 22, 2014 at 2:00
  • 1
    \$\begingroup\$ It would be best to include implementation details. \$\endgroup\$ Commented Aug 22, 2014 at 2:21
  • \$\begingroup\$ That just removes the customer from the database. No need to get into those details. Im talking about oop design principles. Thats it. Does this design overall follow, OCP, DRP, ISP, and SRP? \$\endgroup\$ Commented Aug 22, 2014 at 3:06

1 Answer 1

2
\$\begingroup\$

Single responsibility principle

In terms of SRP, the current code is good, each class is has one clear purpose:

  • Customer: only the customer details, nothing else
  • CustomerDAO: only data accessor methods related to customers
  • DatabaseManager: manage database connection, DatabaseConnectionManager might be a better name

But they can be improved.

Customer

If you can make its fields final, make them final.

No need to call super() in a class whose parent is Object. You can just delete that line.

CustomerDAO

The throws SQLException declarations make this class database specific. In theory, a CustomerDAO could work over a web service or a CSV file. Actually CustomerDAO should be an interface, to model a backend-agnostic concept, with throws declarations that are backend-agnostic. Then an implementation using a database backend should wrap the SQLExceptions and throw the backend-agnostic exceptions instead. In such design you will have the flexibility to replace the DAO with a different backend.

Also, the public empty constructor is pointless.

DatabaseManager

The modern pattern of implementing singletons is this:

public enum DatabaseManager {
 INSTANCE;
 // ...
}

This is truly thread-safe. Use this instead of a private static field, which is not truly thread-safe.

Use a framework

Instead of managing database connections yourself, I strongly recommend using a framework that can greatly simply it for you. For example Spring Framework.

answered Aug 22, 2014 at 6:20
\$\endgroup\$
4
  • \$\begingroup\$ Thank you for the greatt explanation! Im learning oop concepts as well as common design patterns hence i wanted to build this thing myself rather than depend on a framework. But do u think it would be considered over engineering if i created an interface for each one of my domain object daos? Can i define a single dao that is database specific and have my domain daos implement that? What would you recommend? \$\endgroup\$ Commented Aug 22, 2014 at 11:13
  • \$\begingroup\$ I don't think it will be overengineering, it will be a good thing to do to create an interface for each domain dao. To avoid repetition in your database operations, you can either have a common parent class, or a utility class. \$\endgroup\$ Commented Aug 22, 2014 at 12:46
  • \$\begingroup\$ Hmmm, how would I make this enum work for my code? \$\endgroup\$ Commented Aug 22, 2014 at 12:47
  • \$\begingroup\$ Replace all occurrences of DatabaseManager.getInstance() with DatabaseManager.INSTANCE \$\endgroup\$ Commented Aug 22, 2014 at 13:25

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.