How to create a deep copy of a passed array if the array is valid?

Question

SO an IPv4 array is passed for this method, and if valid, a deep copy of the array is created in the instance variable "parts"

/** * If the ip address from the array passed (data) is valid, * makes a deep copy of the array passed in the instance variable parts. * For example, if data = {192,168,0,1}, parts should become {192,168,0,1} * by copying each item of data into corresponding item in parts. * If the ip address passed is invalid (for example {500,4,60,216} * or {192,16,01}, or {13,13,13,13,13}, parts should become {0,0,0,0} * * remember to reset the instance array parts before you do anything else * @param data */ public void setParts(int[] data) { this.parts = new int[4]; if (data.length != 4){ parts = new int[]{0,0,0,0}; } else for (int i = 0; i <= data.length; i++) if ((data[i] < 0) || (data[i] > 255)) parts = new int[]{0,0,0,0}; else parts[i] = data[i]; } 

- that’s all I have so far. What can i skip?

EDIT: one simple change made:

 for (int i = 0; i <= data.length; i++) 

to

 for (int i = 0; i < data.length; i++) 

And the JUnit test

  public void testSetPartsIntArray() { correct1.setParts(new int[]{12, 14, 16, 18}); int[] a = correct1.getParts(); assertEquals(4, a.length); assertEquals(12, a[0]); assertEquals(14, a[1]); assertEquals(16, a[2]); assertEquals(18, a[3]); correct1.setParts(new int[]{-12, 14, 16, 18}); a = correct1.getParts(); assertEquals(4, a.length); assertEquals(0, a[0]); assertEquals(0, a[1]); assertEquals(0, a[2]); assertEquals(0, a[3]); 

UNTIL works

 assertEquals(0, a[1]); 

What makes him stop there?

Answer 1

Once you find an invalid value, you should exit the loop:

 for (int i = 0; i <= data.length; i++) if ((data[i] < 0) || (data[i] > 255)) { parts = new int[]{0,0,0,0}; break; } else { parts[i] = data[i]; } 

In the failed case, the first -12 element is invalid, so parts set to new int[]{0,0,0,0} , but then you continue the loop, and since the remaining numbers are valid, you get {0, 14, 16, 18} instead of {0, 0, 0, 0} .

Answer 2

Eran solved your direct problem, but keep in mind: you still use bad abstractions, and your design should be improved in the following aspects:

• Do not use an int array to represent an IP address. For this purpose, Java INetAddress . There is no point in using an array of 4 elements to represent this information. You see, you are limiting yourself to IPv4. 4 ints do not for IPv6. So you have to throw it all away when someone asks you to work with IPv6!
• You mix duties. It seems that setParts () is used to customize the parts of the field in your class. But actually this method has valide input and setting . And even worse: if the incoming address is invalid, you will not notice. Well, you redefine the elements of the array to be all 0, but the address obtained from this 0.0.0.0 is still not valid! Therefore, you will need to check all over the place to make sure your address is valid. Do not do this. It would be better if A) a separate input check from setting your field, and B) not allowing your parts of the field to represent an invalid IP address.

You see, if you were to use the INetAddress class and you would pass a string with an invalid address; this class will throw an exception. And this is actually better than just turning something into 0.0.0.0.

Because there is no good reason to say that “this thing here means X, but for convenience we also let it understand“ broken X. ”Just: do not do this. Your application becomes much more reliable when you do not allow“ invalid ”data to move Otherwise, as said, each piece of code associated with the parts must be prepared so that it equals 0.0.0.0, so you need error handling for this ... everywhere!

And finally: what if there is a typo somewhere. Someone wanted to transmit 128.192.168.1, but dialed 128.912.168.1. Should this user better say that he provided unexpected input ?!