-1

As I was developing something, I wondered what might be the better/faster approach to create a bunch of objects while iterating over something.

Lets say we have this wrapper-class:

public class Photo{
    private String url;
    private String creator;

    public Photo(String url, String creator){
        this.url = url;
        this.creator = creator;
    }
    // Getter methods down here...
}

And we have a JSONArray of photos from which we want only the url- and the creator-strings:

JSONArray photos = json.getJSONArray("photos");
Photo[] photo_arr = new Photo[photos.length()];
// Iterate:
for (int i = 0; i < photos.length(); i++){
    // Create the objects here.
}

Now, I see three possible solutions:

Creating temporary variables

Creating some temporary variables which get the desired values from the current object and then construct the new Photo-object:

// Iterate:
String url = "";
String creator = "";
for (int i = 0; i < photos.length(); i++){
    url = photos[i].getString("url");
    creator = photos[i].getString("creator");
    photo_arr[i] = new Photo(url, creator);
}

Use the returned-values directly in the constructor

Don't create temporary variables but use the returned values from the getString()-method in the constructor-call:

// Iterate:
for (int i = 0; i < photos.length(); i++){
    photo_arr[i] = new Photo(
        photos[i].getString("url"),
        photos[i].getString("creator")
    );
}

Using setter-methods

Adding a no-parameter constructor and setter-methods for the url and creator to the wrapper-class and use them to populate the object:

// Iterate:
for (int i = 0; i < photos.length(); i++){
    photo_arr[i] = new Photo();
    photo_arr[i].setUrl( photos[i].getString("url") );
    photo_arr[i].setCreator( photos[i].getString("creator") );
}

Which one is the better/faster/cleaner approach here?

Cœur
  • 37,241
  • 25
  • 195
  • 267
Lukas Knuth
  • 25,449
  • 15
  • 83
  • 111

2 Answers2

3

The two first methods are similar. Introduce variables if you feel it's more readable and maintainable. The last one makes the Photo class mutable whereas it was immutable in the first two methods (not in the strictest meaning used in concurrency, though).

You should always favor immutability when possible, mainly because it makes programs more robust: a Photo is always in a usable, fully constructed state in the first two cases. Not in the third one.

Note that performance is irrelevant here: the three ways will certainly lead to comparable times, and it's not that kind of code that will make a program slow. Don't optimize prematurely. It's the root of all evil.

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • +1 for immutability and not caring about intermediate variables that would be optimized out anyway. /* BTW, one can create immutable objects with chain calls returning a new object every time, but this usually makes sense only when such a call seriously changes the object. See e.g. http://blog.zilverline.com/2011/02/05/towards-an-immutable-domain-model-immutability-achieved-part-3/ */ – 9000 Oct 09 '11 at 19:09
2

The difference in speed is negligible between the solutions you've provided for the code you're looking at here. Readability is actually more important than one might think. Is it more important to you that your end-users experience a 1% speed increase? Or that you will be able to maintain this code in future versions? The answer is usually the latter.

For that reason, I'd go with Creating temporary variables. Using setter-methods is (in my opinion) less readable, but potentially slower since you will have to enter each of those setter methods and allocate those resources. Us[ing] the returned-values directly in the constructor is (again, in my opinion) less readable and should therefore be avoided in cases like this; but if the returned object was something of a larger size, then allocating the resources to store it in a temporary value may actually be non-negligible.

Jon Newmuis
  • 25,722
  • 2
  • 45
  • 57