Is it better to call instance methods or declare variables here?

Question

Here's two ways of approaching a method which merges two ordered sub-decks of playing cards into one ordered deck:

Approach 1:

public static Deck merge(Deck d1, Deck d2) {
  Deck result = new Deck(d1.cards.length + d2.cards.length);
  int i = 0;
  int j = 0;

  for (int k = 0; k < result.cards.length; k++) {
    if (j >= d2.cards.length || i < d1.cards.length && d1.cards[i].compareTo(d2.cards[j]) <= 0) {
      result.cards[k] = d1.cards[i];
      i++;
    } else {
      result.cards[k] = d2.cards[j];
      j++;
    }
  }
  return result;
}

Approach 2:

public static Deck merge(Deck d1, Deck d2) {
  Deck result = new Deck(l1+l2);
  Card[] c1 = d1.getCards();
  Card[] c2 = d2.getCards();
  int l1 = c1.length;
  int l2 = c2.length;
  Card[] sorted = new Card[l1+l2];
  int i = 0;
  int j = 0;

     for (int k = 0;k<sorted.length;k++){
        if (j >= c2.length || i < c1.length && c1[i].compareTo(c2[j]) <= 0){
           sorted[k] = c1[i];
           i++;
        } 
        else {
           sorted[k] = c2[j];
           j++;
        }
     }
  }
  result.cards = sorted;
  return result;
}

Which method is more efficient? Is there actually any difference?

From what I can tell the first method would have to generate a much larger amount of objects to complete a run for, let's say, two 26 card sub decks. However, the method itself would store less information which makes me question which method would be more efficient.

I know on this scale it probably doesn't matter too much, but as someone who's very new to Java, I'm curious to know what is best practice and why. I've tried searching for similar scenarios but haven't managed to find any. If anyone could point me in the right direction, it'd be greatly appreciated.

Can you post your Deck class? It would be useful to see what is that class about. — whiplash, May 13 '18 at 05:47
It's based on this class from Think Java https://github.com/AllenDowney/ThinkJavaCode/blob/master/ch13/Deck.java — Josh Hardman, May 13 '18 at 05:57
So are you asking whether you should put stuff into variables first then use the variables or not use variables at all? — Sweeper, May 13 '18 at 06:41
Approach two won't compile -- where is `deck` declared? Why do you think approach one creates more objects? It looks as though it creates fewer -- both create a new `Deck`, and approach two creates a `Card[]` as well. — tgdavies, May 13 '18 at 06:47
@ tgdavies There was a mistake in the return. Updated it to result. — Josh Hardman, May 13 '18 at 06:54
@ tgdavies .cards used throughout Approach 1 is a reference to a Card object. Here's the Card class if you're curious... https://github.com/AllenDowney/ThinkJavaCode/blob/master/ch12/Card.java — Josh Hardman, May 13 '18 at 06:58
@Sweeper I'm trying to understand the best way to instantiate the Card class in this situation. Card and Deck are both objects in this case. — Josh Hardman, May 13 '18 at 07:04
@JoshHardman You are not instantiating any `Card`s. No new `Card` object is created in any of the approaches. Read my answer. — Sweeper, May 13 '18 at 07:09
@Sweeper Is there not an explicit card object instantiation in method 2? Also wouldn't Card objects have to be instantiated for the .cards instance methods in method 1? — Josh Hardman, May 13 '18 at 07:32
@JoshHardman You are creating an array of cards in method 2, not cards themselves. The array is empty when it is first created. Then you fill the array with `Card` objects _which have already been created_. For both methods, yes there are `Card` objects being passed around, but they are not created by the `merge` method. They are already in the deck. — Sweeper, May 13 '18 at 07:35

Stephen C · Answer 1 · 2018-05-13T07:27:46.080

Since getCards() is simply returning a reference variable (i.e. it is not copying the cards array) the performance difference is likely to be minimal.

The only way be sure is to benchmark the two versions of the application. But if the difference measured is anything more than a couple of percentage points, the benchmark¹ is probably flawed!

I would advise not to waste your time "optimizing at this level" unless you have clear evidence that:

the code is too slow, and
you have clear evidence that the code that you are about to optimize is a substantial contributor to the slowness.

In other words, benchmark then profile then optimize the parts of the code that are worth optimizing.

^{1 - It is advisable to read up on how to write a sound benchmark before you leap into coding. Start with: How do I write a correct micro-benchmark in Java?.}

Sweeper · Accepted Answer · 2018-05-13T07:10:52.897

Your first approach will create two objects - the new Deck object, and the array storing the cards (the array created by the Deck constructor). Your second approach will create 3 objects - the new Deck, the array created by the Deck constructor, and also your array named sorted.

However, this does not really matter because you are not creating Card objects at all. You are just copying a reference of the Card objects in the two decks to the new deck! Assigning Cards to arrays doesn't create new cards. It's just like the following won't create two objects:

Object obj = new Object();
Object obj2 = obj;

So practically the two approaches are the same. I suggest you use the one that you find most readable.

Is it better to call instance methods or declare variables here?

2 Answers2