Creating Object List in Java class - better approach

Question

A have a question related to creating object list in Java class. Can someone tell me which solution is better? Pros and cons of it?

1) My first version of class:

public class ProductRepositoryImpl implements ProductRepository {

    private List<Product> listOfProducts = new ArrayList<Product>();

    public ProductRepositoryImpl() {

        addProducts(1, "Silnik", "Ferrari", 1000, listOfProducts);
        addProducts(2, "Sprzęgło", "Opel", 500, listOfProducts);
        addProducts(3, "Kierownica", "Fiat", 100, listOfProducts);
        addProducts(4, "Panewka", "Maluch", 250.00, listOfProducts);
        addProducts(5, "Akumulator", "Autosan", 1700.00, listOfProducts);
        addProducts(6, "Zakrętka", "Maseratii", 100.00, listOfProducts);
    }

    private void addProducts(int idProduct, String name, String brand, double price, List list) {
        Product product = new Product();
        product.setId(idProduct);
        product.setName(name);
        product.setBrand(brand);
        product.setPrice(price);
        list.add(product);
    }

    @Override
    public List<Product> getListOfProducts() {    
        return listOfProducts;
    }

 }

2) And the second one:

public class ProductRepositoryImpl implements ProductRepository {

    private List<Product> listOfProducts = new ArrayList<Product>();

    public ProductRepositoryImpl() {

        Product p1 = new Product();
        p1.setId(1);
        p1.setName("Silnik");
        p1.setBrand("Ferrari");
        p1.setPrice(1000);

        Product p2 = new Product();
        p2.setId(2);
        p2.setName("Hamulec");
        p2.setBrand("Opel");
        p2.setPrice(500);

        Product p3 = new Product();
        p3.setId(3);
        p3.setName("Kierownica");
        p3.setBrand("Fiat");
        p3.setPrice(100);

        Product p4 = new Product();
        p4.setId(4);
        p4.setName("Akumulator");
        p4.setBrand("Autosan");
        p4.setPrice(1700);

        Product p5 = new Product();
        p5.setId(5);
        p5.setName("Zakrętka");
        p5.setBrand("Maseratii");
        p5.setPrice(100);

        listOfProducts.add(p1);
        listOfProducts.add(p2);
        listOfProducts.add(p3);
        listOfProducts.add(p4);
        listOfProducts.add(p5);
    }

    @Override
    public List<Product> getListOfProducts() {
        return listOfProducts;
    }
}

I will be grateful for every explaination.

Answer 1

In general the first design is better. It reduces code duplication. On the other hand you can improve it further.

• It seems that your object stores only hardcoded values. You should consider moving this code into .property files
• Returning immutable version of your list. It might prevent some unpleasant surprises
• If you want to perceive this object as a true repository you should provide method which will have some fetching method by specification/filter. Currently it does not provide encapsulation.

Answer 2

There are some design guide lines which will help you in improving your code :

• Separation of responsibilities. ProductRepositoryImpl should be responsible for dealing with a repository of products. It is not its responsibility and should not have code for constructing Products. Construction of products should be the responsibility of Product
• Code reuse. Version 1 is better than version 2 since the code for construction of a product is written once, inside addProducts(), rather than multiple times as in version 2.
• Encapsulation. The interface of a class is the only part which should be public. It's implementation should be hidden. This means that the id field of a product should not be directly accesed from the outside, but instead should be accesed through interface methods. The advantage being that if you later on need to change how ids internally work you can do it easily because all ids come and go from a few methods in the Product class. If the id field is public then there may be hundreds of places accessing it and dealing with such a change would be a nightmare.

Another issue is mutability. Your Product class is mutable, as implied by the setName method. If that is an absolute requirement of your application go ahead. But if a product does not change once defined you should rather make Product inmutable. Inmutability has several advantages, one of them being that it is thread safe without needing synchronization. So I have made Product inmutable.

With those guide lines in mind this is the approach I would take :

class Product
{
    private final int idProduct;
    private final String name;
    private final String brand;
    private final double price;

    public Product(int idProduct, String name, String brand, double price)
    {
        this.idProduct = idProduct;
        this.name = name;
        this.brand = brand;
        this.price = price;
    }
}

public class ProductRepositoryImpl implements ProductRepository
{
    private List<Product> listOfProducts = new ArrayList<Product>();

    public ProductRepositoryImpl()
    {
        addProduct(new Product(1, "Silnik", "Ferrari", 1000));
        addProduct(new Product(2, "Sprzęgło", "Opel", 500));
        addProduct(new Product(3, "Kierownica", "Fiat", 100));
        addProduct(new Product(4, "Panewka", "Maluch", 250.00));
        addProduct(new Product(5, "Akumulator", "Autosan", 1700.00));
        addProduct(new Product(6, "Zakrętka", "Maseratii", 100.00));
    }

    private void addProduct(Product product)
    {
        listOfProducts.add(product);
    }

    @Override
    public List<Product> getListOfProducts()
    {
        return listOfProducts;
    }
}

Answer 3

The first option denotes a Factory and is usually recommended.

The reason why it is recommended is because object initialization is localized to one central place, thus if you need to perform any extra checks before the initialization, such design would ensure that you need only make changes to one portion of the code.

As a side note, in the example you posted, it woon't really make much of a difference since should the structure of the object change, you would only need to add an extra setter instead of passing an extra parameter.

However, in other scenarios object creation would depend on other objects. Thus, the pattern would allow you to make the least amount of changes, thus reducing the chances of introducing new bugs in the code because you forgot to update that one line burried somewhere in your code.

Also as it has been pointed out in the comments, addProducts should really become addProduct, since it is adding only one item.