Multiple calls to a void method using list parameter as return value is better than a method that return a List?

Question

In short, my question is: If a method is called multiple times, is it better in terms of memory consumption to make it void and use a List as parameter to return its values? In case it really saves memory, isn't it a bad practice as the code is harder to read?

Let me provide an example to make it clearer. Suppose I have a class Car and each car must belong to a brand. I have a method that returns all the cars from a list of brands and this method uses a foreach and a method that retrieves all the cars from one brand. Like the following code:

private List<Car> getCarsByBrands(List<Brand> brands) {
    List<Car> result = new Arraylist<>;
    for(Brand brand : brands) {
        result.add(getCarsBySingleBrand(brand))
    }
    return result;
}

private List<Car> getCarsBySingleBrand(Brand brand) {
    List<Car> result = new Arraylist<>;
    result.add(retrieveCarsByBrand(brand)) // retrieveCarsByBrand omitted
    return result;
}

A colleague of mine defends that the method getCarsBySingleBrand should be rewritten to be void and use a List as parameter and this list would contain all the cars as you can see below:

private List<Car> getCarsByBrands(List<Brand> brands) {
    List<Car> result = new Arraylist<>;
    for(Brand brand : brands) {
        getCarsBySingleBrand(result, brand))
    }
    return result;
}

private void getCarsBySingleBrand(List<Car> cars, Brand brand) {
    cars.add(retrieveCarsByBrand(brand)) // retrieveCarsByBrand omitted
}

He argues that this way it consumes less memory, because he does not create a List everytime the method getCarsBySingleBrand is called. I think this is a kind of unnecessary optimization and is a bad practice because the code is harder to understand.

Answer 1

The second option MIGHT be more optimal.

The issue is the first way not only has to make another List object for every brand, which is then just thrown away but if there are a lot of cars for a brand, then Java will resize the list (which is initialized to the default size of 16) many times. The resizing operation requires copying the array. This could get expensive if you resize many times.

The second option only has one list and since resizing usually doubles the capacity, it should have to resize fewer times than the first option.

However, this is getting into micro-optimizations. I wouldn't worry about this kind of thing unless you are noticing a performance issue and have done analysis to determine that this is a bottleneck.

If you are worried about the method name, I think having the word "get" in the name is throwing you off because it usually implies returning something. Naming it something like addBrandOfCarsTo() might make it read more like a sentence:

for(Brand brand : brands) {
    addBrandOfCarsTo(result, brand));
}

Answer 2

Both options are good, and depend mostly of your preferences:

• some preople prefer clarity (and each people have his own style).
• others prefer preformance, however small it.

If your case you could also call directly to retrieveCarsByBrand and uses its result:

private List<Car> getCarsByBrands(List<Brand> brands) {
    List<Car> result = new Arraylist<>;
    for(Brand brand : brands) {
        result.add(retrieveCarsByBrand(brand))
    }
    return result;
}

Or simplify getCarsBySingleBrand and uses its result:

private List<Car> getCarsByBrands(List<Brand> brands) {
    List<Car> result = new Arraylist<>;
    for(Brand brand : brands) {
        result.add(retrieveCarsByBrand(brand));
    }
    return result;
}

private List<Car> getCarsBySingleBrand(Brand brand) {
    return retrieveCarsByBrand(brand);
}

or do a compromise between clarity and performance changing the name of getCarsBySingleBrand:

private List<Car> getCarsByBrands(List<Brand> brands) {
    List<Car> result = new Arraylist<>;
    for(Brand brand : brands) {
        getCarsBySingleBrand(result, brand))
    }
    return result;
}

private void addBrandCars(List<Car> cars, Brand brand) {
    cars.add(retrieveCarsByBrand(brand)) // retrieveCarsByBrand omitted


}

Answer 3

I guess you can use both worlds. The only advantage in the second one is it consumes less memory. Making a functional method instead a void one will increase clarity, as said before, and will help in unit testing (you can mock the returned value). Here is how I would do it:

private List<Car> getCarsByBrands(List<Brand> brands) {
    List<Car> result = new Arraylist<>;
    for(Brand brand : brands) {
        getCarsBySingleBrand(result, brand))
    }
    return result;
}

private List<Car> addBrandCars(List<Car> cars, Brand brand) {
    cars.add(retrieveCarsByBrand(brand)) // retrieveCarsByBrand omitted
    return cars;
}