provide initialization of oldValue array property in SetAttributesCommand

[nprigour]

oldValue array is not initialized during command construction time resulting in a NPE when trying to set values to an array element during run() or rollback method

Signed-off-by: Nikolaos Pringouris nprigour@gmail.com

[fgdrf]

Maybe its worth to check for null of value here. Looks like the only caller is EditFeature where value is set. However this is an implementation detail but from javadoc constructor argument description it could be possible if somebody like to implement to "clean"/"empty" attributes.

I would suggest to check for null in both constructors or even delegate from the second to the first one using:

public SetAttributesCommand( String xpath[], Object value[] ) {
        this(new EditFeatureProvider(this), new EditLayerProvider(this), xpath, value); 
}

[nprigour]

Hi @fgdrf,

• Delegation the way you specify it is not possible since we cannot refer to 'this' while explicitly invoking a constructor.
• Regarding null value check what can be done is to put an Assert.isNotNull for both xpath[] and value[] at the beginning of each constructor, since providing null values for these parameters is more or less meaningless for this specific command and should be not permitted (the subsequent execution of the run method will fail). Generally if a user wants to clean/empty attributes he should set a value[] array with the proper length and null values for each array element desired to be cleaned

If this will this suffice your request I will add the Assertion checks asap.

[fgdrf]

Delegation the way you specify it is not possible since we cannot refer to 'this' while explicitly invoking a constructor.

Oh yes, haven't seen this. However, the first Constructor is never used in codebase. I'll provide a pull request to eliminate it.

I'm wondering a bit why value.length is used while the loop

		// for loop to get the old values
		for(int i = 0; i < xpath.length; i++){
		    oldValue[i] = feature2.getAttribute(xpath[i]);
		}

uses xpath.lenght. I assume that object-Array might have same length but who knows ;)

[fgdrf]

see pull nprigour#10 for validation. I would merge afterwards..

[nprigour]

Ok with the validations. My only objection is about removing of the unused constructor. In my opinion, although not used at the moment it might be worth to keep it assuming a more complete SDK. After all SetAttributesCommand class is a generalization of SetAttributeCommand allowing for multiple attribute sets. There a similar constructor does exist.

[fgdrf]

Going to change the pull ;)

[nprigour]

I just rebased and merged your pull request