CODEWITHSUNDEEP
java
user3079090
user3079090

Reputation: 13

Why does my Java deep copy code tell me "Assignment requires a deep, not shallow copy"?

I am trying to submit this sound manipulation program, but I keep getting this error:

Assignment requires a deep, not shallow copy. Are you just copying pointers, or copying the contents of the array?...

I think I am doing a shallow copy on my public void set(double[] mySamples), but I am new to Java and really don't know what to do.

public class Sound
{
    private double[] temp;
    private double[] samples;

    public Sound() {
        samples = null;
    }

    public Sound(Sound s) {
        int j = 0;
        double[] source = s.samples;
        double temp[] = new double[source.length];
        for(int i = 0; i < source.length; i++) {
            temp[j] = source[i];
        }
        samples = temp;
    }

    public double[] get() {
        return samples;
    }

    public void set(double[] mySamples) {
        if (mySamples == null) {
            throw new IllegalArgumentException("samples cannot be null");
        } else {
            samples = mySamples;
        }
    }


    public void increaseVol(double percent) {
        double [] result = new double[samples.length];
        for (int i = 0; i < samples.length; i++) {
            double reduce = samples[i] * percent;
            result[i] = samples[i] + reduce;
        }
        samples = result;
    }

    public void wavSave(java.lang.String fileName) {
        WavIO.write(fileName, samples);
    }
}

Upvotes: 0

Views: 100

Answers (3)

scottb
scottb

Reputation: 10084

Primitive arrays and arrays of autoboxed reference types (from primitives) are final and immutable. Therefore, they are ideal candidates for the clone() method.

You can edit this code:

public void set(double[] mySamples) {
    if (mySamples == null) {
        throw new IllegalArgumentException(
            "samples cannot be null");
    } else {
        samples = new double[mySamples.length];
        for (int i = 0; i < mySamples.length; i++) {
            samples[i] = mySamples[i];
        }
    }
}

into this with the same functionality and superior performance:

public void set(double[] mySamples) {
    if (mySamples == null || mySamples.length < 1) {
        throw new IllegalArgumentException(
            "samples cannot be null or empty");
    }
    samples = mySamples.clone();
}

This works safely because doubles are immutable and primitive and so a shallow copy is the same as a deep copy in this case. It's also safe because double cannot be subclassed and so no mischief is possible with clone().

Moreover, the performance of clone() on arrays is much, much faster than iterating over an array in code and copying element by element.

TL;DR: Making a copy of an array of primitives (or an array of the corresponding autoboxed reference types that correspond to a primitive) is quite possibly the only case in which it is ideal to use the clone() method.

Upvotes: 0

Stuart Marks
Stuart Marks

Reputation: 132450

Yes, you're doing "shallow" copies. Arrays in Java are like pointers, in that if you return or set an array, it's just a pointer (really, reference) to that array. With your current code it's pretty easy for different Sound objects to reference the same underlying array, which is probably not what you want.

In the set method, you need to copy the contents of the array instead of just copying the reference to that array.

You do copy the contents of the array properly in the Sound(Sound s) constructor. However, you don't need to manually allocate a new array and write a for-loop to copy the values. Just call java.util.Arrays.copyOf() to copy it. You can use a similar technique in the set method.

The get method also has the issue where it's returning a reference to the array from inside your Sound object. The caller now has a reference to the Sound object's internal state and can manipulate it. That might or might not be what you want.

Upvotes: 0

Elliott Frisch
Elliott Frisch

Reputation: 201477

I think you need to make two changes -
1 Your constructor is doing something weird with those samples (and might be causing your error by itself) - I'm not sure what you were trying to do, but I think you should just defer to set.

public Sound(Sound s) {
  int j = 0;
  set(s.samples);
}

2 Just copy the samples array in set, and store it.

public void set(double[] mySamples) {
  if (mySamples == null) {
    throw new IllegalArgumentException(
        "samples cannot be null");
  } else {
    samples = new double[mySamples.length];
    for (int i = 0; i < mySamples.length; i++) {
      samples[i] = mySamples[i];
    }
  }
}

Upvotes: 1

Related Questions