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

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

Answer 2

As @dkatzel said, the second option might be better. But that's only because in the first option, in method getCarsBySingleBrand(Brand brand), you are unnecessarily copying retrieveCarsByBrand(brand) result to a fresh new ArrayList. This means that 3 different lists exist before the method getCarsBySingleBrand(Brand brand) returns:

• The one that holds the final result
• The sublist with the cars of one single brand, returned by retrieveCarsByBrand(brand)
• The new ArrayList you're using to copy the list returned by retrieveCarsByBrand(brand)

If, as suggested by @Narkha, in the first option, you simplified the method getCarsBySingleBrand(brand) by not copying the list of cars returned by retrieveCarsByBrand(brand) to a new ArrayList, or by just inlining retrieveCarsByBrand(brand) method, then both options would be the same, in terms of memory consumption. This is because in both approaches, the list of cars retrieved for a single brand would be copied to the list that holds the final result once per brand. This means that only 2 lists would exist at the same time:

• The one that holds the final result
• The sublist with the cars of one single brand, returned by retrieveCarsByBrand(brand)

As stated by others, this kind of optimization is hardly ever needed. However, you should know there's a more optimal way to do this, if you use Guava (you should!).

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

This solution stores a list of sublists of cars, where each sublist holds the cars of each brand, and at the end decorates the list of sublists of cars into a single Iterable of cars. If you really, really need to return a List instead of an Iterable, then you could modify the last line as follows:

    return Lists.newArrayList(Iterables.concat(carsGroupedByBrand));

The key word here is decorates, meaning no actual copy of any sublist is performed. Instead, a special-purpose iterator is created, which traverses every element of the first sublist, and when it reaches its end, it automatically and transparently 'jumps' to the first element of the second sublist, until it reaches its end, and so on, until it reaches the last element of the last sublist. Please refer to Guava's Iterables.concat(...) method documentation for further details.

Answer 3

He is might be correct.

Let's explain WHY.

so in first example for each brand you are creating new list in getCarsBySingleBrand method and than you have to add cars from newly created list in result list being in getCarsByBrands.

result.add(getCarsBySingleBrand(brand))

in this line you just get list from getCarsBySingleBrand and add all those elements to result.

But in second case you add cars directly to getCarsByBrands list , so you do less operations.

Also It's correct to say that first example consumes slightly more memory. But I think more problem here is just operations , not memory.

Conclusion:

In my opinion second method is not really a bad style. So you can make such optimizations if it's needed.

Answer 4

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 5

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