ProductManager: a basic CRUD for products with SQLite

I would like to have a review of this basic CRUD application so I can improve and learn from your experience. The code can be found here.

• Design pattern/coding principles improvement that could be used

• Exception handling

• Readability

• Coding best practices

ProductManager.java

```
public class ProductManager {

private IProductDatabase db;

public ProductManager(String Dburl) throws ServiceException {
try {
db = new SQLiteProductDatabase(Dburl);
} catch (DatabaseException ex) {
throw new ServiceException(ex);
}
}

public int getNumberOfProducts() throws ServiceException{
try {
return db.size();
} catch (DatabaseException ex) {
throw new ServiceException(ex);
}
}

public void addProduct(Product product) throws ServiceException {
try {
db.addProduct(product);
} catch (DatabaseException ex) {
throw new ServiceException(ex);
}
}

public Product getProductByEan(Long ean) throws ServiceException {
try {
return db.getProductByEan(ean);
} catch (DatabaseException ex) {
throw new ServiceException(ex);
}
}

public Product getProductByHope(int hope) throws ServiceException {
try {
return db.getProductByHope(hope);
} catch (DatabaseException ex) {
throw new ServiceException(ex);
}
}

public Collection getAllProducts() throws ServiceException {
try {
return db.getAllProducts();
} catch (DatabaseException ex) {
throw new ServiceException(ex);
}
}

public void updateProduct(Product product) throws ServiceException {
try {
db.updateProduct(product);
} catch (DatabaseException ex) {
throw new ServiceException(ex);
}
}

public void deleteProduct(Long ean) throws ServiceException

I'd like to add a few points to your proposed implementation.

Inversion of control

The first thing I'd change is the constructor of your ProductManager class. I'd pass in an IProductDatabase rather than the connection string. This will make the code more testable as you'd be able to pass in test doubles at 'test time'.

public class ProductManager {

      private IProductDatabase m_db;

      public ProductManager(IProductDatabase db) {       
            m_db =db       
        }
      ...
     }

Exception Handling

The other point is regarding exception handling. Usually you want to catch exceptions if you intend to handle them in any way. I'm not sure how much value you get if you just wrap the original exception into another exception. Catching exceptions is somewhat expensive

Validation

I'd also add validation on all the method calls. For example if a null object would passed to the constructor of the ProductManager class, an imediat IllegalARgumentException would be much useful than an eventuall NullPointerException.

public class ProductManager {

      private IProductDatabase m_db;

      public ProductManager(IProductDatabase db) {       
            if(db==null)
            {
              throw IllegalArgumentException("db");
             }
            m_db =db       
        }
      ...
     }

I'd also try to use relevant exceptions. For example instead of :

@Override
public void addProduct(Product product) throws DatabaseException {
    if (product == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_NULL);
    }
  ...
 }

I would use

@Override
public void addProduct(Product product) throws DatabaseException {
    if (product == null) {
        throw new IllegalArgumentException("product");
    }
  ...
 }

Instead of :

@Override
public Product getProductByHope(int hope) throws DatabaseException {
...
  if (product == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_NOT_FOUND_HOPE);
    }
}

I'd use

@Override
public Product getProductByHope(int hope) throws ProductNotFoundException {
    ...
      if (product == null) {
            throw new ProductNotFoundException(ErrorMessages.PRODUCT_NOT_FOUND_HOPE);
        }
    }

I hope this helps

StackExchange Code Review Q#103821, answer score: 2