4
\$\begingroup\$

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.

  1. Design pattern/coding principles improvement that could be used
  2. Exception handling
  3. Readability
  4. 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<Product> 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 {
        try {
            db.deleteProduct(ean);
        } catch (DatabaseException ex) {
            throw new ServiceException(ex);
        }
    }
}

IDatabase.java

public interface IProductDatabase {

    public int size() throws DatabaseException;

    public void addProduct(Product product) throws DatabaseException;

    public Product getProductByEan(Long ean) throws DatabaseException;

    public Product getProductByHope(int hope) throws DatabaseException;

    public Collection<Product> getAllProducts() throws DatabaseException;

    public void updateProduct(Product product) throws DatabaseException;

    public void deleteProduct(Long ean) throws DatabaseException;

}

SQLiteProductDatabase.java

public class SQLiteProductDatabase implements IProductDatabase {

private Connection connection;
private PreparedStatement statement;
String url;

public SQLiteProductDatabase(String url) throws DatabaseException {
    try {
        Class.forName("org.sqlite.JDBC");
        this.url = url;
    } catch (ClassNotFoundException e) {
        throw new DatabaseException(ErrorMessages.DATABASE_DRIVER_NOT_LOADED, e);
    }
}

@Override
public int size() throws DatabaseException {
    String query = "SELECT COUNT(ean) AS size FROM product";
    int size = 0;
    initiateStatement(query);
    try {
        ResultSet r = statement.executeQuery();
        r.next();
        size = r.getInt("size");
    } catch (SQLException ex) {
        throw new DatabaseException(ErrorMessages.DATABASE_FAULT_IN_QUERY, ex);
    } finally {
        closeConnection();
    }
    return size;
}

@Override
public void addProduct(Product product) throws DatabaseException {
    if (product == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_NULL);
    }
    String query = "INSERT INTO product (ean, hope, name) VALUES (?, ?, ?)";
    initiateStatement(query);
    try {
        statement.setLong(1, product.getEan());
        statement.setInt(2, product.getHope());
        statement.setString(3, product.getName());
        statement.execute();
    } catch (SQLException e) {
        throw new DatabaseException(ErrorMessages.PRODUCT_ALREADY_EXISTS, e);
    } finally {
        closeConnection();
    }
}

@Override
public Product getProductByEan(Long ean) throws DatabaseException {
    if (ean == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_EAN_NULL);
    }
    String query = "SELECT * FROM product WHERE ean = ?";
    Product product = null;
    initiateStatement(query);
    try {
        statement.setLong(1, ean);
        ResultSet result = statement.executeQuery();
        while (result.next()) {
            int hope = result.getInt("hope");
            String name = result.getString("name");
            product = new Product(ean, hope, name);
        }
    } catch (SQLException e) {
        throw new DatabaseException(ErrorMessages.DATABASE_FAULT_IN_QUERY, e);
    } catch (DomainException ex) {
        throw new DatabaseException(ex);
    } finally {
        closeConnection();
    }
    if (product == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_NOT_FOUND_EAN);
    }
    return product;
}

@Override
public Product getProductByHope(int hope) throws DatabaseException {
    String query = "SELECT * FROM product WHERE hope = ?";
    Product product = null;
    initiateStatement(query);
    try {
        statement.setInt(1, hope);
        ResultSet result = statement.executeQuery();
        while (result.next()) {
            Long ean = result.getLong("ean");
            String name = result.getString("name");
            product = new Product(ean, hope, name);
        }
    } catch (SQLException e) {
        throw new DatabaseException(ErrorMessages.DATABASE_FAULT_IN_QUERY, e);
    } catch (DomainException ex) {
        throw new DatabaseException(ex);
    } finally {
        closeConnection();
    }
    if (product == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_NOT_FOUND_HOPE);
    }
    return product;
}

@Override
public Collection<Product> getAllProducts() throws DatabaseException {
    String query = "SELECT * FROM product";
    Collection<Product> products = new ArrayList<>();
    initiateStatement(query);
    try {
        ResultSet result = statement.executeQuery();
        while (result.next()) {
            Long ean = result.getLong("ean");
            int hope = result.getInt("hope");
            String name = result.getString("name");
            Product product = new Product(ean, hope, name);
            products.add(product);
        }
    } catch (SQLException e) {
        throw new DatabaseException(ErrorMessages.DATABASE_FAULT_IN_QUERY, e);
    } catch (DomainException ex) {
        throw new DatabaseException(ex);
    } finally {
        closeConnection();
    }
    return products;
}

@Override
public void updateProduct(Product product) throws DatabaseException {
    if (product == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_NULL);
    }
    String query = "UPDATE product SET hope = ?, name = ? WHERE ean = ?";
    initiateStatement(query);
    try {
        statement.setInt(1, product.getHope());
        statement.setString(2, product.getName());
        statement.setLong(3, product.getEan());
        statement.execute();
    } catch (SQLException e) {
        throw new DatabaseException(ErrorMessages.DATABASE_FAULT_IN_QUERY, e);
    } finally {
        closeConnection();
    }
}

@Override
public void deleteProduct(Long ean) throws DatabaseException {
    if (ean == null) {
        throw new DatabaseException(ErrorMessages.PRODUCT_EAN_NULL);
    }
    String query = "DELETE FROM product WHERE ean = ?";
    initiateStatement(query);
    try {
        statement.setLong(1, ean);
        statement.execute();
    } catch (SQLException e) {
        throw new DatabaseException(ErrorMessages.DATABASE_FAULT_IN_QUERY, e);
    } finally {
        closeConnection();
    }
}

private void initiateStatement(String query) throws DatabaseException {
    try {
        connection = DriverManager.getConnection(url);
        statement = connection.prepareStatement(query);
    } catch (SQLException ex) {
        throw new DatabaseException(ErrorMessages.DATABASE_NOT_FOUND, ex);
    }
}

private void closeConnection() throws DatabaseException {
    try {
        statement.close();
        connection.close();
    } catch (SQLException e) {
        throw new DatabaseException(ErrorMessages.DATABASE_CLOSSING_CONNECTION, e);
    }
}
}

Product.java

public class Product {

    /**
     * The minimum amount of digits the ean of a product must have.
     */
    public static final int MIN_AMOUNT_OF_CIPHERS_EAN = 8;

    /**
     * The maximum amount of digits the ean of a product must have.
     */
    public static final int MAX_AMOUNT_OF_CIPHERS_EAN = 13;

    /**
     * The minimum amount of digits the hope of a product must have.
     */
    public static final int MIN_AMOUNT_OF_CIPHERS_HOPE = 4;

    /**
     * The maximum amount of digits the hope of a product must have.
     */
    public static final int MAX_AMOUNT_OF_CIPHERS_HOPE = 8;

    /**
     * The minimum amount of characters the name of a product must have.
     */
    public static final int MIN_AMOUNT_OF_CHARACTERS_NAME = 2;

    /**
     * The maximum amount of characters the name of a product may have.
     */
    public static final int MAX_AMOUNT_OF_CHARACTERS_NAME = 30;

    /**
     * The EAN of the product.
     */
    private Long ean;

    /**
     * The hope of the product.
     */
    private int hope;

    /**
     * The name of the product.
     */
    private String name;

    public Product() {
    }

    public Product(Long ean, int hope, String name) throws DomainException {
        setEan(ean);
        setHope(hope);
        setName(name);
    }

    public Long getEan() {
        return ean;
    }

    public void setEan(Long ean) throws DomainException {
        if (ean == null) {
            throw new DomainException(ErrorMessages.PRODUCT_EAN_NULL);
        }
        if (ean < 0) {
            throw new DomainException(ErrorMessages.PRODUCT_EAN_NEGATIVE);
        }
        if (String.valueOf(ean).length() < MIN_AMOUNT_OF_CIPHERS_EAN) {
            throw new DomainException(ErrorMessages.PRODUCT_EAN_MIN_LENGTH);
        }
        if (String.valueOf(ean).length() > MAX_AMOUNT_OF_CIPHERS_EAN) {
            throw new DomainException(ErrorMessages.PRODUCT_EAN_MAX_LENGTH);
        }
        this.ean = ean;
    }

    public int getHope() {
        return hope;
    }

    public void setHope(int hope) throws DomainException {
        if (hope < 0) {
            throw new DomainException(ErrorMessages.PRODUCT_HOPE_NEGATIVE);
        }
        if (String.valueOf(hope).length() < MIN_AMOUNT_OF_CIPHERS_HOPE) {
            throw new DomainException(ErrorMessages.PRODUCT_HOPE_MIN_LENGTH);
        }
        if (String.valueOf(hope).length() > MAX_AMOUNT_OF_CIPHERS_HOPE) {
            throw new DomainException(ErrorMessages.PRODUCT_HOPE_MAX_LENGTH);
        }
        this.hope = hope;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) throws DomainException {
        name = name.replaceAll(System.getProperty("line.separator"), "");
        name = name.replaceAll("\r|\n", "");
        name = name.toLowerCase().trim();
        if (!name.matches("^[.,a-zA-Z0-9 ]+$")) {
            throw new DomainException(ErrorMessages.PRODUCT_NAME_ALPHANUMERIC);
        }
        if (name.length() < MIN_AMOUNT_OF_CHARACTERS_NAME) {
            throw new DomainException(ErrorMessages.PRODUCT_NAME_MIN_LENGTH);
        }
        if (name.length() > MAX_AMOUNT_OF_CHARACTERS_NAME) {
            throw new DomainException(ErrorMessages.PRODUCT_NAME_MAX_LENGTH);
        }
        this.name = name;
    }

    @Override
    public int hashCode() {
        int hash = 5;
        hash = 83 * hash + Objects.hashCode(this.ean);
        hash = 83 * hash + this.hope;
        hash = 83 * hash + Objects.hashCode(this.name);
        return hash;
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == null) {
            return false;
        }
        if (getClass() != obj.getClass()) {
            return false;
        }
        final Product other = (Product) obj;
        if (!Objects.equals(this.ean, other.ean)) {
            return false;
        }
        if (this.hope != other.hope) {
            return false;
        }
        if (!Objects.equals(this.name, other.name)) {
            return false;
        }
        return true;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I'm sure our Java experts will have some good advice for you. Hope you get some great answers! \$\endgroup\$ – Phrancis Sep 4 '15 at 20:43
  • 1
    \$\begingroup\$ This is a minor remark, but there is not a need to document each and every variable in Product class. The naming should speak for itself. I do think using DIGITS instead of CIPHERS would make it easier to understand, e.g.: MIN_AMOUNT_OF_DIGITS_EAN \$\endgroup\$ – Phrancis Sep 4 '15 at 20:46
  • \$\begingroup\$ Thank you! That's very nice :) And I'll make use of your naming tip! \$\endgroup\$ – Mithos-Yggdrasill Sep 4 '15 at 20:51
  • \$\begingroup\$ Hi! I rolled back your edit that added the revised code because doing so is discouraged on Code Review and does not cooperate with the Q&A format. See that Meta post for other ways you can accomplish that. \$\endgroup\$ – Alexis King Sep 5 '15 at 21:33
2
\$\begingroup\$

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

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you. That's really helpful! Especially the exception-handling part. \$\endgroup\$ – Mithos-Yggdrasill Sep 4 '15 at 21:10
  • \$\begingroup\$ Inversion of Control: I pass a string so I can later add Simple Factory to support another db-implementation. Then the String url can change to a dbType. \$\endgroup\$ – Mithos-Yggdrasill Sep 4 '15 at 21:13
  • \$\begingroup\$ I still believe that passing in dependencies in your constructor as abstractions is a better approach. You get at least a few benefits for this approach. The code is cleaner, you keep things simpler, you can test the implementation more easily, and you isolate the creation of the IProductDatabase . How the database is created is no concern of the product manager. You can use factories, or an IOC container or even create it yourself. All it needs to know is that it requires an IProductDatabase \$\endgroup\$ – Andrei Sep 4 '15 at 21:23
  • \$\begingroup\$ You convinced me, thanks for teaching me! :) \$\endgroup\$ – Mithos-Yggdrasill Sep 4 '15 at 21:24
  • 1
    \$\begingroup\$ Glad to be of help :). One other thing, in the world of Java, it is custom not to use I as the prefix for interfaces. For example in the standard library you will find interfaces like List, Collection, Stream .. etc. The I prefix is sort of standard for .NET. There you will find things like IList, ICollection, IEnumerable .. etc \$\endgroup\$ – Andrei Sep 4 '15 at 21:30
2
\$\begingroup\$

Naming

  1. In Product class, the constant names are too verbose. They could have been named just MIN_LENGTH_EAN, MAX_LENGTH_EAN etc. Or EAN_MIN_LENGTH, EAN_LENGTH_MIN and the like. And they don't need javadoc, because their meaning should be clear from the names.

  2. That may be a question of taste, but the 'I' prefix in interface names (IProductDatabase) is useless and noisy. Generally, there is no matter whether the reference that is used in a routine corresponds to an interface or a class.

JDBC

Since JDBC 4, you do not need to call Class.forName(arg) in order to load the driver class. Just use DriverManager.getConnection(args) directly.

equals(arg)

In Product.equals(Object), the call

if (getClass() != obj.getClass()) {
    return false;
} 

seems to me quite careless. It breaks the general principle of equals() method implementation. What if an object extending Product calls super.equals() while overriding equals()? The descendant will never receive true. This condition should be

if (!(obj instanceof Product)) {
    return false;
}

In the rest of this method, there are too many ifs. After the cast to Product, they can be avoided like this:

return Objects.equals(this.ean, other.ean) 
       && this.hope == other.hope
       && Objects.equals(this.name, other.name);

Validation

To make your field validation conditions more readable, I would suggest to create two validation methods:

private static boolean isLengthGreaterThan(Object obj, int maxLength) {
    return String.valueOf(obj).length() > maxLength;
}

private static boolean isLengthLessThan(Object obj, int minLength) {
    return String.valueOf(obj).length() < minLength;
}

and call them to validate your fields, for example, in setHope(arg):

if (isLengthGreaterThan(hope, MAX_AMOUNT_OF_CIPHERS_HOPE)) {
    throw new DomainException(ErrorMessages.PRODUCT_HOPE_MAX_LENGTH);
}
if (isLengthLessThan(hope, MIN_AMOUNT_OF_CIPHERS_HOPE)) {
    throw new DomainException(ErrorMessages.PRODUCT_HOPE_MIN_LENGTH);
}

By the way, the name field is not validated against nullability: the NullPointerException in the first name.replaceAll call will not be as explicit as the other ones thrown by the validation sequences.

Constructors Chaining

Since you have more than one constructor in Product class, they should be chained by using this(), for example:

public Product() throws DomainException {
    this(null, 0, null);
}

public Product(Long ean, int hope, String name) throws DomainException {
    setEan(ean);
    setHope(hope);
    setName(name);
}

but... there is a problem, because the field values are validated and nulls are not accepted, unless acceptable values are set by default! I'd suggest to remove the 3-args constructor and always use the no-args one.

Long vs long

Is there a particular reason why Product.ean is of Long type, instead of long ?

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Wow, that's very useful information. I'll take try to make the changes. There is no reason why I use Long instead of long. Should the validation methods be moved to a seperate class (reuse in other classes)? \$\endgroup\$ – Mithos-Yggdrasill Sep 5 '15 at 17:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.