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.

The first design is better, but in reality this is based entirely on opinion. But the first solution is more organized and also more reusable compared to the second, which has a lot of repetitive code. A third option, which is probably even better, is to make a constructor in the Product class that takes in those parameters. — Zarwan
– Zarwan, Commented Aug 31, 2015 at 6:56
It the add method adds only one product, why is it called addProducts instead of addProduct ;) ? — piezol
– piezol, Commented Aug 31, 2015 at 6:58
In addProducts() method no need to pass listOfProducts — Bacteria
– Bacteria, Commented Aug 31, 2015 at 7:01
@piezol 32 yes it's my mistake. But i was excepting different answer:) — piotrowicki
– piotrowicki, Commented Aug 31, 2015 at 7:01

Karthik R · Accepted Answer · 2015-08-31 07:38:56Z

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;
    }
}

npinti · Accepted Answer · 2015-08-31 07:05:43Z

0

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.

answered Aug 31, 2015 at 7:05

npinti

52.2k5 gold badges74 silver badges98 bronze badges

2 Comments

piotrowicki Over a year ago

Now i see. It 's means that this approach its closer to encapsulation. You wrote that I should set extra setter instead of passing an parameter.. its refers to passing list to addProducts method?

npinti Over a year ago

@cichacz: Yes this could lead to encapsulation since you are hiding the initialization. What I meant about the extra parameter is that if now your Product also needs a stock keeping number, then in your first case you would need to pass the extra field to the method. In the second case, you would need to do something along the lines of .setSKU("...") for each item you have.

Marcin Szymczak · Accepted Answer · 2015-08-31 08:05:58Z

0

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.

answered Aug 31, 2015 at 8:05

Marcin Szymczak

11.5k5 gold badges58 silver badges65 bronze badges

Collectives™ on Stack Overflow

Creating Object List in Java class - better approach

3 Answers 3

Comments

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related